Add scoped class for overriding field trials.

Add scoped class for overriding field trials.

To be used in tests that depend on specific field-trial settings without
overwriting the command-line flag for overriding field trials.

BUG=webrtc:4820
R=stefan@webrtc.org

Committed: https://crrev.com/19492f1c4c7411b7632fe34824226c1c579a6412
Cr-Commit-Position: refs/heads/master@{#9547}

[stefan-webrtc, 2015-07-07 14:28:25]

The CQ bit was checked by stefan@webrtc.org

[stefan-webrtc, 2015-07-07 14:28:25]

lgtm

[commit-bot: I haz the power, 2015-07-07 14:28:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1227653002/1

[commit-bot: I haz the power, 2015-07-07 15:22:31]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-07-07 15:22:40]

Patchset 1 (id:??) landed as
https://crrev.com/19492f1c4c7411b7632fe34824226c1c579a6412
Cr-Commit-Position: refs/heads/master@{#9547}

[tommi, 2015-07-08 09:01:34]

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

[tommi, 2015-07-08 09:01:35]

https://codereview.webrtc.org/1227653002/diff/1/webrtc/test/field_trial.cc
File webrtc/test/field_trial.cc (right):

https://codereview.webrtc.org/1227653002/diff/1/webrtc/test/field_trial.cc#ne...
webrtc/test/field_trial.cc:52: if (trials_string == "")
nit: empty() is better than comparison

https://codereview.webrtc.org/1227653002/diff/1/webrtc/test/field_trial.cc#ne...
webrtc/test/field_trial.cc:91: field_trials_ = std::map<std::string,
std::string>();
why not clear()?

https://codereview.webrtc.org/1227653002/diff/1/webrtc/test/field_trial.cc#ne...
webrtc/test/field_trial.cc:96: field_trials_ = previous_field_trials_;
also restore field_trials_initiated_ ?

[pbos-webrtc, 2015-07-08 12:10:36]

https://codereview.webrtc.org/1227653002/diff/1/webrtc/test/field_trial.cc
File webrtc/test/field_trial.cc (right):

https://codereview.webrtc.org/1227653002/diff/1/webrtc/test/field_trial.cc#ne...
webrtc/test/field_trial.cc:52: if (trials_string == "")
On 2015/07/08 09:01:35, tommi (webrtc) wrote:
> nit: empty() is better than comparison

I think == "" reads better. It's also under test code, so added code size /
cycles are negligible/only affect tests.

https://codereview.webrtc.org/1227653002/diff/1/webrtc/test/field_trial.cc#ne...
webrtc/test/field_trial.cc:91: field_trials_ = std::map<std::string,
std::string>();
On 2015/07/08 09:01:35, tommi (webrtc) wrote:
> why not clear()?

Brain fart, ptal: https://codereview.webrtc.org/1215713019

https://codereview.webrtc.org/1227653002/diff/1/webrtc/test/field_trial.cc#ne...
webrtc/test/field_trial.cc:96: field_trials_ = previous_field_trials_;
On 2015/07/08 09:01:35, tommi (webrtc) wrote:
> also restore field_trials_initiated_ ?

Not necessary (InitFieldTrialsFromString called from ctor), addressed with
comment + assert in above CL.

[tommi, 2015-07-08 12:21:12]

https://codereview.webrtc.org/1227653002/diff/1/webrtc/test/field_trial.cc
File webrtc/test/field_trial.cc (right):

https://codereview.webrtc.org/1227653002/diff/1/webrtc/test/field_trial.cc#ne...
webrtc/test/field_trial.cc:52: if (trials_string == "")
On 2015/07/08 12:10:36, pbos-webrtc wrote:
> On 2015/07/08 09:01:35, tommi (webrtc) wrote:
> > nit: empty() is better than comparison
> 
> I think == "" reads better. It's also under test code, so added code size /
> cycles are negligible/only affect tests.

Here are some reasons for why I don't agree:
* It's more code
* It's ambiguous (foo == "" is not the same thing as "" == foo)
* not clear if "" gets converted to std::string first
* this pattern doesn't work for regular zero terminated strings
* it's encouraged to use empty() in chromium for stl containers.
* there's no question what empty() does.

besides, why should we do different kind of string comparisons in test code than
production code?

https://codereview.webrtc.org/1227653002/diff/1/webrtc/test/field_trial.cc#ne...
webrtc/test/field_trial.cc:96: field_trials_ = previous_field_trials_;
On 2015/07/08 12:10:36, pbos-webrtc wrote:
> On 2015/07/08 09:01:35, tommi (webrtc) wrote:
> > also restore field_trials_initiated_ ?
> 
> Not necessary (InitFieldTrialsFromString called from ctor), addressed with
> comment + assert in above CL.

I would still want the preconditions to mast the post conditions unless there's
a good reason for why it should not (I might be missing something though).

Should this not work?

{
  ScopedFieldTrials foo(...);
}

{
  ScopedFieldTrials bar(...);
}

Why should that not work?

[pbos-webrtc, 2015-07-08 12:44:18]

https://codereview.webrtc.org/1227653002/diff/1/webrtc/test/field_trial.cc
File webrtc/test/field_trial.cc (right):

https://codereview.webrtc.org/1227653002/diff/1/webrtc/test/field_trial.cc#ne...
webrtc/test/field_trial.cc:52: if (trials_string == "")
On 2015/07/08 12:21:12, tommi (webrtc) wrote:
> On 2015/07/08 12:10:36, pbos-webrtc wrote:
> > On 2015/07/08 09:01:35, tommi (webrtc) wrote:
> > > nit: empty() is better than comparison
> > 
> > I think == "" reads better. It's also under test code, so added code size /
> > cycles are negligible/only affect tests.
> 
> Here are some reasons for why I don't agree:
> * It's more code
> * It's ambiguous (foo == "" is not the same thing as "" == foo)
> * not clear if "" gets converted to std::string first
> * this pattern doesn't work for regular zero terminated strings
> * it's encouraged to use empty() in chromium for stl containers.
> * there's no question what empty() does.

So my argument is readability (which is personal). I read that '== ""' shows
that lhs is a string, whereas .empty() looks like a stl container.

.empty(): looks like it's called on a container, "": looks like a string.

> besides, why should we do different kind of string comparisons in test code
than
> production code?

If it generates different code that matters more for production. I'd hope that a
compiler could optimize std::string == "" to std::string.empty(), but that's
more of a wish and I'm not sure they're allowed to (or ever do).

https://codereview.webrtc.org/1227653002/diff/1/webrtc/test/field_trial.cc#ne...
webrtc/test/field_trial.cc:96: field_trials_ = previous_field_trials_;
On 2015/07/08 12:21:12, tommi (webrtc) wrote:
> On 2015/07/08 12:10:36, pbos-webrtc wrote:
> > On 2015/07/08 09:01:35, tommi (webrtc) wrote:
> > > also restore field_trials_initiated_ ?
> > 
> > Not necessary (InitFieldTrialsFromString called from ctor), addressed with
> > comment + assert in above CL.
> 
> I would still want the preconditions to mast the post conditions unless
there's
> a good reason for why it should not (I might be missing something though).
> 
> Should this not work?
> 
> {
>   ScopedFieldTrials foo(...);
> }
> 
> {
>   ScopedFieldTrials bar(...);
> }
> 
> Why should that not work?

It works. field_trials_initiated_ doesn't need to be set to true as it's already
true after the ctor.

[tommi, 2015-07-08 13:36:59]

https://codereview.webrtc.org/1227653002/diff/1/webrtc/test/field_trial.cc
File webrtc/test/field_trial.cc (right):

https://codereview.webrtc.org/1227653002/diff/1/webrtc/test/field_trial.cc#ne...
webrtc/test/field_trial.cc:52: if (trials_string == "")
On 2015/07/08 12:44:18, pbos-webrtc wrote:
> On 2015/07/08 12:21:12, tommi (webrtc) wrote:
> > On 2015/07/08 12:10:36, pbos-webrtc wrote:
> > > On 2015/07/08 09:01:35, tommi (webrtc) wrote:
> > > > nit: empty() is better than comparison
> > > 
> > > I think == "" reads better. It's also under test code, so added code size
/
> > > cycles are negligible/only affect tests.
> > 
> > Here are some reasons for why I don't agree:
> > * It's more code
> > * It's ambiguous (foo == "" is not the same thing as "" == foo)
> > * not clear if "" gets converted to std::string first
> > * this pattern doesn't work for regular zero terminated strings
> > * it's encouraged to use empty() in chromium for stl containers.
> > * there's no question what empty() does.
> 
> So my argument is readability (which is personal). I read that '== ""' shows
> that lhs is a string, whereas .empty() looks like a stl container.

std::string is a container.
http://stackoverflow.com/questions/17259595/c-is-a-stdstring-a-container

is const char[] a string? consider this:

const char foo[10] = "";
if (foo == "")
  WillItRun();

Some compilers will even refuse to compile the condition :)

> 
> .empty(): looks like it's called on a container, "": looks like a string.
> 
> > besides, why should we do different kind of string comparisons in test code
> than
> > production code?
> 
> If it generates different code that matters more for production. I'd hope that
a
> compiler could optimize std::string == "" to std::string.empty(), but that's
> more of a wish and I'm not sure they're allowed to (or ever do).

If readability is a concern, wouldn't it be better to be consistent?  To me
comparing to "" is less readable since it's not clear what's going to happen.
You have to check the type first and then know how the comparison operator is
implemented.

In fact, this pattern should always raise an alarm with reviewers.
Days since we last had a bug because of it==1. 
https://codereview.webrtc.org/1229443002/

The compiler argument would be valid if we knew for sure the compiler would
optimize this away and that all compilers we use, would.  That would however
mean the compiler would know more than it's supposed to.  In practice, this
would rather be handled at the library level and then by checking for the global
pointer to "" first and then checking empty().  Still always one more step.

https://codereview.webrtc.org/1227653002/diff/1/webrtc/test/field_trial.cc#ne...
webrtc/test/field_trial.cc:96: field_trials_ = previous_field_trials_;
On 2015/07/08 12:44:17, pbos-webrtc wrote:
> On 2015/07/08 12:21:12, tommi (webrtc) wrote:
> > On 2015/07/08 12:10:36, pbos-webrtc wrote:
> > > On 2015/07/08 09:01:35, tommi (webrtc) wrote:
> > > > also restore field_trials_initiated_ ?
> > > 
> > > Not necessary (InitFieldTrialsFromString called from ctor), addressed with
> > > comment + assert in above CL.
> > 
> > I would still want the preconditions to mast the post conditions unless
> there's
> > a good reason for why it should not (I might be missing something though).
> > 
> > Should this not work?
> > 
> > {
> >   ScopedFieldTrials foo(...);
> > }
> > 
> > {
> >   ScopedFieldTrials bar(...);
> > }
> > 
> > Why should that not work?
> 
> It works. field_trials_initiated_ doesn't need to be set to true as it's
already
> true after the ctor.

Ah, I see.

[pbos-webrtc, 2015-07-08 15:49:46]

https://codereview.webrtc.org/1227653002/diff/1/webrtc/test/field_trial.cc
File webrtc/test/field_trial.cc (right):

https://codereview.webrtc.org/1227653002/diff/1/webrtc/test/field_trial.cc#ne...
webrtc/test/field_trial.cc:52: if (trials_string == "")
On 2015/07/08 13:36:59, tommi (webrtc) wrote:
> On 2015/07/08 12:44:18, pbos-webrtc wrote:
> > So my argument is readability (which is personal). I read that '== ""' shows
> > that lhs is a string, whereas .empty() looks like a stl container.
> 
> std::string is a container.
> http://stackoverflow.com/questions/17259595/c-is-a-stdstring-a-container
> 
> is const char[] a string? consider this:
> 
> const char foo[10] = "";
> if (foo == "")
>   WillItRun();
> 
> Some compilers will even refuse to compile the condition :)

I like those compilers.

No I'm not saying that I'm technically correct, or that it's a good idea for all
cases (C (and Java) strings are scary if you're not hard-wired to do strcmp( or
.isEqual)). I just view them as semantically separate types, a string is one
thing, but a std::vector<char> is a container of many things, even though
they're technically both containers.

I compare it similarly to using if (my_int != 0) instead of if (my_int), since
!= 0 conveys the integer type, whereas if (my_int) looks like a bool. Similarly
I think that .empty() conveys a container, and I don't semantically view string
as a container even though it technically is.

> > 
> > .empty(): looks like it's called on a container, "": looks like a string.
> > 
> > > besides, why should we do different kind of string comparisons in test
code
> > than
> > > production code?
> > 
> > If it generates different code that matters more for production. I'd hope
that
> a
> > compiler could optimize std::string == "" to std::string.empty(), but that's
> > more of a wish and I'm not sure they're allowed to (or ever do).
> 
> If readability is a concern, wouldn't it be better to be consistent?  To me
> comparing to "" is less readable since it's not clear what's going to happen.
> You have to check the type first and then know how the comparison operator is
> implemented.
> 
> In fact, this pattern should always raise an alarm with reviewers.
> Days since we last had a bug because of it==1. 
> https://codereview.webrtc.org/1229443002/

That feels more like pay attention to C strings, which is generally valid. One
good argument for it would be that this code implicitly breaks if we change
const std::string& to a const char*.

> The compiler argument would be valid if we knew for sure the compiler would
> optimize this away and that all compilers we use, would.  That would however
> mean the compiler would know more than it's supposed to.  In practice, this
> would rather be handled at the library level and then by checking for the
global
> pointer to "" first and then checking empty().  Still always one more step.

Yeah, my argument is readability. Don't think we agree, but that's the argument
either way.

If this is a consistent pattern in chromium I think we can call it a style-guide
thing.

If it is: Wanna do a sweeping pass to remove all '== ""' occurrences/want me to
do to it? :)

[tommi, 2015-07-08 20:12:35]

On 2015/07/08 15:49:46, pbos-webrtc wrote:
> https://codereview.webrtc.org/1227653002/diff/1/webrtc/test/field_trial.cc
> File webrtc/test/field_trial.cc (right):
> 
>
https://codereview.webrtc.org/1227653002/diff/1/webrtc/test/field_trial.cc#ne...
> webrtc/test/field_trial.cc:52: if (trials_string == "")
> On 2015/07/08 13:36:59, tommi (webrtc) wrote:
> > On 2015/07/08 12:44:18, pbos-webrtc wrote:
> > > So my argument is readability (which is personal). I read that '== ""'
shows
> > > that lhs is a string, whereas .empty() looks like a stl container.
> > 
> > std::string is a container.
> > http://stackoverflow.com/questions/17259595/c-is-a-stdstring-a-container
> > 
> > is const char[] a string? consider this:
> > 
> > const char foo[10] = "";
> > if (foo == "")
> >   WillItRun();
> > 
> > Some compilers will even refuse to compile the condition :)
> 
> I like those compilers.

Me too.  Those that don't complain though, will compile code where WillItRun()
will in fact not run, so even though it's comparing a string in exactly the same
way you're comparing a string, the outcome of the comparison is opposite of what
it is in your case.

> No I'm not saying that I'm technically correct, or that it's a good idea for
all
> cases (C (and Java) strings are scary if you're not hard-wired to do strcmp(
or
> .isEqual)). I just view them as semantically separate types, a string is one
> thing, but a std::vector<char> is a container of many things, even though
> they're technically both containers.

Yeah, well, std::string is more than "technically" a container as if by
accident.  It's specifically designed to be a container.  See my previous reply.

> I compare it similarly to using if (my_int != 0) instead of if (my_int), since
> != 0 conveys the integer type, whereas if (my_int) looks like a bool.
Similarly
> I think that .empty() conveys a container, and I don't semantically view
string
> as a container even though it technically is.

OK, fwiw, I do understand where you're coming from and we wouldn't be having
this conversation if the code was python or visual basic :)
However, since "" is a string and it is also a char* and comparison doesn't work
this way for char*, a std::string is not a native type in C++.
Operator overloading was banned for many years for this reason.  Now it's only
discouraged ;)

> 
> > > 
> > > .empty(): looks like it's called on a container, "": looks like a string.
> > > 
> > > > besides, why should we do different kind of string comparisons in test
> code
> > > than
> > > > production code?
> > > 
> > > If it generates different code that matters more for production. I'd hope
> that
> > a
> > > compiler could optimize std::string == "" to std::string.empty(), but
that's
> > > more of a wish and I'm not sure they're allowed to (or ever do).
> > 
> > If readability is a concern, wouldn't it be better to be consistent?  To me
> > comparing to "" is less readable since it's not clear what's going to
happen.
> > You have to check the type first and then know how the comparison operator
is
> > implemented.
> > 
> > In fact, this pattern should always raise an alarm with reviewers.
> > Days since we last had a bug because of it==1. 
> > https://codereview.webrtc.org/1229443002/
> 
> That feels more like pay attention to C strings, which is generally valid. One
> good argument for it would be that this code implicitly breaks if we change
> const std::string& to a const char*.

"" is a C string of course and yup, and I think that's what happened in the cl I
pointed to.

> > The compiler argument would be valid if we knew for sure the compiler would
> > optimize this away and that all compilers we use, would.  That would however
> > mean the compiler would know more than it's supposed to.  In practice, this
> > would rather be handled at the library level and then by checking for the
> global
> > pointer to "" first and then checking empty().  Still always one more step.
> 
> Yeah, my argument is readability. Don't think we agree, but that's the
argument
> either way.

Well, I know you may find this a ridiculous example, but I've heard the same
arguments, readability and "the compiler should be able to optimize it" for
passing strings by value instead of by const&.  :)

In any case, I understand that point.  However, the string we're talking about
is universally known as "the empty string", so empty() isn't that hard to
understand.
https://en.wikipedia.org/wiki/Empty_string

So, in my opinion  == "" isn't any more readable than .empty(), .length == 0,
.size() == 0, all of which are used in the code base. .empty() is probably the
most common one to be used to test this condition if you include all the
containers. I don't think there's any way a programmer that works in this code
base to have questions about what .empty() does, but (as in this conversation),
there is when it comes to == "" and it can be the source of bugs as pointed out.
It's also the least efficient approach of all of the listed ones.  You know all
of this, so I think you're just pulling my leg.

> If this is a consistent pattern in chromium I think we can call it a
style-guide
> thing.

Hah, well, if you want to ignore the other things, fine with me.

> If it is: Wanna do a sweeping pass to remove all '== ""' occurrences/want me
to
> do to it? :)

Hmm, I'd be interested to know if you find any bugs if you take a look at those
places.

[pbos-webrtc, 2015-07-09 12:10:26]

Soo this is getting long, so I'll not reply inline because we've had enough
lines to rant I think. :D

I've no problem with .empty meaning empty string, it's just that it doesn't
convey the lhs type. (foo.empty() says nothing about foo being a vector, list or
string, whereas == "" conveys either a string or a bug.)

No bugs found (still compiles), but CL here, PTAL:
https://codereview.webrtc.org/1228913003/

[tommi, 2015-07-09 12:32:32]

On 2015/07/09 12:10:26, pbos-webrtc wrote:
> Soo this is getting long, so I'll not reply inline because we've had enough
> lines to rant I think. :D
> 
> I've no problem with .empty meaning empty string, it's just that it doesn't
> convey the lhs type. (foo.empty() says nothing about foo being a vector, list
or
> string, whereas == "" conveys either a string or a bug.)
> 
> No bugs found (still compiles), but CL here, PTAL:
> https://codereview.webrtc.org/1228913003/

Thanks!  Length of rants and conversations like this probably goes up with the
decreased importance of the issue. ;)