Test field trial group with startswith rather than equals.

Test field trial group with startswith rather than equals.

BUG=webrtc:7266
Review-Url: https://codereview.webrtc.org/2717973005
Cr-Commit-Position: refs/heads/master@{#16915}
Committed: https://chromium.googlesource.com/external/webrtc/+/c1b57a15bf421503bb654fa731e06feee7d85694

[sprang_webrtc, 2017-02-28 10:31:57]

sprang@webrtc.org changed reviewers:
+ tommi@webrtc.org

[sprang_webrtc, 2017-02-28 10:31:58]

tommi@, can you have a look as root owner so that we don't need a dozen
reviewers?
This should essentially be a noop right now...

[tommi, 2017-02-28 10:51:43]

Can we use this opportunity to add a couple of methods for the two most common
cases?  It feels like we're making it too easy for us to shoot ourselves in the
foot as is with tiny little typos.  Also copy/pasting the code which is
essentially creating strings all over the place, seems suboptimal :)

What if, instead of this:

if (field_trial::FindFullName("WebRTC-QuickPerfTest").find("Enabled") != 0) {

We could do this?

if (field_trial::IsEnabled("WebRTC-QuickPerfTest")) {

(simple to implement, doesn't have to copy a string only for searching)

Same for "Disabled"

[sprang_webrtc, 2017-02-28 11:30:14]

On 2017/02/28 10:51:43, tommi (webrtc) wrote:
> Can we use this opportunity to add a couple of methods for the two most common
> cases?  It feels like we're making it too easy for us to shoot ourselves in
the
> foot as is with tiny little typos.  Also copy/pasting the code which is
> essentially creating strings all over the place, seems suboptimal :)
> 
> What if, instead of this:
> 
> if (field_trial::FindFullName("WebRTC-QuickPerfTest").find("Enabled") != 0) {
> 
> We could do this?
> 
> if (field_trial::IsEnabled("WebRTC-QuickPerfTest")) {
> 
> (simple to implement, doesn't have to copy a string only for searching)
> 
> Same for "Disabled"

Sure, we can add IsEnabled() as a convenience method, though we still need to
keep the old way in a few places where we use other names or parse data from
them.

[sprang_webrtc, 2017-02-28 11:42:31]

Added IsEnabled(), ptal

[tommi, 2017-02-28 11:44:16]

On 2017/02/28 11:30:14, språng wrote:
> On 2017/02/28 10:51:43, tommi (webrtc) wrote:
> > Can we use this opportunity to add a couple of methods for the two most
common
> > cases?  It feels like we're making it too easy for us to shoot ourselves in
> the
> > foot as is with tiny little typos.  Also copy/pasting the code which is
> > essentially creating strings all over the place, seems suboptimal :)
> > 
> > What if, instead of this:
> > 
> > if (field_trial::FindFullName("WebRTC-QuickPerfTest").find("Enabled") != 0)
{
> > 
> > We could do this?
> > 
> > if (field_trial::IsEnabled("WebRTC-QuickPerfTest")) {
> > 
> > (simple to implement, doesn't have to copy a string only for searching)
> > 
> > Same for "Disabled"
> 
> Sure, we can add IsEnabled() as a convenience method, though we still need to
> keep the old way in a few places where we use other names or parse data from
> them.

Yup - I was thinking having convenience methods for "Enabled" and "Disabled"
since those are so common.
Actually, a more efficient way than how we do the regular check could be done
too (I really dislike allocating strings for no reason).

bool b =
(webrtc::field_trial::FindFullName("WebRTC-SendSideBwe-WithOverhead").find("Enabled")
== 0)

could be done like so:

bool b = webrtc::field_trial::IsFullName("WebRTC-SendSideBwe-WithOverhead",
"Enabled")

[sprang_webrtc, 2017-02-28 11:44:22]

On 2017/02/28 11:42:31, språng wrote:
> Added IsEnabled(), ptal

I omitted the IsDisabled() case, which was used in only two places, one of which
I intend to remove.
If you do your own custom parsing, including default-off, I think it's fair that
you're on your own.

[tommi, 2017-02-28 11:52:41]

https://codereview.webrtc.org/2717973005/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.cc
(right):

https://codereview.webrtc.org/2717973005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.cc:57:
.find("Enabled"));
RTC_DCHECK(webrtc::field_trial::IsEnabled("WebRTC-SendSideBwe-WithOverhead")) ?

https://codereview.webrtc.org/2717973005/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right):

https://codereview.webrtc.org/2717973005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:187:
"WebRTC-SendSideBwe-WithOverhead").find("Enabled") == 0),
IsEnabled?

https://codereview.webrtc.org/2717973005/diff/20001/webrtc/system_wrappers/in...
File webrtc/system_wrappers/include/field_trial.h (right):

https://codereview.webrtc.org/2717973005/diff/20001/webrtc/system_wrappers/in...
webrtc/system_wrappers/include/field_trial.h:67: bool IsEnabled(const
std::string& name);
const char*

https://codereview.webrtc.org/2717973005/diff/20001/webrtc/system_wrappers/so...
File webrtc/system_wrappers/source/field_trial_default.cc (right):

https://codereview.webrtc.org/2717973005/diff/20001/webrtc/system_wrappers/so...
webrtc/system_wrappers/source/field_trial_default.cc:57: return
FindFullName(name).find("Enabled") == 0;
Even though FindFullName currently forces creation of a std::string for every
static string, I don't think that's a good interface design.  Most (all?)
callers pass a static string, so that should be the best supported case.

Since FindFullName still requires creating a std::string, I'd prefer to keep
that conversion as an implementation detail of IsEnabled for now.  FindFullName
should be changed.

[sprang_webrtc, 2017-02-28 12:18:31]

https://codereview.webrtc.org/2717973005/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.cc
(right):

https://codereview.webrtc.org/2717973005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/audio_network_adaptor/bitrate_controller.cc:57:
.find("Enabled"));
On 2017/02/28 11:52:41, tommi (webrtc) wrote:
> RTC_DCHECK(webrtc::field_trial::IsEnabled("WebRTC-SendSideBwe-WithOverhead"))
?

Done.

https://codereview.webrtc.org/2717973005/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc (right):

https://codereview.webrtc.org/2717973005/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/codecs/opus/audio_encoder_opus.cc:187:
"WebRTC-SendSideBwe-WithOverhead").find("Enabled") == 0),
On 2017/02/28 11:52:41, tommi (webrtc) wrote:
> IsEnabled?

Done.

https://codereview.webrtc.org/2717973005/diff/20001/webrtc/system_wrappers/in...
File webrtc/system_wrappers/include/field_trial.h (right):

https://codereview.webrtc.org/2717973005/diff/20001/webrtc/system_wrappers/in...
webrtc/system_wrappers/include/field_trial.h:67: bool IsEnabled(const
std::string& name);
On 2017/02/28 11:52:41, tommi (webrtc) wrote:
> const char*

Done.

https://codereview.webrtc.org/2717973005/diff/20001/webrtc/system_wrappers/so...
File webrtc/system_wrappers/source/field_trial_default.cc (right):

https://codereview.webrtc.org/2717973005/diff/20001/webrtc/system_wrappers/so...
webrtc/system_wrappers/source/field_trial_default.cc:57: return
FindFullName(name).find("Enabled") == 0;
On 2017/02/28 11:52:41, tommi (webrtc) wrote:
> Even though FindFullName currently forces creation of a std::string for every
> static string, I don't think that's a good interface design.  Most (all?)
> callers pass a static string, so that should be the best supported case.
> 
> Since FindFullName still requires creating a std::string, I'd prefer to keep
> that conversion as an implementation detail of IsEnabled for now. 
FindFullName
> should be changed.

That sounds good. 
Done.

[tommi, 2017-02-28 12:41:01]

lgtm

[tommi, 2017-02-28 12:41:15]

On 2017/02/28 12:41:01, tommi (webrtc) wrote:
> lgtm

and thanks for making the extra changes :)

[sprang_webrtc, 2017-02-28 12:45:19]

The CQ bit was checked by sprang@webrtc.org

[commit-bot: I haz the power, 2017-02-28 12:45:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-28 13:33:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-28 13:33:03]

Try jobs failed on following builders:
  linux_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)

[sprang_webrtc, 2017-02-28 13:35:05]

The CQ bit was checked by sprang@webrtc.org

[commit-bot: I haz the power, 2017-02-28 13:35:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-28 14:14:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-28 14:14:16]

Try jobs failed on following builders:
  linux_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)

[tommi, 2017-02-28 16:41:21]

The CQ bit was checked by tommi@webrtc.org

[commit-bot: I haz the power, 2017-02-28 16:41:29]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-02-28 16:50:49]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488300081121850,
"parent_rev": "f8ee65ead6e16cbb1c4311b9fd596a3435ec2f9a", "commit_rev":
"c1b57a15bf421503bb654fa731e06feee7d85694"}

[commit-bot: I haz the power, 2017-02-28 16:50:52]

Description was changed from

==========
Test field trial group with startswith rather than equals.

BUG=webrtc:7266
==========

to

==========
Test field trial group with startswith rather than equals.

BUG=webrtc:7266

Review-Url: https://codereview.webrtc.org/2717973005
Cr-Commit-Position: refs/heads/master@{#16915}
Committed:
https://chromium.googlesource.com/external/webrtc/+/c1b57a15bf421503bb654fa73...
==========

[commit-bot: I haz the power, 2017-02-28 16:50:53]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/webrtc/+/c1b57a15bf421503bb654fa73...

[danilchap, 2017-03-01 12:07:14]

danilchap@webrtc.org changed reviewers:
+ danilchap@webrtc.org

[danilchap, 2017-03-01 12:07:15]

https://codereview.webrtc.org/2717973005/diff/40001/webrtc/call/call_perf_tes...
File webrtc/call/call_perf_tests.cc (right):

https://codereview.webrtc.org/2717973005/diff/40001/webrtc/call/call_perf_tes...
webrtc/call/call_perf_tests.cc:293: if
(field_trial::IsEnabled("WebRTC-QuickPerfTest")) {
should this be negation?

https://codereview.webrtc.org/2717973005/diff/40001/webrtc/modules/remote_bit...
File webrtc/modules/remote_bitrate_estimator/test/bwe_test.cc (right):

https://codereview.webrtc.org/2717973005/diff/40001/webrtc/modules/remote_bit...
webrtc/modules/remote_bitrate_estimator/test/bwe_test.cc:383: if
(field_trial::IsEnabled("WebRTC-QuickPerfTest")) {
should this be negation?