Add scoped class for overriding field trials.

webrtc/test/field_trial.cc

 /*
  *  Copyright (c) 2014 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 27 matching lines @@
   return it->second;
 }
 }  // namespace field_trial
 
 namespace test {
 // Note: this code is copied from src/base/metrics/field_trial.cc since the aim
 // is to mimic chromium --force-fieldtrials.
 void InitFieldTrialsFromString(const std::string& trials_string) {
   static const char kPersistentStringSeparator = '/';
 
+  // Catch an error if this is called more than once.
+  assert(field_trials_initiated_ == false);
   field_trials_initiated_ = true;
 
-  if (trials_string.empty()) {
-    field_trials_.clear();
+  if (trials_string == "")

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

nit: empty() is better than comparison

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

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

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

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?

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

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.
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).

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

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 :)
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.

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

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.
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*.
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? :)

     return;
-  }
 
   size_t next_item = 0;
   while (next_item < trials_string.length()) {
     size_t name_end = trials_string.find(kPersistentStringSeparator, next_item);
     if (name_end == trials_string.npos || next_item == name_end)
       break;
     size_t group_name_end = trials_string.find(kPersistentStringSeparator,
                                                name_end + 1);
     if (group_name_end == trials_string.npos || name_end + 1 == group_name_end)
       break;
@@ skipping 12 matching lines @@
     // Successfully parsed all field trials from the string.
     if (next_item == trials_string.length())
       return;
   }
   // LOG does not prints when this is called early on main.
   fprintf(stderr, "Invalid field trials string.\n");
 
   // Using abort so it crashs both in debug and release mode.
   abort();
 }
+
+ScopedFieldTrials::ScopedFieldTrials(const std::string& config)
+    : previous_field_trials_(field_trials_) {
+  assert(field_trials_initiated_);
+  field_trials_initiated_ = false;
+  field_trials_ = std::map<std::string, std::string>();

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

why not clear()?

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

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

+  InitFieldTrialsFromString(config);
+}
+
+ScopedFieldTrials::~ScopedFieldTrials() {
+  field_trials_ = previous_field_trials_;

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

also restore field_trials_initiated_ ?

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

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

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

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:17]

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]

Ah, I see.

+}
+
 }  // namespace test
 }  // namespace webrtc