Add one-time randomization support for FieldTrial, and the ability to

Add one-time randomization support for FieldTrial, and the ability to
disable field trials. I am going to have a need for both soon.

Update some documentation about empty trial names, add TrialExists() method and update many call-sites to use this (it simplifies the previous logic which checked for existence and then for non-empty name, which can no longer happen).

Refactor a bit in browser_main.

While I'm in there and needing base/OWNERS approval, add an OWNERS
file for base/metrics that adds jar@chromium.org as an owner for that
directory.

Initially committed as r84197.
Rolled back due to DCHECK in official builds, r84373.
Will re-submit with fix.

BUG=81750
TEST=base_unittests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=84801

[Jói, 2011-04-20 22:57:04]

Jim, I will wait for your LGTM before asking someone from base/OWNERS to rubber
stamp.

Cheers,
Jói

[jar (doing other things), 2011-04-21 01:03:50]

Thanks for cleaning up a number of typos an nits already in these files!

I have a bunch of general comments.  If you think I'm off base, perhaps we can
chat on the phone a bit.

http://codereview.chromium.org/6883102/diff/1/base/file_util_win.cc
File base/file_util_win.cc (right):

http://codereview.chromium.org/6883102/diff/1/base/file_util_win.cc#newcode561
base/file_util_win.cc:561: FilePath GetHomeDir() {
I don't think this is a good source of entropy.  ...but as per my comments where
it is used... I'm not sure you need this machine specific entropy.

http://codereview.chromium.org/6883102/diff/1/base/metrics/field_trial.cc
File base/metrics/field_trial.cc (right):

http://codereview.chromium.org/6883102/diff/1/base/metrics/field_trial.cc#new...
base/metrics/field_trial.cc:71: }
style nit: Don't bother with braces on one line if statements (unless other
clause has braces) in this file.

http://codereview.chromium.org/6883102/diff/1/base/metrics/field_trial.cc#new...
base/metrics/field_trial.cc:83: divisor_ *
MachineIdToUniformDouble(machine_id));
Is your hope to make these repeatable on a given machine?  This doesn't seem
that critical or useful.  In fact, it makes it hard for a caller to get
different values from other users.

If you really want to go this route, you should accept an argument "salt" rather
than machine_id, and that salt should be added to a machine specific string as
the basis of the consistent randomization.  That would at least allow different
experiments to have different selections from their cases.  (i.e., when the
random number comes out near 0.001, we don't always choose the first trial
suggested in all field trials).


It seems much nicer to not depend on the caller for any (explicit) entropy.  If
you do want consistent randomness, you could depend on the field_trial_name_ as
your salt, and combine it with a persistent unique value.  That would simplify
the API, and still get you the proposed perpetual(?) consistency.

With the current interface, what guarantee do you have that the machine_id will
persist?

We generate a random value in the metrics system when we do an UMA upload to tag
the client.  We are very cautious NOT to generate a unique tag unless the user
opts-in to upload, as folks concerned with privacy did not want any tags on
their machine.  The interesting thing is that you're trying to define a tag
(i.e., persistent machine_id), and that may fly in the face of users that DON'T
want to be tagged.

My suggestion is to accelerate the tagging when UMA is enabled, so that you can
call it now and get that id as your major source of entropy.  You should
probably be able to detect that it is not yet set, and force your experiment
into a disabled mode (until the day that the UMA ID is available... and then
you'd start to use it).

http://codereview.chromium.org/6883102/diff/1/base/metrics/field_trial.cc#new...
base/metrics/field_trial.cc:161: file_util::GetHomeDir().WriteToPickle(&pickle);
Pickling is a waste.  It doesn't add to the entropy, and just takes time and
memory.  

I'd also suggest that you rename this function HashStringToDouble()

it is really a hash function.

The fact that the string is (or might be) a machine id seems inconsequential, so
the argument probably shouldn't be named machine_id.

http://codereview.chromium.org/6883102/diff/1/base/metrics/field_trial.h
File base/metrics/field_trial.h (right):

http://codereview.chromium.org/6883102/diff/1/base/metrics/field_trial.h#newc...
base/metrics/field_trial.h:130: void UseOneTimeRandomization(const std::string&
machine_id);
I'm not convinced you need an argument here, and clearly it puts a large burden
on the caller.  Can't we do without??

I'd rather see an interface that emulates persisted coin toss, with one toss per
persistent trial, than to require all coin-tosses on a single machine to always
come out the same way.

http://codereview.chromium.org/6883102/diff/1/base/metrics/field_trial.h#newc...
base/metrics/field_trial.h:134: void Disable();
Traditionally, disabling meant we were not part of the field trial.  That state
is quite distinct from the default case, although we do eventually turn into a
disabled case when our time expires. 

The subtle distinction is visible in MakeName() and the pattern by which that
function is used in histograms.  When we are disabled, the user often has
over-ridden the target state, and hence the user can't be included in a
would-be-random population.

How will this fit in with your planned use case?

http://codereview.chromium.org/6883102/diff/1/base/metrics/field_trial.h#newc...
base/metrics/field_trial.h:230: bool disable_field_trial_;
My apology for not noticing this in previous reviews (for other author's
checkins):

Flags are most readable when they are positive (so that you don't have to deal
with double negatives in your head).  This should probably be:

enable_field_trial_

It is fine to have a call to Disable()... just this flag name should be a
positive name.

[You can probably tell I didn't define NDEBUG for the C standard!  ;-)  ]

http://codereview.chromium.org/6883102/diff/1/base/metrics/field_trial_unitte...
File base/metrics/field_trial_unittest.cc (right):

http://codereview.chromium.org/6883102/diff/1/base/metrics/field_trial_unitte...
base/metrics/field_trial_unittest.cc:333: EXPECT_GT(kExpectedMax, average);
Flaky tests are not acceptable, even with an explanation message.
You *can* re-run the test until you pass (by some definition), but you can't
have an Expect() that can fail with any detectable probability.  

The common trick is to "run it again" if it fails... and again... and again.  If
it can't pass in about 5 minutes, the test harness will finally kill you with a
failed-to-complete.

Looking at the test, even if this was very non random, it would "eventually"
pass.

Maybe it would be better to just put results into some array of possible ranges,
and run until you have data in all N range buckets??

http://codereview.chromium.org/6883102/diff/1/base/metrics/field_trial_unitte...
base/metrics/field_trial_unittest.cc:348: new FieldTrial("first", 100,
"default", 2050, 1, 1),
use (... next_year_, 12, 31) for the date.  See line 294 as an example.

http://codereview.chromium.org/6883102/diff/1/base/rand_util.cc
File base/rand_util.cc (right):

http://codereview.chromium.org/6883102/diff/1/base/rand_util.cc#newcode33
base/rand_util.cc:33: // is expected to accommodate 53 bits.
Why don't you just do a conversion to float (which will take as many bits as it
can!), and then divide by 2^64?

The only risk you' would then have is that the result *could* == 1.0, but that
has probability around 1 in 2^54.... so you can then map that outline *anywhere*
constant and not (significantly) impact the randomness (for any tests we can
do!!).

http://codereview.chromium.org/6883102/diff/1/base/rand_util.h
File base/rand_util.h (right):

http://codereview.chromium.org/6883102/diff/1/base/rand_util.h#newcode32
base/rand_util.h:32: BASE_API double BitsToRandDouble(uint64 bits);
This doesn't create a random number, but only converts bits to a floating point
number.  Hence its name probably shouldn't include Rand.
I think this should just go in your file, and is not generally useful [it is
very rare to have to add code to base].

[Jói, 2011-04-21 02:16:47]

Thanks Jim.

To summarize the conclusion of our discussion on the phone:  Using the
UMA ID is clearly better for a few reasons, including making the code
a lot simpler; I wasn't aware of the UMA ID pref, and I will switch.

All of the following will go away with that change, but just to explain them:
- The BitsToRandDouble thing wasn't not new code, it was extracted
from RandDouble. The naming, I agree, was bad :)
- The Pickle thing was just to use tested code to concatenate a
std::string with a FilePath (which has different string types
depending on platform), because the SHA1 interface needs one solid
block of bytes (you can't e.g. SHA1Add like you can with MD5).
- Thanks for mentioning the run again trick for the unit test.  It
would already fail with something like astronomical unlikelihood, but
that's a pattern I hadn't seen before and it's useful.

Cheers,
Jói


On Wed, Apr 20, 2011 at 9:03 PM,  <jar@chromium.org> wrote:
> Thanks for cleaning up a number of typos an nits already in these files!
>
> I have a bunch of general comments.  If you think I'm off base, perhaps we
> can
> chat on the phone a bit.
>
>
> http://codereview.chromium.org/6883102/diff/1/base/file_util_win.cc
> File base/file_util_win.cc (right):
>
> http://codereview.chromium.org/6883102/diff/1/base/file_util_win.cc#newcode561
> base/file_util_win.cc:561: FilePath GetHomeDir() {
> I don't think this is a good source of entropy.  ...but as per my
> comments where it is used... I'm not sure you need this machine specific
> entropy.
>
> http://codereview.chromium.org/6883102/diff/1/base/metrics/field_trial.cc
> File base/metrics/field_trial.cc (right):
>
>
http://codereview.chromium.org/6883102/diff/1/base/metrics/field_trial.cc#new...
> base/metrics/field_trial.cc:71: }
> style nit: Don't bother with braces on one line if statements (unless
> other clause has braces) in this file.
>
>
http://codereview.chromium.org/6883102/diff/1/base/metrics/field_trial.cc#new...
> base/metrics/field_trial.cc:83: divisor_ *
> MachineIdToUniformDouble(machine_id));
> Is your hope to make these repeatable on a given machine?  This doesn't
> seem that critical or useful.  In fact, it makes it hard for a caller to
> get different values from other users.
>
> If you really want to go this route, you should accept an argument
> "salt" rather than machine_id, and that salt should be added to a
> machine specific string as the basis of the consistent randomization.
> That would at least allow different experiments to have different
> selections from their cases.  (i.e., when the random number comes out
> near 0.001, we don't always choose the first trial suggested in all
> field trials).
>
>
> It seems much nicer to not depend on the caller for any (explicit)
> entropy.  If you do want consistent randomness, you could depend on the
> field_trial_name_ as your salt, and combine it with a persistent unique
> value.  That would simplify the API, and still get you the proposed
> perpetual(?) consistency.
>
> With the current interface, what guarantee do you have that the
> machine_id will persist?
>
> We generate a random value in the metrics system when we do an UMA
> upload to tag the client.  We are very cautious NOT to generate a unique
> tag unless the user opts-in to upload, as folks concerned with privacy
> did not want any tags on their machine.  The interesting thing is that
> you're trying to define a tag (i.e., persistent machine_id), and that
> may fly in the face of users that DON'T want to be tagged.
>
> My suggestion is to accelerate the tagging when UMA is enabled, so that
> you can call it now and get that id as your major source of entropy.
> You should probably be able to detect that it is not yet set, and force
> your experiment into a disabled mode (until the day that the UMA ID is
> available... and then you'd start to use it).
>
>
http://codereview.chromium.org/6883102/diff/1/base/metrics/field_trial.cc#new...
> base/metrics/field_trial.cc:161:
> file_util::GetHomeDir().WriteToPickle(&pickle);
> Pickling is a waste.  It doesn't add to the entropy, and just takes time
> and memory.
>
> I'd also suggest that you rename this function HashStringToDouble()
>
> it is really a hash function.
>
> The fact that the string is (or might be) a machine id seems
> inconsequential, so the argument probably shouldn't be named machine_id.
>
> http://codereview.chromium.org/6883102/diff/1/base/metrics/field_trial.h
> File base/metrics/field_trial.h (right):
>
>
http://codereview.chromium.org/6883102/diff/1/base/metrics/field_trial.h#newc...
> base/metrics/field_trial.h:130: void UseOneTimeRandomization(const
> std::string& machine_id);
> I'm not convinced you need an argument here, and clearly it puts a large
> burden on the caller.  Can't we do without??
>
> I'd rather see an interface that emulates persisted coin toss, with one
> toss per persistent trial, than to require all coin-tosses on a single
> machine to always come out the same way.
>
>
http://codereview.chromium.org/6883102/diff/1/base/metrics/field_trial.h#newc...
> base/metrics/field_trial.h:134: void Disable();
> Traditionally, disabling meant we were not part of the field trial.
> That state is quite distinct from the default case, although we do
> eventually turn into a disabled case when our time expires.
>
> The subtle distinction is visible in MakeName() and the pattern by which
> that function is used in histograms.  When we are disabled, the user
> often has over-ridden the target state, and hence the user can't be
> included in a would-be-random population.
>
> How will this fit in with your planned use case?
>
>
http://codereview.chromium.org/6883102/diff/1/base/metrics/field_trial.h#newc...
> base/metrics/field_trial.h:230: bool disable_field_trial_;
> My apology for not noticing this in previous reviews (for other author's
> checkins):
>
> Flags are most readable when they are positive (so that you don't have
> to deal with double negatives in your head).  This should probably be:
>
> enable_field_trial_
>
> It is fine to have a call to Disable()... just this flag name should be
> a positive name.
>
> [You can probably tell I didn't define NDEBUG for the C standard!  ;-)
> ]
>
>
http://codereview.chromium.org/6883102/diff/1/base/metrics/field_trial_unitte...
> File base/metrics/field_trial_unittest.cc (right):
>
>
http://codereview.chromium.org/6883102/diff/1/base/metrics/field_trial_unitte...
> base/metrics/field_trial_unittest.cc:333: EXPECT_GT(kExpectedMax,
> average);
> Flaky tests are not acceptable, even with an explanation message.
> You *can* re-run the test until you pass (by some definition), but you
> can't have an Expect() that can fail with any detectable probability.
>
> The common trick is to "run it again" if it fails... and again... and
> again.  If it can't pass in about 5 minutes, the test harness will
> finally kill you with a failed-to-complete.
>
> Looking at the test, even if this was very non random, it would
> "eventually" pass.
>
> Maybe it would be better to just put results into some array of possible
> ranges, and run until you have data in all N range buckets??
>
>
http://codereview.chromium.org/6883102/diff/1/base/metrics/field_trial_unitte...
> base/metrics/field_trial_unittest.cc:348: new FieldTrial("first", 100,
> "default", 2050, 1, 1),
> use (... next_year_, 12, 31) for the date.  See line 294 as an example.
>
> http://codereview.chromium.org/6883102/diff/1/base/rand_util.cc
> File base/rand_util.cc (right):
>
> http://codereview.chromium.org/6883102/diff/1/base/rand_util.cc#newcode33
> base/rand_util.cc:33: // is expected to accommodate 53 bits.
> Why don't you just do a conversion to float (which will take as many
> bits as it can!), and then divide by 2^64?
>
> The only risk you' would then have is that the result *could* == 1.0,
> but that has probability around 1 in 2^54.... so you can then map that
> outline *anywhere* constant and not (significantly) impact the
> randomness (for any tests we can do!!).
>
> http://codereview.chromium.org/6883102/diff/1/base/rand_util.h
> File base/rand_util.h (right):
>
> http://codereview.chromium.org/6883102/diff/1/base/rand_util.h#newcode32
> base/rand_util.h:32: BASE_API double BitsToRandDouble(uint64 bits);
> This doesn't create a random number, but only converts bits to a
> floating point number.  Hence its name probably shouldn't include Rand.
> I think this should just go in your file, and is not generally useful
> [it is very rare to have to add code to base].
>
> http://codereview.chromium.org/6883102/
>

[Jói, 2011-04-21 19:50:33]

Hi Jim,

Please take another look.  I've switched to using the UMA ID, added
initialization at the appropriate place in browser_main.cc, and added unit tests
for Cancel() (and fixed it).

Note the changes I made to documentation about how to find if you're
participating in a trial, specifically removal of the "group name will be empty"
language and changing how to determine whether to output the trial-specific UMA
metric, since having an empty group_name() does not seem possible any more (even
before my change).  Please verify these changes, maybe I'm misunderstanding
something.

Cheers,
Jói

http://codereview.chromium.org/6883102/diff/1/base/file_util_win.cc
File base/file_util_win.cc (right):

http://codereview.chromium.org/6883102/diff/1/base/file_util_win.cc#newcode561
base/file_util_win.cc:561: FilePath GetHomeDir() {
 2011/04/21 01:03:50, jar wrote:
> I don't think this is a good source of entropy.  ...but as per my comments
where
> it is used... I'm not sure you need this machine specific entropy.

Switched to UMA ID.

http://codereview.chromium.org/6883102/diff/1/base/metrics/field_trial.cc
File base/metrics/field_trial.cc (right):

http://codereview.chromium.org/6883102/diff/1/base/metrics/field_trial.cc#new...
base/metrics/field_trial.cc:71: }
On 2011/04/21 01:03:50, jar wrote:
> style nit: Don't bother with braces on one line if statements (unless other
> clause has braces) in this file.

Done.

http://codereview.chromium.org/6883102/diff/1/base/metrics/field_trial.cc#new...
base/metrics/field_trial.cc:83: divisor_ *
MachineIdToUniformDouble(machine_id));
On 2011/04/21 01:03:50, jar wrote:
> Is your hope to make these repeatable on a given machine?  This doesn't seem
> that critical or useful.  In fact, it makes it hard for a caller to get
> different values from other users.
> 
> If you really want to go this route, you should accept an argument "salt"
rather
> than machine_id, and that salt should be added to a machine specific string as
> the basis of the consistent randomization.  That would at least allow
different
> experiments to have different selections from their cases.  (i.e., when the
> random number comes out near 0.001, we don't always choose the first trial
> suggested in all field trials).
> 
> 
> It seems much nicer to not depend on the caller for any (explicit) entropy. 
If
> you do want consistent randomness, you could depend on the field_trial_name_
as
> your salt, and combine it with a persistent unique value.  That would simplify
> the API, and still get you the proposed perpetual(?) consistency.
> 
> With the current interface, what guarantee do you have that the machine_id
will
> persist?
> 
> We generate a random value in the metrics system when we do an UMA upload to
tag
> the client.  We are very cautious NOT to generate a unique tag unless the user
> opts-in to upload, as folks concerned with privacy did not want any tags on
> their machine.  The interesting thing is that you're trying to define a tag
> (i.e., persistent machine_id), and that may fly in the face of users that
DON'T
> want to be tagged.
> 
> My suggestion is to accelerate the tagging when UMA is enabled, so that you
can
> call it now and get that id as your major source of entropy.  You should
> probably be able to detect that it is not yet set, and force your experiment
> into a disabled mode (until the day that the UMA ID is available... and then
> you'd start to use it).

I'm now using the UMA ID and salting with the trial name.

http://codereview.chromium.org/6883102/diff/1/base/metrics/field_trial.cc#new...
base/metrics/field_trial.cc:161: file_util::GetHomeDir().WriteToPickle(&pickle);
On 2011/04/21 01:03:50, jar wrote:
> Pickling is a waste.  It doesn't add to the entropy, and just takes time and
> memory.  
> 
> I'd also suggest that you rename this function HashStringToDouble()
> 
> it is really a hash function.
> 
> The fact that the string is (or might be) a machine id seems inconsequential,
so
> the argument probably shouldn't be named machine_id.

Done.

http://codereview.chromium.org/6883102/diff/1/base/metrics/field_trial.h
File base/metrics/field_trial.h (right):

http://codereview.chromium.org/6883102/diff/1/base/metrics/field_trial.h#newc...
base/metrics/field_trial.h:130: void UseOneTimeRandomization(const std::string&
machine_id);
On 2011/04/21 01:03:50, jar wrote:
> I'm not convinced you need an argument here, and clearly it puts a large
burden
> on the caller.  Can't we do without??
> 
> I'd rather see an interface that emulates persisted coin toss, with one toss
per
> persistent trial, than to require all coin-tosses on a single machine to
always
> come out the same way.  

Done.

http://codereview.chromium.org/6883102/diff/1/base/metrics/field_trial.h#newc...
base/metrics/field_trial.h:134: void Disable();
On 2011/04/21 01:03:50, jar wrote:
> Traditionally, disabling meant we were not part of the field trial.  That
state
> is quite distinct from the default case, although we do eventually turn into a
> disabled case when our time expires. 
> 
> The subtle distinction is visible in MakeName() and the pattern by which that
> function is used in histograms.  When we are disabled, the user often has
> over-ridden the target state, and hence the user can't be included in a
> would-be-random population.
> 
> How will this fit in with your planned use case?

So I actually took a closer look at this logic.  It seems that group_name() can
never return the empty string, neither before nor after my change, as you are
always in one of the groups of a trial if the trial is registered.  I updated
the documentation to reflect this, including at the top where it shows how to
determine whether to produce a separate histogram name.  Please verify my
findings as you obviously know this code a lot better than I do.

http://codereview.chromium.org/6883102/diff/1/base/metrics/field_trial.h#newc...
base/metrics/field_trial.h:230: bool disable_field_trial_;
On 2011/04/21 01:03:50, jar wrote:
> My apology for not noticing this in previous reviews (for other author's
> checkins):
> 
> Flags are most readable when they are positive (so that you don't have to deal
> with double negatives in your head).  This should probably be:
> 
> enable_field_trial_
> 
> It is fine to have a call to Disable()... just this flag name should be a
> positive name.
> 
> [You can probably tell I didn't define NDEBUG for the C standard!  ;-)  ]

Fixed this, and also fixed handling of Disable() when it is called after
initialization (which will be the case in my expected usage).

http://codereview.chromium.org/6883102/diff/1/base/metrics/field_trial_unitte...
File base/metrics/field_trial_unittest.cc (right):

http://codereview.chromium.org/6883102/diff/1/base/metrics/field_trial_unitte...
base/metrics/field_trial_unittest.cc:333: EXPECT_GT(kExpectedMax, average);
On 2011/04/21 01:03:50, jar wrote:
> Flaky tests are not acceptable, even with an explanation message.
> You *can* re-run the test until you pass (by some definition), but you can't
> have an Expect() that can fail with any detectable probability.  
> 
> The common trick is to "run it again" if it fails... and again... and again. 
If
> it can't pass in about 5 minutes, the test harness will finally kill you with
a
> failed-to-complete.
> 
> Looking at the test, even if this was very non random, it would "eventually"
> pass.
> 
> Maybe it would be better to just put results into some array of possible
ranges,
> and run until you have data in all N range buckets??

Fixed.

http://codereview.chromium.org/6883102/diff/1/base/metrics/field_trial_unitte...
base/metrics/field_trial_unittest.cc:348: new FieldTrial("first", 100,
"default", 2050, 1, 1),
On 2011/04/21 01:03:50, jar wrote:
> use (... next_year_, 12, 31) for the date.  See line 294 as an example.

Done.

http://codereview.chromium.org/6883102/diff/1/base/rand_util.cc
File base/rand_util.cc (right):

http://codereview.chromium.org/6883102/diff/1/base/rand_util.cc#newcode33
base/rand_util.cc:33: // is expected to accommodate 53 bits.
On 2011/04/21 01:03:50, jar wrote:
> Why don't you just do a conversion to float (which will take as many bits as
it
> can!), and then divide by 2^64?
> 
> The only risk you' would then have is that the result *could* == 1.0, but that
> has probability around 1 in 2^54.... so you can then map that outline
*anywhere*
> constant and not (significantly) impact the randomness (for any tests we can
> do!!).

As discussed, this is just extracting most of the existing body of RandDouble()
so that its implementation of converting 8 bytes to a random float in the range
[0.0, 1.0) can be reused.

http://codereview.chromium.org/6883102/diff/1/base/rand_util.h
File base/rand_util.h (right):

http://codereview.chromium.org/6883102/diff/1/base/rand_util.h#newcode32
base/rand_util.h:32: BASE_API double BitsToRandDouble(uint64 bits);
On 2011/04/21 01:03:50, jar wrote:
> This doesn't create a random number, but only converts bits to a floating
point
> number.  Hence its name probably shouldn't include Rand.
> I think this should just go in your file, and is not generally useful [it is
> very rare to have to add code to base].

Changed to BitsToRandLikeDouble. Still not a great name but beats
BitsToDoubleBetweenZeroAndOne (which still wouldn't be 100% accurate).

As we discussed on the phone, this is not adding code to base, this is
extracting most of the implementation of RandDouble() into a function that can
be reused.  An argument could be made to say this would belong in something like
base/math_util.h but I don't really think that's worth it for now.

[Jói, 2011-04-21 20:22:59]

http://codereview.chromium.org/6883102/diff/3025/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/6883102/diff/3025/chrome/browser/browser_main....
chrome/browser/browser_main.cc:1332: // browser::RegisterLocalState() above.
Whoops, my bad, that's not where the state actually gets created.  Ignore this
file for now, I'll upload an update once I've done this properly.

[Jói, 2011-04-21 20:37:08]

Jim, quick question:

It sort of looks like I could move InitializeMetricsService up to before where I
call FieldTrialList::EnableOneTimeRandomization(), but I'm sort of scared of
that change.

The other approach is to determine whether the client ID will be required, and
initialize it early if it will be, but leave the actual starting of the metrics
server where it is today.  I'm leaning towards doing this, as a less risky
approach, but wanted your opinion first.

Cheers,
Jói



On 2011/04/21 20:22:59, Jói wrote:
>
http://codereview.chromium.org/6883102/diff/3025/chrome/browser/browser_main.cc
> File chrome/browser/browser_main.cc (right):
> 
>
http://codereview.chromium.org/6883102/diff/3025/chrome/browser/browser_main....
> chrome/browser/browser_main.cc:1332: // browser::RegisterLocalState() above.
> Whoops, my bad, that's not where the state actually gets created.  Ignore this
> file for now, I'll upload an update once I've done this properly.

[jar (doing other things), 2011-04-21 22:10:02]

http://codereview.chromium.org/6883102/diff/3025/base/metrics/field_trial.cc
File base/metrics/field_trial.cc (right):

http://codereview.chromium.org/6883102/diff/3025/base/metrics/field_trial.cc#...
base/metrics/field_trial.cc:338: return EmptyString();
If you switch over to using a detectable uninitialized value for client_id, you
could return it here rather than empty string.

http://codereview.chromium.org/6883102/diff/3025/base/metrics/field_trial.h
File base/metrics/field_trial.h (left):

http://codereview.chromium.org/6883102/diff/3025/base/metrics/field_trial.h#o...
base/metrics/field_trial.h:57: //    
!base::FieldTrialList::Find("MemoryExperiment")->group_name().empty());
Removal of this code is a VERY good improvement.  The class has evolved, and I
think you are totally correct, that the name can no longer be empty (since the
commitment is made when we query the state... and worst case, it becomes the
default string... which is required to be non-empty).

There are a number of places where the above pattern is currently used, and
should be simplified to match your correction.  Search for:
FieldTrialList::Find

to get a list.

If you don't want to do this, please file a bug against me and I'll do the
cleanup.

http://codereview.chromium.org/6883102/diff/3025/base/metrics/field_trial.h
File base/metrics/field_trial.h (right):

http://codereview.chromium.org/6883102/diff/3025/base/metrics/field_trial.h#n...
base/metrics/field_trial.h:194: const std::string& trial_name);
I would have expected the client_id to be a constant, that is held in a class
static (accessible via a public static method in the singleton for
FieldTrialList).  I'd then expect that the trial_name could be extracted from
the FieldTrial instance, which could be done by making this a non-static method.
 

Is there a good reason for this to be a static method?

http://codereview.chromium.org/6883102/diff/3025/base/metrics/field_trial.h#n...
base/metrics/field_trial.h:197: // This is empty if the trial is not in the
experiment.
hmmm.... is this ever empty now?

http://codereview.chromium.org/6883102/diff/3025/base/metrics/field_trial.h#n...
base/metrics/field_trial.h:227: bool enable_field_trial_;
Thanks for making this change!

http://codereview.chromium.org/6883102/diff/3025/base/metrics/field_trial.h#n...
base/metrics/field_trial.h:298: static void EnableOneTimeRandomization(const
std::string& client_id);
I'd be tempted to make this part of required initialization, passed into the
constructor on line 248, and then have assertions rather than the test at line
302.

The only concern is the lack of a value for the persistent id.  I'd suggest that
there be a constant, defined in metrics_services, which is
kUNINITALIZED_CLIENT_ID (or something of that sort).  The constructor can then
detect this non-initialized state rather directly.

http://codereview.chromium.org/6883102/diff/3025/base/metrics/field_trial_uni...
File base/metrics/field_trial_unittest.cc (right):

http://codereview.chromium.org/6883102/diff/3025/base/metrics/field_trial_uni...
base/metrics/field_trial_unittest.cc:323: while (!success && timer.Elapsed() <
TimeDelta::FromSeconds(20)) {
You probably don't need to (and shouldn't) stop in 20 seconds.  The philosophy
is "design tests to complete quickly when everything is fine," which you're
doing, but there is no reason to "rush" when there is a problem.  The test
harness will (in the bustage case) eventually terminate us.

IF the VM hangs for a 20 second period, then this test can sadly flake.  This
class of flake due to extended "murphy-thread" pauses is more common in
multi-threaded tests, but in general, we shouldn't depend on a time (inside a
test) like this.

I *really* don't think this test is ever going to fail... but it causes a lot of
havoc if/when it does... so its best to avoid a failure until there is a real
failure (machine is hung forever??).

http://codereview.chromium.org/6883102/diff/3025/base/metrics/field_trial_uni...
base/metrics/field_trial_unittest.cc:399: ASSERT_EQ("default",
trial->group_name());
Good tests. ;-)

http://codereview.chromium.org/6883102/diff/3025/base/rand_util.cc
File base/rand_util.cc (right):

http://codereview.chromium.org/6883102/diff/3025/base/rand_util.cc#newcode29
base/rand_util.cc:29: double BitsToRandLikeDouble(uint64 bits) {
Why not just BitsToDouble()?  I really don't like Rand in there.  Perhaps
BitsToSmallDouble()?  Does that help with your desire to state that there is a
"limited output range," and not merely something like a reintepret_cast?

http://codereview.chromium.org/6883102/diff/3025/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/6883102/diff/3025/chrome/browser/browser_main....
chrome/browser/browser_main.cc:1332: // browser::RegisterLocalState() above.
On 2011/04/21 20:23:00, Jói wrote:
> Whoops, my bad, that's not where the state actually gets created.  Ignore this
> file for now, I'll upload an update once I've done this properly.

Perhaps the above comment means this... but to reitterate: There is (or should
be) a public call in metrics_services that will get or create an ID, if UMA is
enabled.  It is better to use that, and not export knowledge about where the
pref is stored (i.e., you're violating encapsulation by exporting infomation
like kMetricsClientID and pulling this directly from the prefs system... and
you're not allowing for a lazy construction, which means you won't run such
tests in the first run of a browser).

Also, be sure you call said metrics functions after we've initialized that
module, which will include decoding of whether UMA is enabled or not.  I think
that module is initialized as of line 1683 below.

If you want to get the "head start" and aren't worried about "first run"
execution of your field trial, then you should at least add a method to metrics
services which pulls the value out of a pref.  This way only the
metrics_services will be aware of the name of the stored pref (and it will be
easier to change said code in metrics services as we evolve).

[Paweł Hajdan Jr., 2011-04-26 10:23:10]

Drive-by with a test-related comment. No need to wait for me if you just use
TestTimeouts or remove the timeout completely.

http://codereview.chromium.org/6883102/diff/3025/base/metrics/field_trial_uni...
File base/metrics/field_trial_unittest.cc (right):

http://codereview.chromium.org/6883102/diff/3025/base/metrics/field_trial_uni...
base/metrics/field_trial_unittest.cc:323: while (!success && timer.Elapsed() <
TimeDelta::FromSeconds(20)) {
Just in case you decide to keep a timeout here, please use one from
base/test/test_timeouts. Hardcoding timeouts is *always* a bad thing. Simple
example: on Valgrind things will be an order of magnitude slower, and
base/test/test_timeouts are adjusted for Valgrind in a way transparent for test
writers.

[Jói, 2011-04-28 01:03:50]

Hi Jim,

I'm finally back from the 4-day weekend here in Canada, and from dealing with
stuff that piled up in the meantime. Please take another look.

Paweł, the timeout is gone from the test you commented on.

Cheers,
Jói

http://codereview.chromium.org/6883102/diff/3025/base/metrics/field_trial.h
File base/metrics/field_trial.h (left):

http://codereview.chromium.org/6883102/diff/3025/base/metrics/field_trial.h#o...
base/metrics/field_trial.h:57: //    
!base::FieldTrialList::Find("MemoryExperiment")->group_name().empty());
On 2011/04/21 22:10:02, jar wrote:
> Removal of this code is a VERY good improvement.  The class has evolved, and I
> think you are totally correct, that the name can no longer be empty (since the
> commitment is made when we query the state... and worst case, it becomes the
> default string... which is required to be non-empty).
> 
> There are a number of places where the above pattern is currently used, and
> should be simplified to match your correction.  Search for:
> FieldTrialList::Find
> 
> to get a list.
> 
> If you don't want to do this, please file a bug against me and I'll do the
> cleanup.

I've updated all the call-sites now.  I introduced a new static function
TrialExists(name) that is equivalent to !!Find(name) to avoid having to coerce
to boolean at every call-site.  I also updated the docs to suggest using a
static const bool to store the results of the find, as that seems like a good
idea in most cases.

http://codereview.chromium.org/6883102/diff/3025/base/metrics/field_trial.h
File base/metrics/field_trial.h (right):

http://codereview.chromium.org/6883102/diff/3025/base/metrics/field_trial.h#n...
base/metrics/field_trial.h:194: const std::string& trial_name);
 2011/04/21 22:10:02, jar wrote:
> I would have expected the client_id to be a constant, that is held in a class
> static (accessible via a public static method in the singleton for
> FieldTrialList).  I'd then expect that the trial_name could be extracted from
> the FieldTrial instance, which could be done by making this a non-static
method.
>  
> 
> Is there a good reason for this to be a static method?

The main reason I chose this interface is that it's a lot lighter-weight to
test, no need to set up various FieldTrialTest instances.  It's a private
implementation detail and it should be just as performant as if it were a
method, so in my opinion it doesn't need to be changed.

http://codereview.chromium.org/6883102/diff/3025/base/metrics/field_trial.h#n...
base/metrics/field_trial.h:197: // This is empty if the trial is not in the
experiment.
On 2011/04/21 22:10:02, jar wrote:
> hmmm.... is this ever empty now?

You're right, I updated the documentation and added a DCHECK in the constructor
to ensure the name is never empty.

http://codereview.chromium.org/6883102/diff/3025/base/metrics/field_trial.h#n...
base/metrics/field_trial.h:227: bool enable_field_trial_;
On 2011/04/21 22:10:02, jar wrote:
> Thanks for making this change!

No prob.

http://codereview.chromium.org/6883102/diff/3025/base/metrics/field_trial.h#n...
base/metrics/field_trial.h:298: static void EnableOneTimeRandomization(const
std::string& client_id);
On 2011/04/21 22:10:02, jar wrote:
> I'd be tempted to make this part of required initialization, passed into the
> constructor on line 248, and then have assertions rather than the test at line
> 302.
> 
> The only concern is the lack of a value for the persistent id.  I'd suggest
that
> there be a constant, defined in metrics_services, which is
> kUNINITALIZED_CLIENT_ID (or something of that sort).  The constructor can then
> detect this non-initialized state rather directly.

I disagree, but am willing to be overridden.  Let me explain why I disgaree:

I think it's nice to be able to use FieldTrial / FieldTrialList without
necessarily having a one-time random ID for your client.  It's also more
flexible to be able to defer initializing the FieldTrialList with this until you
actually need it (right before you create any one-time randomized trials) rather
than having to do it at construction time.

For the ability of using the FieldTrialList without providing a one-time ID, if
you were suggesting allowing users to specify "not initialized" in the
constructor, I think it makes for a cleaner public interface not to have to
declare a "not initialized" value in the header file and document on the
constructor that callers need to use it - cleaner to keep that detail
implementation-private by using a separate initialization function, and safer
since callers might not read the documentation and might specify e.g. some
static string as the one-time secret, which would destroy our ability to DCHECK
on initialization before they request a one-time randomized trial.

I would suggest keeping the interface as-is, but as I said, I am willing to be
overridden.

http://codereview.chromium.org/6883102/diff/3025/base/metrics/field_trial_uni...
File base/metrics/field_trial_unittest.cc (right):

http://codereview.chromium.org/6883102/diff/3025/base/metrics/field_trial_uni...
base/metrics/field_trial_unittest.cc:323: while (!success && timer.Elapsed() <
TimeDelta::FromSeconds(20)) {
On 2011/04/26 10:23:10, Paweł Hajdan Jr. wrote:
> Just in case you decide to keep a timeout here, please use one from
> base/test/test_timeouts. Hardcoding timeouts is *always* a bad thing. Simple
> example: on Valgrind things will be an order of magnitude slower, and
> base/test/test_timeouts are adjusted for Valgrind in a way transparent for
test
> writers.

Thanks, I'm removing the timeout altogether.

http://codereview.chromium.org/6883102/diff/3025/base/metrics/field_trial_uni...
base/metrics/field_trial_unittest.cc:323: while (!success && timer.Elapsed() <
TimeDelta::FromSeconds(20)) {
On 2011/04/21 22:10:02, jar wrote:
> You probably don't need to (and shouldn't) stop in 20 seconds.  The philosophy
> is "design tests to complete quickly when everything is fine," which you're
> doing, but there is no reason to "rush" when there is a problem.  The test
> harness will (in the bustage case) eventually terminate us.
> 
> IF the VM hangs for a 20 second period, then this test can sadly flake.  This
> class of flake due to extended "murphy-thread" pauses is more common in
> multi-threaded tests, but in general, we shouldn't depend on a time (inside a
> test) like this.
> 
> I *really* don't think this test is ever going to fail... but it causes a lot
of
> havoc if/when it does... so its best to avoid a failure until there is a real
> failure (machine is hung forever??).

The way the test was written (with or without the suggested timeout), a
non-uniform distribution could still make the test pass given enough tries. 
I've changed it so that if it isn't within bounds after the first 300 samples
have been averaged, it adds another 300, and so on.  The expected value (in this
case 0.5) is the average of samples from the number generator as the number of
samples approach infinity, so this should make the test correct; if it converges
to something within the 0.48-0.52 range, it is correct; if it doesn't, the test
can actually catch that now.

http://codereview.chromium.org/6883102/diff/3025/base/metrics/field_trial_uni...
base/metrics/field_trial_unittest.cc:399: ASSERT_EQ("default",
trial->group_name());
On 2011/04/21 22:10:02, jar wrote:
> Good tests. ;-)

Thanks :)

http://codereview.chromium.org/6883102/diff/3025/base/rand_util.cc
File base/rand_util.cc (right):

http://codereview.chromium.org/6883102/diff/3025/base/rand_util.cc#newcode29
base/rand_util.cc:29: double BitsToRandLikeDouble(uint64 bits) {
On 2011/04/21 22:10:02, jar wrote:
> Why not just BitsToDouble()?  I really don't like Rand in there.  Perhaps
> BitsToSmallDouble()?  Does that help with your desire to state that there is a
> "limited output range," and not merely something like a reintepret_cast?

This is technically one of the half-open unit intervals (the other is (0.0,
1.0]).  I am now calling it BitsToOpenEndedUnitInterval which I think captures
fairly nicely that it's the [0.0, 1.0) half-open unit interval.

http://codereview.chromium.org/6883102/diff/3025/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/6883102/diff/3025/chrome/browser/browser_main....
chrome/browser/browser_main.cc:1332: // browser::RegisterLocalState() above.
On 2011/04/21 22:10:02, jar wrote:
> On 2011/04/21 20:23:00, Jói wrote:
> > Whoops, my bad, that's not where the state actually gets created.  Ignore
this
> > file for now, I'll upload an update once I've done this properly.
> 
> Perhaps the above comment means this... but to reitterate: There is (or should
> be) a public call in metrics_services that will get or create an ID, if UMA is
> enabled.  It is better to use that, and not export knowledge about where the
> pref is stored (i.e., you're violating encapsulation by exporting infomation
> like kMetricsClientID and pulling this directly from the prefs system... and
> you're not allowing for a lazy construction, which means you won't run such
> tests in the first run of a browser).
> 
> Also, be sure you call said metrics functions after we've initialized that
> module, which will include decoding of whether UMA is enabled or not.  I think
> that module is initialized as of line 1683 below.
> 
> If you want to get the "head start" and aren't worried about "first run"
> execution of your field trial, then you should at least add a method to
metrics
> services which pulls the value out of a pref.  This way only the
> metrics_services will be aware of the name of the stored pref (and it will be
> easier to change said code in metrics services as we evolve).
> 

Agreed, this was me thinking I had found a shortcut but I hadn't - and it was
sloppy not to encapsulate the access in a method on the metrics service.

The updated version of this just relies on the metrics service already being
initialized when you call GetClientId on it, which will return the empty string
if it's not running (e.g. if it's not enabled).  Note that this requires me to
move the call to InitializeMetrics quite a bit earlier in the BrowserMain
function; reviewing the various things that can affect whether InitializeMetrics
starts metrics recording or not, this seems OK to me, and looking at
MetricsService::Start it is already delaying the actual work, but please double
check if this looks OK to you.

[jar (doing other things), 2011-04-29 00:57:11]

Thanks for cleaning up the pattern of usage for FieldTrials.  It is really
helpful to the codebase when folks do this!!!

I still have an argument with separating the construction into pieces (see
below), along with plausible ways you could combine them (that might be nicer
than passing a seed string in the constructor).

http://codereview.chromium.org/6883102/diff/3025/base/metrics/field_trial.cc
File base/metrics/field_trial.cc (right):

http://codereview.chromium.org/6883102/diff/3025/base/metrics/field_trial.cc#...
base/metrics/field_trial.cc:338: return EmptyString();
On 2011/04/21 22:10:02, jar wrote:
> If you switch over to using a detectable uninitialized value for client_id,
you
> could return it here rather than empty string.

In fact.... I'd argument that the EmptyString() would be a great way to say that
there is no client_id().

http://codereview.chromium.org/6883102/diff/3025/base/metrics/field_trial.h
File base/metrics/field_trial.h (right):

http://codereview.chromium.org/6883102/diff/3025/base/metrics/field_trial.h#n...
base/metrics/field_trial.h:298: static void EnableOneTimeRandomization(const
std::string& client_id);
I'm not sold on your arguments.  To be specific, for your two arguments:

a) "[enable] ...right before you create any one-time randomized trials...": 
This makes little sense to me, because I wouldn't want various users of this
service, presumably each residing all over creation in a binary, deciding on
*their* seed.  If we went in a lazy direction, we'd want some central service to
select the seed... you can't leave it to be decided by multiple consumers. 
...which again points back to doing something around construction time.

b) "[I don't like overloading initialization state with the presence of a
low-entropy seed]"  This seems motivated by a hypothentical future user that
doesn't want to use a seed, and doesn't want this functionality.  I don't think
we should cater to a fictional user that doesn't use the functionality.  ...but
avoiding the overloading is reasonable.... so... A boolean slot to indicate that
a seed was provided, or not provided, would be fine.  That would provide
something to to DCHECK on, and wouldn't overload the meaning of the seed. 

It seems a waste to provide a whole separate bool to indicate if there is such a
seed argument, and certainly a zero-length string might be a hint... but then
you could argue again about overloading the meaning of the string. I'm not as
concerned about that difference, as it still seems to focus construction details
at the singular point of time. 

Bottom line: piecemeal construction makes much more sense when there really are
a variety of use cases.  Without such real world use cases, a simple clear
construct-and-you-are-ready-to-roll interface is generally cleaner.

<sigh> That said... I didn't see a super-straight-forward way to call the
constructor with an argument (i.e., call a non-default constructor).  Currently
the code seems to cheat(?) and just put an instance definition in a class, and
count on the default (no-arg) constructor.  Most (all?) of the other
initializations also have this form :-/.  It could be that this initialization
will be different... and will pass an argument to the constructor.  It could be
the seed string, or it could be an instance of something (metrics services?)
that supports some GetUserSpecificSeed() interface.  The less code we need to
land in browser_main, and the less insight into this exchange, the better.

On 2011/04/28 01:03:50, Jói wrote:
> On 2011/04/21 22:10:02, jar wrote:
> > I'd be tempted to make this part of required initialization, passed into the
> > constructor on line 248, and then have assertions rather than the test at
line
> > 302.
> > 
> > The only concern is the lack of a value for the persistent id.  I'd suggest
> that
> > there be a constant, defined in metrics_services, which is
> > kUNINITALIZED_CLIENT_ID (or something of that sort).  The constructor can
then
> > detect this non-initialized state rather directly.
> 
> I disagree, but am willing to be overridden.  Let me explain why I disagree:
> 
> I think it's nice to be able to use FieldTrial / FieldTrialList without
> necessarily having a one-time random ID for your client.  It's also more
> flexible to be able to defer initializing the FieldTrialList with this until
you
> actually need it (right before you create any one-time randomized trials)
rather
> than having to do it at construction time.
> 
> For the ability of using the FieldTrialList without providing a one-time ID,
if
> you were suggesting allowing users to specify "not initialized" in the
> constructor, I think it makes for a cleaner public interface not to have to
> declare a "not initialized" value in the header file and document on the
> constructor that callers need to use it - cleaner to keep that detail
> implementation-private by using a separate initialization function, and safer
> since callers might not read the documentation and might specify e.g. some
> static string as the one-time secret, which would destroy our ability to
DCHECK
> on initialization before they request a one-time randomized trial.
> 
> I would suggest keeping the interface as-is, but as I said, I am willing to be
> overridden.

http://codereview.chromium.org/6883102/diff/2004/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/6883102/diff/2004/chrome/browser/browser_main....
chrome/browser/browser_main.cc:1330: MetricsService* metrics =
InitializeMetrics(parsed_command_line, local_state);
This will probably work.  I'm always nervous about moving much around at startup
time.  There is always the sad chance that we'll change dll load ordering or
such.  Be sure to watch the startup perf dashboard as you land with this change.

http://codereview.chromium.org/6883102/diff/2004/chrome/browser/browser_main....
chrome/browser/browser_main.cc:1341: parts->SetupFieldTrials();
Wouldn't lines 1327-1337 fit nicely into SetupFieldTrials()?

Note that this function always grows bigger and bigger... and folks keep trying
to factor out sections.  It sure seems like the initialization should fit nicely
in there.

[Jói, 2011-04-29 01:29:41]

Thanks Jim.  Wighout looking at the code I guess I can switch the
FieldTrialList object kept by BrowserMain to be a scoped_ptr and thus do the
at-construction-time init as you suggest (think I would just use an empty
string as guard value).  Agree with your other couple of comments too.

(sent from phone, pardon brevity)
On Apr 28, 2011 8:57 PM, <jar@chromium.org> wrote:
> Thanks for cleaning up the pattern of usage for FieldTrials. It is really
> helpful to the codebase when folks do this!!!
>
> I still have an argument with separating the construction into pieces (see
> below), along with plausible ways you could combine them (that might be
> nicer
> than passing a seed string in the constructor).
>
>
>
http://codereview.chromium.org/6883102/diff/3025/base/metrics/field_trial.cc
> File base/metrics/field_trial.cc (right):
>
>
http://codereview.chromium.org/6883102/diff/3025/base/metrics/field_trial.cc#...
> base/metrics/field_trial.cc:338: return EmptyString();
> On 2011/04/21 22:10:02, jar wrote:
>> If you switch over to using a detectable uninitialized value for
> client_id, you
>> could return it here rather than empty string.
>
> In fact.... I'd argument that the EmptyString() would be a great way to
> say that there is no client_id().
>
>
http://codereview.chromium.org/6883102/diff/3025/base/metrics/field_trial.h
> File base/metrics/field_trial.h (right):
>
>
http://codereview.chromium.org/6883102/diff/3025/base/metrics/field_trial.h#n...
> base/metrics/field_trial.h:298: static void
> EnableOneTimeRandomization(const std::string& client_id);
> I'm not sold on your arguments. To be specific, for your two arguments:
>
> a) "[enable] ...right before you create any one-time randomized
> trials...": This makes little sense to me, because I wouldn't want
> various users of this service, presumably each residing all over
> creation in a binary, deciding on *their* seed. If we went in a lazy
> direction, we'd want some central service to select the seed... you
> can't leave it to be decided by multiple consumers. ...which again
> points back to doing something around construction time.
>
> b) "[I don't like overloading initialization state with the presence of
> a low-entropy seed]" This seems motivated by a hypothentical future
> user that doesn't want to use a seed, and doesn't want this
> functionality. I don't think we should cater to a fictional user that
> doesn't use the functionality. ...but avoiding the overloading is
> reasonable.... so... A boolean slot to indicate that a seed was
> provided, or not provided, would be fine. That would provide something
> to to DCHECK on, and wouldn't overload the meaning of the seed.
>
> It seems a waste to provide a whole separate bool to indicate if there
> is such a seed argument, and certainly a zero-length string might be a
> hint... but then you could argue again about overloading the meaning of
> the string. I'm not as concerned about that difference, as it still
> seems to focus construction details at the singular point of time.
>
> Bottom line: piecemeal construction makes much more sense when there
> really are a variety of use cases. Without such real world use cases, a
> simple clear construct-and-you-are-ready-to-roll interface is generally
> cleaner.
>
> <sigh> That said... I didn't see a super-straight-forward way to call
> the constructor with an argument (i.e., call a non-default constructor).
> Currently the code seems to cheat(?) and just put an instance
> definition in a class, and count on the default (no-arg) constructor.
> Most (all?) of the other initializations also have this form :-/. It
> could be that this initialization will be different... and will pass an
> argument to the constructor. It could be the seed string, or it could
> be an instance of something (metrics services?) that supports some
> GetUserSpecificSeed() interface. The less code we need to land in
> browser_main, and the less insight into this exchange, the better.
>
> On 2011/04/28 01:03:50, Jói wrote:
>> On 2011/04/21 22:10:02, jar wrote:
>> > I'd be tempted to make this part of required initialization, passed
> into the
>> > constructor on line 248, and then have assertions rather than the
> test at line
>> > 302.
>> >
>> > The only concern is the lack of a value for the persistent id. I'd
> suggest
>> that
>> > there be a constant, defined in metrics_services, which is
>> > kUNINITALIZED_CLIENT_ID (or something of that sort). The
> constructor can then
>> > detect this non-initialized state rather directly.
>
>> I disagree, but am willing to be overridden. Let me explain why I
> disagree:
>
>> I think it's nice to be able to use FieldTrial / FieldTrialList
> without
>> necessarily having a one-time random ID for your client. It's also
> more
>> flexible to be able to defer initializing the FieldTrialList with this
> until you
>> actually need it (right before you create any one-time randomized
> trials) rather
>> than having to do it at construction time.
>
>> For the ability of using the FieldTrialList without providing a
> one-time ID, if
>> you were suggesting allowing users to specify "not initialized" in the
>> constructor, I think it makes for a cleaner public interface not to
> have to
>> declare a "not initialized" value in the header file and document on
> the
>> constructor that callers need to use it - cleaner to keep that detail
>> implementation-private by using a separate initialization function,
> and safer
>> since callers might not read the documentation and might specify e.g.
> some
>> static string as the one-time secret, which would destroy our ability
> to DCHECK
>> on initialization before they request a one-time randomized trial.
>
>> I would suggest keeping the interface as-is, but as I said, I am
> willing to be
>> overridden.
>
>
http://codereview.chromium.org/6883102/diff/2004/chrome/browser/browser_main.cc
> File chrome/browser/browser_main.cc (right):
>
>
http://codereview.chromium.org/6883102/diff/2004/chrome/browser/browser_main....
> chrome/browser/browser_main.cc:1330: MetricsService* metrics =
> InitializeMetrics(parsed_command_line, local_state);
> This will probably work. I'm always nervous about moving much around at
> startup time. There is always the sad chance that we'll change dll load
> ordering or such. Be sure to watch the startup perf dashboard as you
> land with this change.
>
>
http://codereview.chromium.org/6883102/diff/2004/chrome/browser/browser_main....
> chrome/browser/browser_main.cc:1341: parts->SetupFieldTrials();
> Wouldn't lines 1327-1337 fit nicely into SetupFieldTrials()?
>
> Note that this function always grows bigger and bigger... and folks keep
> trying to factor out sections. It sure seems like the initialization
> should fit nicely in there.
>
> http://codereview.chromium.org/6883102/

[Jói, 2011-05-02 18:55:03]

Please take another look.

Cheers,
Jói

http://codereview.chromium.org/6883102/diff/3025/base/metrics/field_trial.h
File base/metrics/field_trial.h (left):

http://codereview.chromium.org/6883102/diff/3025/base/metrics/field_trial.h#o...
base/metrics/field_trial.h:57: //    
!base::FieldTrialList::Find("MemoryExperiment")->group_name().empty());
On 2011/04/28 01:03:50, Jói wrote:
> On 2011/04/21 22:10:02, jar wrote:
> > Removal of this code is a VERY good improvement.  The class has evolved, and
I
> > think you are totally correct, that the name can no longer be empty (since
the
> > commitment is made when we query the state... and worst case, it becomes the
> > default string... which is required to be non-empty).
> > 
> > There are a number of places where the above pattern is currently used, and
> > should be simplified to match your correction.  Search for:
> > FieldTrialList::Find
> > 
> > to get a list.
> > 
> > If you don't want to do this, please file a bug against me and I'll do the
> > cleanup.
> 
> I've updated all the call-sites now.  I introduced a new static function
> TrialExists(name) that is equivalent to !!Find(name) to avoid having to coerce
> to boolean at every call-site.  I also updated the docs to suggest using a
> static const bool to store the results of the find, as that seems like a good
> idea in most cases.

FYI, there were some merge changes to resolve, I updated call sites again.

http://codereview.chromium.org/6883102/diff/3025/base/metrics/field_trial.h
File base/metrics/field_trial.h (right):

http://codereview.chromium.org/6883102/diff/3025/base/metrics/field_trial.h#n...
base/metrics/field_trial.h:298: static void EnableOneTimeRandomization(const
std::string& client_id);
On 2011/04/29 00:57:11, jar wrote:
> I'm not sold on your arguments.  To be specific, for your two arguments:
> 
> a) "[enable] ...right before you create any one-time randomized trials...": 
> This makes little sense to me, because I wouldn't want various users of this
> service, presumably each residing all over creation in a binary, deciding on
> *their* seed.  If we went in a lazy direction, we'd want some central service
to
> select the seed... you can't leave it to be decided by multiple consumers. 
> ...which again points back to doing something around construction time.
> 
> b) "[I don't like overloading initialization state with the presence of a
> low-entropy seed]"  This seems motivated by a hypothentical future user that
> doesn't want to use a seed, and doesn't want this functionality.  I don't
think
> we should cater to a fictional user that doesn't use the functionality. 
...but
> avoiding the overloading is reasonable.... so... A boolean slot to indicate
that
> a seed was provided, or not provided, would be fine.  That would provide
> something to to DCHECK on, and wouldn't overload the meaning of the seed. 
> 
> It seems a waste to provide a whole separate bool to indicate if there is such
a
> seed argument, and certainly a zero-length string might be a hint... but then
> you could argue again about overloading the meaning of the string. I'm not as
> concerned about that difference, as it still seems to focus construction
details
> at the singular point of time. 
> 
> Bottom line: piecemeal construction makes much more sense when there really
are
> a variety of use cases.  Without such real world use cases, a simple clear
> construct-and-you-are-ready-to-roll interface is generally cleaner.
> 
> <sigh> That said... I didn't see a super-straight-forward way to call the
> constructor with an argument (i.e., call a non-default constructor). 
Currently
> the code seems to cheat(?) and just put an instance definition in a class, and
> count on the default (no-arg) constructor.  Most (all?) of the other
> initializations also have this form :-/.  It could be that this initialization
> will be different... and will pass an argument to the constructor.  It could
be
> the seed string, or it could be an instance of something (metrics services?)
> that supports some GetUserSpecificSeed() interface.  The less code we need to
> land in browser_main, and the less insight into this exchange, the better.
> 
> On 2011/04/28 01:03:50, Jói wrote:
> > On 2011/04/21 22:10:02, jar wrote:
> > > I'd be tempted to make this part of required initialization, passed into
the
> > > constructor on line 248, and then have assertions rather than the test at
> line
> > > 302.
> > > 
> > > The only concern is the lack of a value for the persistent id.  I'd
suggest
> > that
> > > there be a constant, defined in metrics_services, which is
> > > kUNINITALIZED_CLIENT_ID (or something of that sort).  The constructor can
> then
> > > detect this non-initialized state rather directly.
> > 
> > I disagree, but am willing to be overridden.  Let me explain why I disagree:
> > 
> > I think it's nice to be able to use FieldTrial / FieldTrialList without
> > necessarily having a one-time random ID for your client.  It's also more
> > flexible to be able to defer initializing the FieldTrialList with this until
> you
> > actually need it (right before you create any one-time randomized trials)
> rather
> > than having to do it at construction time.
> > 
> > For the ability of using the FieldTrialList without providing a one-time ID,
> if
> > you were suggesting allowing users to specify "not initialized" in the
> > constructor, I think it makes for a cleaner public interface not to have to
> > declare a "not initialized" value in the header file and document on the
> > constructor that callers need to use it - cleaner to keep that detail
> > implementation-private by using a separate initialization function, and
safer
> > since callers might not read the documentation and might specify e.g. some
> > static string as the one-time secret, which would destroy our ability to
> DCHECK
> > on initialization before they request a one-time randomized trial.
> > 
> > I would suggest keeping the interface as-is, but as I said, I am willing to
be
> > overridden.
> 

I've switched to having client_id as an argument to FieldTrialList and using a
scoped_ptr<FieldTrialList> in BrowserMainParts to let us defer initialization of
the object until we have the parameter available.

Just to clarify on the former approach that is now gone, it had DCHECKs in place
to ensure you did not initialize multiple times, and that you initialized before
using a one-time randomized trial, so there was no risk of misuse.

Switching to constructor initialization uncovered a bug, I wasn't reflecting the
client_id over to the renderer process. So there's something to be said for
construction-time initialization :)

http://codereview.chromium.org/6883102/diff/2004/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/6883102/diff/2004/chrome/browser/browser_main....
chrome/browser/browser_main.cc:1330: MetricsService* metrics =
InitializeMetrics(parsed_command_line, local_state);
On 2011/04/29 00:57:11, jar wrote:
> This will probably work.  I'm always nervous about moving much around at
startup
> time.  There is always the sad chance that we'll change dll load ordering or
> such.  Be sure to watch the startup perf dashboard as you land with this
change.

I will watch the startup perf dash.

http://codereview.chromium.org/6883102/diff/2004/chrome/browser/browser_main....
chrome/browser/browser_main.cc:1341: parts->SetupFieldTrials();
On 2011/04/29 00:57:11, jar wrote:
> Wouldn't lines 1327-1337 fit nicely into SetupFieldTrials()?
> 
> Note that this function always grows bigger and bigger... and folks keep
trying
> to factor out sections.  It sure seems like the initialization should fit
nicely
> in there.

Good point.  I've factored it out.

[Jói, 2011-05-02 19:34:05]

Adding mark@chromium.org as reviewer for base/OWNERS approval; this would
primarily be the change to base/rand_util.* and the addition of
base/metrics/OWNERS.

Will also email mark@ separately.

Cheers,
Jói

[Mark Mentovai, 2011-05-02 19:39:58]

LGTM for base approval, I looked at the files in base only, and defer to jar for
base/metrics.

[jar (doing other things), 2011-05-03 00:17:47]

FWIW, the review uncovered a micro-bug below in the pre-existing code (see
below).

I also had a number of questions.

Thanks,

Jim

http://codereview.chromium.org/6883102/diff/15004/base/metrics/field_trial.cc
File base/metrics/field_trial.cc (right):

http://codereview.chromium.org/6883102/diff/15004/base/metrics/field_trial.cc...
base/metrics/field_trial.cc:78: DCHECK_EQ(group_, kNotFinalized);
Could you also add a DCHECK_EQ(0, next_group_number_) to show that we haven't
started adding groups (and hence haven't yet compared probabilities to the
rand() based random number).

http://codereview.chromium.org/6883102/diff/15004/base/metrics/field_trial.cc...
base/metrics/field_trial.cc:134: group();  // call group() to make group
assignment was done.
nit (in old code): 
"make group assignment" --> "make sure group assignment"

http://codereview.chromium.org/6883102/diff/15004/base/metrics/field_trial.cc...
base/metrics/field_trial.cc:267: // First, write just the client_id plus a
separator.
Please add a DCHECK() comparable to line 279 to ensure that we don't put a
separator into the client_id.

http://codereview.chromium.org/6883102/diff/15004/base/metrics/field_trial.cc...
base/metrics/field_trial.cc:278: group_name = it->second->default_group_name();
This is a bug (now... though not because of your code, and not that big a bug).

When there is no definitive winner yet, we should not include the field trial.
We can (now) allow it to be resolved later.  Hence I *think* the correct thing
is:
if (group_name.empty())
  continue;

Alternatively, if we did want to "default" a group, we would have to do it using
standard code, establishing the default in our process as well.  If we don't
establish the same result in the browser process, then the browser process
*could* later establish a value, which would propagate to the renderers, and
conflict with this initialization value.  Unfortunately, establishing a value
for an incompletely specified FieldTrial *might* race with the ongoing
initialization of that FieldTrial <sigh>.  

Bottom line: I think we should just have the continue.

FYI:

We currently notify all pre-existing renderer processes when we do finalize a
field trial... so the "correct" value will make its way (sooner or later) to the
renderer.  IMO, it is questionable practice to set field trials late.... but
that is what the use case (feature request) called for... and caveat developor)

http://codereview.chromium.org/6883102/diff/15004/base/metrics/field_trial_un...
File base/metrics/field_trial_unittest.cc (right):

http://codereview.chromium.org/6883102/diff/15004/base/metrics/field_trial_un...
base/metrics/field_trial_unittest.cc:241: EXPECT_EQ(client_id + "/Some
name/Default some name/", save_string);
Warning: This test may need to change if we stop defaulting incomplete
FieldTrials during pickling.

http://codereview.chromium.org/6883102/diff/15004/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/6883102/diff/15004/chrome/browser/browser_main...
chrome/browser/browser_main.cc:571: } else {
Please use an early return within the if block, and avoid the else block with
its indentation.

I know you just moved this code... but you may as well clean up the readability
during the relocation.

http://codereview.chromium.org/6883102/diff/15004/chrome/browser/browser_main...
chrome/browser/browser_main.cc:575: #if defined(GOOGLE_CHROME_BUILD)
I'm not sure if an early return works as well with a #if, but these nested
returns are ugly and hard to read.  I'd suggest:
#if !defined(GOOGLE_CHROME_BUILD)
        return metrics;
#endif  // GOOGLE_CHROME_METRICS

I'm not sure if you'd have to use a #else clause, but if you don't it would be a
win to not have to read these nested preprocessor conditionals.

http://codereview.chromium.org/6883102/diff/15004/content/renderer/renderer_m...
File content/renderer/renderer_main.cc (right):

http://codereview.chromium.org/6883102/diff/15004/content/renderer/renderer_m...
content/renderer/renderer_main.cc:197: base::FieldTrialList
field_trial(EmptyString());
I'm not sure about this logic now....

The test in line 199 will always return true, as you (now) always pass in the
client_id.  Hence we'll always call for CreateTrialsInChildProcess().  That
suggests you shouldn't bother with the test for the switch.  It also means that
since we *always* call the CreateTrialInChirldProcess(), that we could also do
the construction of the FieldTrialList in that method, and not have to reset the
seed string.  ...which seems cleaner.

...but...

The other alternative is that we should ONLY construct persistent field trials
in the browser process.  This would mean we wouldn't need to pass the seed at
all.  The ONLY reason to create field trials in renderer processes (rather than
a browser process) would be to be able to have different render processes have
different trial values... which would never happen if we had a persistent trial.

Bottom line: Perhaps we should outlaw persistent trials in the renderer process,
and not bother to pass in the seed.  I leaning that way.  This would also
simplify the code a bit.

WDYT?

[Jói, 2011-05-03 17:41:47]

I think that should be it, please take a look.

Cheers,
Jói

http://codereview.chromium.org/6883102/diff/15004/base/metrics/field_trial.cc
File base/metrics/field_trial.cc (right):

http://codereview.chromium.org/6883102/diff/15004/base/metrics/field_trial.cc...
base/metrics/field_trial.cc:78: DCHECK_EQ(group_, kNotFinalized);
On 2011/05/03 00:17:47, jar wrote:
> Could you also add a DCHECK_EQ(0, next_group_number_) to show that we haven't
> started adding groups (and hence haven't yet compared probabilities to the
> rand() based random number).

Done.

http://codereview.chromium.org/6883102/diff/15004/base/metrics/field_trial.cc...
base/metrics/field_trial.cc:134: group();  // call group() to make group
assignment was done.
On 2011/05/03 00:17:47, jar wrote:
> nit (in old code): 
> "make group assignment" --> "make sure group assignment"

Done.

http://codereview.chromium.org/6883102/diff/15004/base/metrics/field_trial.cc...
base/metrics/field_trial.cc:267: // First, write just the client_id plus a
separator.
On 2011/05/03 00:17:47, jar wrote:
> Please add a DCHECK() comparable to line 279 to ensure that we don't put a
> separator into the client_id.

Removed this code.

http://codereview.chromium.org/6883102/diff/15004/base/metrics/field_trial.cc...
base/metrics/field_trial.cc:278: group_name = it->second->default_group_name();
On 2011/05/03 00:17:47, jar wrote:
> This is a bug (now... though not because of your code, and not that big a
bug).
> 
> When there is no definitive winner yet, we should not include the field trial.
> We can (now) allow it to be resolved later.  Hence I *think* the correct thing
> is:
> if (group_name.empty())
>   continue;
> 
> Alternatively, if we did want to "default" a group, we would have to do it
using
> standard code, establishing the default in our process as well.  If we don't
> establish the same result in the browser process, then the browser process
> *could* later establish a value, which would propagate to the renderers, and
> conflict with this initialization value.  Unfortunately, establishing a value
> for an incompletely specified FieldTrial *might* race with the ongoing
> initialization of that FieldTrial <sigh>.  
> 
> Bottom line: I think we should just have the continue.
> 
> FYI:
> 
> We currently notify all pre-existing renderer processes when we do finalize a
> field trial... so the "correct" value will make its way (sooner or later) to
the
> renderer.  IMO, it is questionable practice to set field trials late.... but
> that is what the use case (feature request) called for... and caveat
developor)

Switched to a continue.

http://codereview.chromium.org/6883102/diff/15004/base/metrics/field_trial_un...
File base/metrics/field_trial_unittest.cc (right):

http://codereview.chromium.org/6883102/diff/15004/base/metrics/field_trial_un...
base/metrics/field_trial_unittest.cc:241: EXPECT_EQ(client_id + "/Some
name/Default some name/", save_string);
On 2011/05/03 00:17:47, jar wrote:
> Warning: This test may need to change if we stop defaulting incomplete
> FieldTrials during pickling.

Done.

http://codereview.chromium.org/6883102/diff/15004/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/6883102/diff/15004/chrome/browser/browser_main...
chrome/browser/browser_main.cc:571: } else {
On 2011/05/03 00:17:47, jar wrote:
> Please use an early return within the if block, and avoid the else block with
> its indentation.
> 
> I know you just moved this code... but you may as well clean up the
readability
> during the relocation.

Done.

http://codereview.chromium.org/6883102/diff/15004/chrome/browser/browser_main...
chrome/browser/browser_main.cc:575: #if defined(GOOGLE_CHROME_BUILD)
On 2011/05/03 00:17:47, jar wrote:
> I'm not sure if an early return works as well with a #if, but these nested
> returns are ugly and hard to read.  I'd suggest:
> #if !defined(GOOGLE_CHROME_BUILD)
>         return metrics;
> #endif  // GOOGLE_CHROME_METRICS
> 
> I'm not sure if you'd have to use a #else clause, but if you don't it would be
a
> win to not have to read these nested preprocessor conditionals.
> 

After removing the else (previous comment) it seems cleanest to just keep the
return at end.  I added a comment on the #endif at line 588 to make it easy to
see where the whole #ifdef block ends.

http://codereview.chromium.org/6883102/diff/15004/content/renderer/renderer_m...
File content/renderer/renderer_main.cc (right):

http://codereview.chromium.org/6883102/diff/15004/content/renderer/renderer_m...
content/renderer/renderer_main.cc:197: base::FieldTrialList
field_trial(EmptyString());
On 2011/05/03 00:17:47, jar wrote:
> I'm not sure about this logic now....
> 
> The test in line 199 will always return true, as you (now) always pass in the
> client_id.  Hence we'll always call for CreateTrialsInChildProcess().  That
> suggests you shouldn't bother with the test for the switch.  It also means
that
> since we *always* call the CreateTrialInChirldProcess(), that we could also do
> the construction of the FieldTrialList in that method, and not have to reset
the
> seed string.  ...which seems cleaner.
> 
> ...but...
> 
> The other alternative is that we should ONLY construct persistent field trials
> in the browser process.  This would mean we wouldn't need to pass the seed at
> all.  The ONLY reason to create field trials in renderer processes (rather
than
> a browser process) would be to be able to have different render processes have
> different trial values... which would never happen if we had a persistent
trial.
> 
> Bottom line: Perhaps we should outlaw persistent trials in the renderer
process,
> and not bother to pass in the seed.  I leaning that way.  This would also
> simplify the code a bit.
> 
> WDYT?

I've outlawed persistent trials in the renderer process.  I started wondering
about this yesterday after I'd done the implementation in StatesToString and
CreateTrialsInChildProcess but decided to leave it in, but I think your comments
show that it really isn't necessary and might even be undesirable to allow
renderer processes to create their own one-time randomized trials.

[jar (doing other things), 2011-05-03 20:08:14]

LGTM

Thanks!

[Jói, 2011-05-04 17:37:05]

Adding Wan-Teh for net/OWNERS approval.

[wtc, 2011-05-04 17:58:05]

LGTM.  I only reviewed the changes to base/rand_util{h,cc}
and net.

http://codereview.chromium.org/6883102/diff/24001/base/metrics/OWNERS
File base/metrics/OWNERS (right):

http://codereview.chromium.org/6883102/diff/24001/base/metrics/OWNERS#newcode1
base/metrics/OWNERS:1: jar@chromium.org
jar: to require owners' approval, you need to add
  set noparent
to this file.

http://codereview.chromium.org/6883102/diff/24001/base/rand_util.h
File base/rand_util.h (right):

http://codereview.chromium.org/6883102/diff/24001/base/rand_util.h#newcode33
base/rand_util.h:33: // double, i.e. in the range [0, 1). Thread-safe.
I didn't understand this description of BitsToOpenEndedUnitInterval
until I looked at its implementation.

I think I was mislead by the mention of "rand-like".  So I
would suggest at least changing the comment to:
  // Given input |bits|, converts with maximum precision to a double
  // in the range [0, 1). Thread-safe.

"convert with maximum precision" is still not clear, but
I cannot suggest better wording now.

[Jói, 2011-05-04 20:02:09]

Thanks Wan-Teh.

Jim, please see question below.


Cheers,
Jói

http://codereview.chromium.org/6883102/diff/24001/base/metrics/OWNERS
File base/metrics/OWNERS (right):

http://codereview.chromium.org/6883102/diff/24001/base/metrics/OWNERS#newcode1
base/metrics/OWNERS:1: jar@chromium.org
On 2011/05/04 17:58:05, wtc wrote:
> jar: to require owners' approval, you need to add
>   set noparent
> to this file.

My intent was that jar@ or anybody in base/OWNERS could approve changes in
base/metrics.  base/OWNERS already has "set noparent" so the approval would (if
I understand correctly) be forced, just not limited to only jar@.

Jim, let me know if you would prefer "set noparent" here and I can add it before
I commit.

http://codereview.chromium.org/6883102/diff/24001/base/rand_util.h
File base/rand_util.h (right):

http://codereview.chromium.org/6883102/diff/24001/base/rand_util.h#newcode33
base/rand_util.h:33: // double, i.e. in the range [0, 1). Thread-safe.
On 2011/05/04 17:58:05, wtc wrote:
> I didn't understand this description of BitsToOpenEndedUnitInterval
> until I looked at its implementation.
> 
> I think I was mislead by the mention of "rand-like".  So I
> would suggest at least changing the comment to:
>   // Given input |bits|, converts with maximum precision to a double
>   // in the range [0, 1). Thread-safe.
> 
> "convert with maximum precision" is still not clear, but
> I cannot suggest better wording now.
>   

You're right, I missed removing the "rand-like" thing when I changed the name of
the method to not include "rand".  Switched to the wording you suggested.

[Mark Mentovai, 2011-05-04 20:21:24]

http://codereview.chromium.org/6883102/diff/24001/base/metrics/OWNERS
File base/metrics/OWNERS (right):

http://codereview.chromium.org/6883102/diff/24001/base/metrics/OWNERS#newcode1
base/metrics/OWNERS:1: jar@chromium.org
Please don’t “set noparent” any OWNERS file that has only one OWNER.

[Jói, 2011-05-04 20:33:03]

Good point - I won't be adding "set noparent" to the OWNERS file.

I've uploaded a final version that modifies the comment in base/rand_util.h as
Wan-Teh suggested.

I plan to commit tonight, let me know if there are any objections.

Cheers,
Jói



On 2011/05/04 20:21:24, Mark Mentovai wrote:
> http://codereview.chromium.org/6883102/diff/24001/base/metrics/OWNERS
> File base/metrics/OWNERS (right):
> 
> http://codereview.chromium.org/6883102/diff/24001/base/metrics/OWNERS#newcode1
> base/metrics/OWNERS:1: mailto:jar@chromium.org
> Please don’t “set noparent” any OWNERS file that has only one OWNER.

[jar (doing other things), 2011-05-04 20:54:46]

Still LGTM

Mark's comment is good... let's not no-parent.

[Jói, 2011-05-05 02:19:08]

Adding jam@ for rubber-stamp content/OWNERS approval of the one trivial
call-site change under content/.  Will TBR.

[Jói, 2011-05-05 15:07:39]

http://codereview.chromium.org/6883102/diff/2004/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/6883102/diff/2004/chrome/browser/browser_main....
chrome/browser/browser_main.cc:1330: MetricsService* metrics =
InitializeMetrics(parsed_command_line, local_state);
On 2011/04/29 00:57:11, jar wrote:
> Be sure to watch the startup perf dashboard as you land with this change.

Just to confirm, I looked in detail at every Startup/New Tab Cold/New Tab Warm
perf chart and there is nothing to indicate this change slowed things down.  I
also glanced over the rest of the perf dashboard, again no indication of a
slowdown.

Cheers,
Jói

[Jói, 2011-05-06 19:48:59]

Adding ukai@chromium.org to review change in initialization of websocket
experiment.

Ukai-san, the only true delta in this latest patch is what you'll see behind
these two links:
http://codereview.chromium.org/6883102/diff2/29002:38001/chrome/browser/brows...
http://codereview.chromium.org/6883102/diff2/29002:38001/chrome/browser/brows...

Everything else has previously been reviewed by others on this thread.  Any
other deltas between patch set 14 and patch set 13 (which I committed yesterday
but rolled back due to http://crbug.com/81750) are simply from merging to a
later version of the code base.

There is a subtle change in logic here:  Before this change, the experiment
would only be enabled in official builds when the user has enabled metrics
reporting.  After this change, it will be enabled in all builds (not just
official) when the user has enabled metrics recording (not necessarily
recording+reporting).  I think this is probably the right thing to do (it looks
like your initial version of the code enabling the experiment would enable it in
both official and non-official builds if metrics were enabled) but I would like
you to verify.

Cheers,
Jói

[ukai, 2011-05-10 01:46:14]

On 2011/05/06 19:48:59, Jói wrote:
> Adding mailto:ukai@chromium.org to review change in initialization of
websocket
> experiment.
> 
> Ukai-san, the only true delta in this latest patch is what you'll see behind
> these two links:
>
http://codereview.chromium.org/6883102/diff2/29002:38001/chrome/browser/brows...
>
http://codereview.chromium.org/6883102/diff2/29002:38001/chrome/browser/brows...
> 
> Everything else has previously been reviewed by others on this thread.  Any
> other deltas between patch set 14 and patch set 13 (which I committed
yesterday
> but rolled back due to http://crbug.com/81750) are simply from merging to a
> later version of the code base.
> 
> There is a subtle change in logic here:  Before this change, the experiment
> would only be enabled in official builds when the user has enabled metrics
> reporting.  After this change, it will be enabled in all builds (not just
> official) when the user has enabled metrics recording (not necessarily
> recording+reporting).  I think this is probably the right thing to do (it
looks
> like your initial version of the code enabling the experiment would enable it
in
> both official and non-official builds if metrics were enabled) but I would
like
> you to verify.

LGTM