Remove the hash fields from FieldTrials.

Remove the hash fields from FieldTrials.

We want to migrate the hash fields and related methods from FieldTrials over to experiments_helper. We've also updated the unit tests that accomodate these changes.

We've also refactored the experiments_helper APIs for GoogleExperimentIDs to take strings instead of NameGroupIds as keys... we do the hashing internally instead.

BUG=None
TEST=Ensure that base_unittests FieldTrialTest.* all pass. Ensure that unit_tests ExperimentsHelperTest.* all pass.


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

[SteveT, 2012-04-25 17:22:52]

PTAL.

[MAD, 2012-04-25 20:03:22]

Some comments/suggestions...

Good stuff... Thanks for that!

BYE
MAD

https://chromiumcodereview.appspot.com/10151017/diff/1/chrome/common/metrics/...
File chrome/common/metrics/experiments_helper.cc (right):

https://chromiumcodereview.appspot.com/10151017/diff/1/chrome/common/metrics/...
chrome/common/metrics/experiments_helper.cc:87: DCHECK(bits !=
kReservedHashValue);
Actually, I don't think we need this anymore. This was to initialize the member
until it got set... Since we don't have a member to initialize anymore, this is
OK...

https://chromiumcodereview.appspot.com/10151017/diff/1/chrome/common/metrics/...
chrome/common/metrics/experiments_helper.cc:107: void
GetFieldTrialNameGroupIdsForSelectedGroups(
Maybe add a comment about the separation from previous function for testing
purposes...

https://chromiumcodereview.appspot.com/10151017/diff/1/chrome/common/metrics/...
chrome/common/metrics/experiments_helper.cc:112: it != selected_groups.end();
++it)
We use {} when the condition/loop has more than one line...
You could also put the declaration of the iterator on a separate line before the
for loop to avoid that.

https://chromiumcodereview.appspot.com/10151017/diff/1/chrome/common/metrics/...
File chrome/common/metrics/experiments_helper.h (right):

https://chromiumcodereview.appspot.com/10151017/diff/1/chrome/common/metrics/...
chrome/common/metrics/experiments_helper.h:48: typedef
std::vector<base::FieldTrial::SelectedGroup> SelectedGroups;
Maybe we could put this typedef within the FieldTrialList class?

https://chromiumcodereview.appspot.com/10151017/diff/1/chrome/common/metrics/...
chrome/common/metrics/experiments_helper.h:70: void
GetFieldTrialNameGroupIdsForSelectedGroups(
You could also put this one in an unnamed namespace (as HashName) and expose a
Test method appropriately prefixed...

https://chromiumcodereview.appspot.com/10151017/diff/1/chrome/common/metrics/...
chrome/common/metrics/experiments_helper.h:75: NameGroupId MakeNameGroupId(const
std::string& trial_name,
Will this be needed other than by tests?

I'm thinking that we could also only expose a testing::TestXXX method for this,
and maybe we wouldn't need to expose HashName since we can test the hashing by
calling this method instead... What Do You Think?

https://chromiumcodereview.appspot.com/10151017/diff/1/chrome/common/metrics/...
chrome/common/metrics/experiments_helper.h:79: uint32 TestingHashName(const
std::string& name);
To make it every clearer, you could put it in the testing namespace, outside of
the experiments_helper namespace.

https://chromiumcodereview.appspot.com/10151017/diff/1/chrome/common/metrics/...
File chrome/common/metrics/experiments_helper_unittest.cc (right):

https://chromiumcodereview.appspot.com/10151017/diff/1/chrome/common/metrics/...
chrome/common/metrics/experiments_helper_unittest.cc:99:
EXPECT_TRUE(trial_one_id != name_group_ids[i].name ||
This doesn't give an error if the returned vector has the same set of values
twice...

[Ilya Sherman, 2012-04-25 20:26:57]

Drive-by nit :)

https://chromiumcodereview.appspot.com/10151017/diff/1/chrome/browser/metrics...
File chrome/browser/metrics/metrics_log.h (right):

https://chromiumcodereview.appspot.com/10151017/diff/1/chrome/browser/metrics...
chrome/browser/metrics/metrics_log.h:99:
std::vector<experiments_helper::NameGroupId>* field_trial_ids) const;
nit: Can we forward-declare this rather than #inlcuding the header?

[SteveT, 2012-04-26 19:03:30]

I believe I've addressed everything. PTAL. Ass soon as you're all done, I'll
pass this onto jar@ for OWNERS review.

https://chromiumcodereview.appspot.com/10151017/diff/1/chrome/browser/metrics...
File chrome/browser/metrics/metrics_log.h (right):

https://chromiumcodereview.appspot.com/10151017/diff/1/chrome/browser/metrics...
chrome/browser/metrics/metrics_log.h:99:
std::vector<experiments_helper::NameGroupId>* field_trial_ids) const;
Yes! I didn't think we could, since NameGroupId isn't a ptr or ref here, but
turns out it's OK as it's a template argument. Thanks  :)

https://chromiumcodereview.appspot.com/10151017/diff/1/chrome/common/metrics/...
File chrome/common/metrics/experiments_helper.cc (right):

https://chromiumcodereview.appspot.com/10151017/diff/1/chrome/common/metrics/...
chrome/common/metrics/experiments_helper.cc:87: DCHECK(bits !=
kReservedHashValue);
Ah okay, I was wondering about that. I've removed this check and the constant
(since it's the only user).

https://chromiumcodereview.appspot.com/10151017/diff/1/chrome/common/metrics/...
chrome/common/metrics/experiments_helper.cc:107: void
GetFieldTrialNameGroupIdsForSelectedGroups(
Cool. I have a comment around the testing namespace for this.

https://chromiumcodereview.appspot.com/10151017/diff/1/chrome/common/metrics/...
chrome/common/metrics/experiments_helper.cc:112: it != selected_groups.end();
++it)
I do recall pkasting telling me that they are only needed if the loop contents
is > 1 line, and it doesn't matter if the condition has > 1 line... But I'll add
the braces here since I think it's a bit neater.

https://chromiumcodereview.appspot.com/10151017/diff/1/chrome/common/metrics/...
File chrome/common/metrics/experiments_helper.h (right):

https://chromiumcodereview.appspot.com/10151017/diff/1/chrome/common/metrics/...
chrome/common/metrics/experiments_helper.h:48: typedef
std::vector<base::FieldTrial::SelectedGroup> SelectedGroups;
Makes more sense. Done.

https://chromiumcodereview.appspot.com/10151017/diff/1/chrome/common/metrics/...
chrome/common/metrics/experiments_helper.h:70: void
GetFieldTrialNameGroupIdsForSelectedGroups(
On 2012/04/25 20:03:22, MAD wrote:
> You could also put this one in an unnamed namespace (as HashName) and expose a
> Test method appropriately prefixed...

Done.

https://chromiumcodereview.appspot.com/10151017/diff/1/chrome/common/metrics/...
chrome/common/metrics/experiments_helper.h:75: NameGroupId MakeNameGroupId(const
std::string& trial_name,
Technically you need it to get GXIDs for individual FieldTrials, but since only
GetFieldTrialNameGroupIds uses it right now, we can technically hide it and
expose a testing version.

I'm not a fan of using this inside of HashName for tests that require a single
Hash for a verification. Seems hacky.

https://chromiumcodereview.appspot.com/10151017/diff/1/chrome/common/metrics/...
chrome/common/metrics/experiments_helper.h:79: uint32 TestingHashName(const
std::string& name);
On 2012/04/25 20:03:22, MAD wrote:
> To make it every clearer, you could put it in the testing namespace, outside
of
> the experiments_helper namespace.

Done.

https://chromiumcodereview.appspot.com/10151017/diff/1/chrome/common/metrics/...
File chrome/common/metrics/experiments_helper_unittest.cc (right):

https://chromiumcodereview.appspot.com/10151017/diff/1/chrome/common/metrics/...
chrome/common/metrics/experiments_helper_unittest.cc:99:
EXPECT_TRUE(trial_one_id != name_group_ids[i].name ||
Right. Going to do this with a set, which requires me to expose my
NameGroupIdCompare helper class in experiments_helper.h. Is it okay that I
inlined the implementation there or should I move it to experiments_helper.cc?

[MAD, 2012-04-26 19:40:12]

Almost there... A few more comments...

BYE
MAD

https://chromiumcodereview.appspot.com/10151017/diff/1/chrome/common/metrics/...
File chrome/common/metrics/experiments_helper.h (right):

https://chromiumcodereview.appspot.com/10151017/diff/1/chrome/common/metrics/...
chrome/common/metrics/experiments_helper.h:75: NameGroupId MakeNameGroupId(const
std::string& trial_name,
On 2012/04/26 19:03:31, SteveT wrote:
> Technically you need it to get GXIDs for individual FieldTrials, but since
only
> GetFieldTrialNameGroupIds uses it right now, we can technically hide it and
> expose a testing version.
> 
> I'm not a fan of using this inside of HashName for tests that require a single
> Hash for a verification. Seems hacky.

I think it's fine to test HashName only via this method, we don't really need to
test a single name hash, if it works for two, it sure works for one... But I'm
OK with leaving it like this too if you insist... :-)

https://chromiumcodereview.appspot.com/10151017/diff/1/chrome/common/metrics/...
File chrome/common/metrics/experiments_helper_unittest.cc (right):

https://chromiumcodereview.appspot.com/10151017/diff/1/chrome/common/metrics/...
chrome/common/metrics/experiments_helper_unittest.cc:99:
EXPECT_TRUE(trial_one_id != name_group_ids[i].name ||
On 2012/04/26 19:03:31, SteveT wrote:
> Right. Going to do this with a set, which requires me to expose my
> NameGroupIdCompare helper class in experiments_helper.h. Is it okay that I
> inlined the implementation there or should I move it to experiments_helper.cc?

You could also create a C style array on the stack, assert that the
name_group_ids has the same size, and then walk the C style, looking for the
value in the vector, as opposed to the other way around...

https://chromiumcodereview.appspot.com/10151017/diff/11012/chrome/common/metr...
File chrome/common/metrics/experiments_helper.h (right):

https://chromiumcodereview.appspot.com/10151017/diff/11012/chrome/common/metr...
chrome/common/metrics/experiments_helper.h:58: bool operator() (const
experiments_helper::NameGroupId& lhs,
I don't think you need the namespace scope in here, since we are within it
now...

https://chromiumcodereview.appspot.com/10151017/diff/11012/chrome/common/metr...
chrome/common/metrics/experiments_helper.h:112: experiments_helper::NameGroupId
TestMakeNameGroupId(
I don't think you need TestMakeNameGroupId since the method is available
above... Unless I missed something???

https://chromiumcodereview.appspot.com/10151017/diff/11012/chrome/common/metr...
File chrome/common/metrics/experiments_helper_unittest.cc (right):

https://chromiumcodereview.appspot.com/10151017/diff/11012/chrome/common/metr...
chrome/common/metrics/experiments_helper_unittest.cc:105:
expected_groups.erase(it);
I think it's dangerous for some container classes to erase an entity with an
iterator and then increment this iterator... It doesn't always work, you may
skip elements...

So that might be another good incentive to use the trick I propose in the
previous comment... Have you walked this code in the debugger to make sure you
go through the loop twice?

[SteveT, 2012-04-26 21:06:03]

Since we'd like to speed up our GXID work, I am going to R+jar before MAD's
official l.g.t.m. I think we're almost done, anyway.

Jim: Here is the refactor, as promised. We've now moved most of the NameGroupId
hash nastiness out of base::FieldTrials and into experiments_helper. We've left
a replacement to the GetFieldTrialNameGroupIds which instead returns structs of
the selected groups. Please take a look at all the base/metrics/* changes, and
feel free to look at everything else if you want to see where the code has
moved.

http://codereview.chromium.org/10151017/diff/11012/chrome/common/metrics/expe...
File chrome/common/metrics/experiments_helper.h (right):

http://codereview.chromium.org/10151017/diff/11012/chrome/common/metrics/expe...
chrome/common/metrics/experiments_helper.h:58: bool operator() (const
experiments_helper::NameGroupId& lhs,
Yup, thanks for the catch :)

Oh, and you're okay with me in-lining this utility struct, yeah?

http://codereview.chromium.org/10151017/diff/11012/chrome/common/metrics/expe...
chrome/common/metrics/experiments_helper.h:112: experiments_helper::NameGroupId
TestMakeNameGroupId(
We discussed changing the API to accept strings and using HashNameIds. I wanted
to put off doing this, but I think it makes good sense :)  It makes the API
simpler, and also resolves some of these issues with where MakeNameGroupId is
exposed at the same time.

http://codereview.chromium.org/10151017/diff/11012/chrome/common/metrics/expe...
File chrome/common/metrics/experiments_helper_unittest.cc (right):

http://codereview.chromium.org/10151017/diff/11012/chrome/common/metrics/expe...
chrome/common/metrics/experiments_helper_unittest.cc:105:
expected_groups.erase(it);
Oops, maybe it's not clear, but there are actually two containers here... a
vector of produced NameGroupIds, which we iterate over, and a set of expected
NameGroupIds, which we remove from to verify that we see all expected
NameGroupIds (the set is expected to be empty if we successfully see every
NameGroupId, which we check below).

So basically the iterator we erase isn't the same one we're looping over.

Does this make sense, or is this pattern a bit too convoluted and you'd rather I
use your trick?

[jar (doing other things), 2012-04-26 22:24:22]

The cleaning up of field_trial.* is looking pretty good.  I had a bunch of nits,
one of which might lead to changing the typedef name you used in field_trial.*

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

http://codereview.chromium.org/10151017/diff/11012/base/metrics/field_trial.h...
base/metrics/field_trial.h:329: // Returns an array of SelectedGroup objects,
each representing a chosen Field
nit: (even in pre-existing comentary): It doesn't really "return" an array (or
even a vector), but only fills in the given data structure... so better may be:

Fills in the supplied vector |selected_groups| (which must be empty when called)
with a snapshot of all existing FieldTrials for which a group has been chosen
(if the group is not yet known, then it excluded from the vector).

http://codereview.chromium.org/10151017/diff/22001/chrome/common/metrics/expe...
File chrome/common/metrics/experiments_helper.cc (right):

http://codereview.chromium.org/10151017/diff/22001/chrome/common/metrics/expe...
chrome/common/metrics/experiments_helper.cc:75: const std::string& group_name) {
nit: This would probably be cleaner taking a:
const& SelectGroup

Usually, it is nicest to keep such pairs of items (name/group) in an object as
long as possible.  I think your original code had it more along those lines.

http://codereview.chromium.org/10151017/diff/22001/chrome/common/metrics/expe...
File chrome/common/metrics/experiments_helper.h (right):

http://codereview.chromium.org/10151017/diff/22001/chrome/common/metrics/expe...
chrome/common/metrics/experiments_helper.h:50: struct NameGroupId {
nit: I found this name hard to guess about.  As it is, SelectedGroup was a
little hard to guess about (it doesn't sound like a "pair" of anything), and now
this is the same data, just hashed, but the struct names of the types are
completely unrelated :-/.

Perhaps SelectedGroup might be GroupInTrial
Then NameGroupId might be HashedGroupInTrial

That is a bit long... but I think the names should be made similar, and easier
to relate to one and other.

Another plausible set of names might be:
SelectedGroup
SelectedGroupHashes

Or maybe even:
SelectedGroup
SelectedGroupIds

Or perhaps:
TrialGroupPair
TrialGroupPairIds

http://codereview.chromium.org/10151017/diff/22001/chrome/common/metrics/expe...
chrome/common/metrics/experiments_helper.h:63: lhs.group < rhs.group;
nit: (You can ignore... as some folks are in love with a ternary operator):  It
is IMO much easier to read as an if statement, where I don't have to hold lots
of context in my head as I read:

if (lsh.group != rhs.group)
  return lhs.group < rhs.group
return lhs.name < rhs.name;

IF you really really want to write it with the ternary operator, it is probably
easier to read when formatted:

return lhs.group == rhs.group ? lhs.name < rhs.name
                              : lhs.group < rhs.group;

or:

return lhs.group != rhs.group ? lhs.group < rhs.group
                              : lhs.name < rhs.name;

...but all of this harks back to my life as an APL programmer.... and I'd rather
see simple easy-to-scan statements ;-).

http://codereview.chromium.org/10151017/diff/22001/chrome/common/metrics/expe...
chrome/common/metrics/experiments_helper.h:73: // Returns an array of Unique IDs
for each Field Trial that has a chosen
nit: "Returns an array..."  ---> "Fills in vector |name_group_ids| ..."

[You'll need some English fixin', but "returns an array" sounds like it
"allocates and returns an array" :-/.

http://codereview.chromium.org/10151017/diff/22001/chrome/common/metrics/expe...
chrome/common/metrics/experiments_helper.h:81: // FieldTrials uninterested in
Google experiments.
I couldn't parse the comment, and understand what it does.
For instance, I wasn't sure what:
"Set the GoogleExperimentID" was meant to do.
I wasn't clear about the personification of "FieldTrials" as being "interested
in Google experiments."

It is probably not appropriate to comment about when a function "does not need
to be called," and better to say when it must be called.  Even then, this might
be a passive voice, and it is not clear who should, or when this should be
called.

http://codereview.chromium.org/10151017/diff/22001/chrome/common/metrics/expe...
File chrome/common/metrics/experiments_helper_unittest.cc (right):

http://codereview.chromium.org/10151017/diff/22001/chrome/common/metrics/expe...
chrome/common/metrics/experiments_helper_unittest.cc:92:
experiments_helper::NameGroupIdCompare> expected_groups;
nit: I hate to see long template types written out... and would much prefer to
see a typedef.  This would also make it easier to write the iterator down around
line 106.

[SteveT, 2012-04-26 23:42:46]

Addressed (most of) Jim's comments - thanks for the quick review. Let me know
what you think now.

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

http://codereview.chromium.org/10151017/diff/11012/base/metrics/field_trial.h...
base/metrics/field_trial.h:329: // Returns an array of SelectedGroup objects,
each representing a chosen Field
Very true. Done.

http://codereview.chromium.org/10151017/diff/22001/chrome/common/metrics/expe...
File chrome/common/metrics/experiments_helper.cc (right):

http://codereview.chromium.org/10151017/diff/22001/chrome/common/metrics/expe...
chrome/common/metrics/experiments_helper.cc:75: const std::string& group_name) {
I'm going to have to push back on this one... This is mostly meant to facilitate
the conversion in AssociateGoogleExperimentID and GetGoogleExperimentID below.
The third place that uses this is GetFieldTrialNameGroupIdsForSelectedGroups
below. Now, I know that you're suggesting this in terms of function signature
cleanliness, but it causes some headaches below.

GetFieldTrialNameGroupIdsForSelectedGroups' impl would be slightly cleaner, but
in Associate/GetGoogleExperimentID, we'd have to create a SelectedGroup both
times and string copy trial_name and group_name into the SelectedGroup. Note
that if we change Associate/Get to also take a SelectedGroup, we'd have to have
every caller of these APIs copy strings into a SelectedGroup.

Let me know if you have any objections to my reasoning for skipping this nit.

http://codereview.chromium.org/10151017/diff/22001/chrome/common/metrics/expe...
File chrome/common/metrics/experiments_helper.h (right):

http://codereview.chromium.org/10151017/diff/22001/chrome/common/metrics/expe...
chrome/common/metrics/experiments_helper.h:50: struct NameGroupId {
I'll take you up on SelectedGroupIds. "Hashes" is more "accurate", bit we do
see/use the hash-pairs as an identifier (when transmitted to our servers).

http://codereview.chromium.org/10151017/diff/22001/chrome/common/metrics/expe...
chrome/common/metrics/experiments_helper.h:63: lhs.group < rhs.group;
I personally find ternaries pretty easy to read*, but I do agree that reading
your suggested code requires slightly less context. Done.

* I generally parse ternary statements in my head as:

[is this true] ? [if so, use this]
               : [otherwise, this]

I find the "?" puts the inflection in just the right place so that it makes
sense in English. But maybe most folks don't read them that way.

http://codereview.chromium.org/10151017/diff/22001/chrome/common/metrics/expe...
chrome/common/metrics/experiments_helper.h:73: // Returns an array of Unique IDs
for each Field Trial that has a chosen
On 2012/04/26 22:24:22, jar wrote:
> nit: "Returns an array..."  ---> "Fills in vector |name_group_ids| ..."
> 
> [You'll need some English fixin', but "returns an array" sounds like it
> "allocates and returns an array" :-/.

Done.

http://codereview.chromium.org/10151017/diff/22001/chrome/common/metrics/expe...
chrome/common/metrics/experiments_helper.h:81: // FieldTrials uninterested in
Google experiments.
Okay, I hope this is a little bit better. We avoid mentioning when it is not
needed.

http://codereview.chromium.org/10151017/diff/22001/chrome/common/metrics/expe...
File chrome/common/metrics/experiments_helper_unittest.cc (right):

http://codereview.chromium.org/10151017/diff/22001/chrome/common/metrics/expe...
chrome/common/metrics/experiments_helper_unittest.cc:92:
experiments_helper::NameGroupIdCompare> expected_groups;
On 2012/04/26 22:24:22, jar wrote:
> nit: I hate to see long template types written out... and would much prefer to
> see a typedef.  This would also make it easier to write the iterator down
around
> line 106.

Done.

[MAD, 2012-04-27 17:33:22]

LGTM!

Thanks...

BYE
MAD...

https://chromiumcodereview.appspot.com/10151017/diff/11012/chrome/common/metr...
File chrome/common/metrics/experiments_helper_unittest.cc (right):

https://chromiumcodereview.appspot.com/10151017/diff/11012/chrome/common/metr...
chrome/common/metrics/experiments_helper_unittest.cc:105:
expected_groups.erase(it);
On 2012/04/26 21:06:03, SteveT wrote:
> Oops, maybe it's not clear, but there are actually two containers here... a
> vector of produced NameGroupIds, which we iterate over, and a set of expected
> NameGroupIds, which we remove from to verify that we see all expected
> NameGroupIds (the set is expected to be empty if we successfully see every
> NameGroupId, which we check below).
> 
> So basically the iterator we erase isn't the same one we're looping over.
> 
> Does this make sense, or is this pattern a bit too convoluted and you'd rather
I
> use your trick?

OK, that's fine... The trick I propose would prevent the need to expose the
compare function publicly, but that's fine... I think it might be more obvious
with a C style array, but that might just be me... So I'm OK with your
solution... Except...

To resolve the potential ambiguity, you could use a more meaningful name for the
set iterator... :-)

Thanks!

[SteveT, 2012-04-27 17:59:33]

Thanks, MAD.

Just waiting on jar@'s approval now.

https://chromiumcodereview.appspot.com/10151017/diff/11012/chrome/common/metr...
File chrome/common/metrics/experiments_helper_unittest.cc (right):

https://chromiumcodereview.appspot.com/10151017/diff/11012/chrome/common/metr...
chrome/common/metrics/experiments_helper_unittest.cc:105:
expected_groups.erase(it);
Good point. I've renamed it to expected_group, which is more clear. Also noticed
that I have to renaming name_group_ids here. Thanks!

[jar (doing other things), 2012-04-27 19:05:44]

...on question to better understand... an and an nit.

http://codereview.chromium.org/10151017/diff/22001/chrome/common/metrics/expe...
File chrome/common/metrics/experiments_helper.cc (right):

http://codereview.chromium.org/10151017/diff/22001/chrome/common/metrics/expe...
chrome/common/metrics/experiments_helper.cc:75: const std::string& group_name) {
Is your concern focused on a performance impact?  Isn't that function rarely
called?  If we were concerned about peroformance, wouldn't we cache the values
(and invalidate when we had any change, which is super rare), and (almost) never
use these calls?




On 2012/04/26 23:42:47, SteveT wrote:
> I'm going to have to push back on this one... This is mostly meant to
facilitate
> the conversion in AssociateGoogleExperimentID and GetGoogleExperimentID below.
> The third place that uses this is GetFieldTrialNameGroupIdsForSelectedGroups
> below. Now, I know that you're suggesting this in terms of function signature
> cleanliness, but it causes some headaches below.
> 
> GetFieldTrialNameGroupIdsForSelectedGroups' impl would be slightly cleaner,
but
> in Associate/GetGoogleExperimentID, we'd have to create a SelectedGroup both
> times and string copy trial_name and group_name into the SelectedGroup. Note
> that if we change Associate/Get to also take a SelectedGroup, we'd have to
have
> every caller of these APIs copy strings into a SelectedGroup.
> 
> Let me know if you have any objections to my reasoning for skipping this nit.

http://codereview.chromium.org/10151017/diff/22001/chrome/common/metrics/expe...
File chrome/common/metrics/experiments_helper.h (right):

http://codereview.chromium.org/10151017/diff/22001/chrome/common/metrics/expe...
chrome/common/metrics/experiments_helper.h:81: // FieldTrials uninterested in
Google experiments.
Perfect! Thanks!

On 2012/04/26 23:42:47, SteveT wrote:
> Okay, I hope this is a little bit better. We avoid mentioning when it is not
> needed.

http://codereview.chromium.org/10151017/diff/45004/chrome/browser/metrics/met...
File chrome/browser/metrics/metrics_log_unittest.cc (right):

http://codereview.chromium.org/10151017/diff/45004/chrome/browser/metrics/met...
chrome/browser/metrics/metrics_log_unittest.cc:17: #include
"chrome/common/metrics/experiments_helper.h"
nit: alphabetize (merge/update messed up your order)

[SteveT, 2012-04-27 19:15:12]

Thanks, Jim. Back to you for additional thoughts.

http://codereview.chromium.org/10151017/diff/22001/chrome/common/metrics/expe...
File chrome/common/metrics/experiments_helper.cc (right):

http://codereview.chromium.org/10151017/diff/22001/chrome/common/metrics/expe...
chrome/common/metrics/experiments_helper.cc:75: const std::string& group_name) {
I'm currently more concerned about code cleanliness in
Assosicate/GetGoogleExperimentID rather than about performance*.

Having to create a new SelectedGroup in those two methods is a  more (and
duplicated) code than keeping the arguments of MakeNameGroupId as-is). If you
feel strongly against this, I can go ahead and make your change.

* Re: Performance: You're right that AssociateGoogleExperimentID is a
seldom-called API. It gets called once per group per FieldTrial. However,
GetGoogleExperimentID might be invoked a lot more. We'd eventually like to fetch
the ID for each registered FieldTrial to send to the server per request to
Google, so the API may be called a lot. However, we'll mitigate that by caching,
like you suggested.

http://codereview.chromium.org/10151017/diff/45004/chrome/browser/metrics/met...
File chrome/browser/metrics/metrics_log_unittest.cc (right):

http://codereview.chromium.org/10151017/diff/45004/chrome/browser/metrics/met...
chrome/browser/metrics/metrics_log_unittest.cc:17: #include
"chrome/common/metrics/experiments_helper.h"
Thanks for the catch. Done.

[jar (doing other things), 2012-04-27 19:31:55]

http://codereview.chromium.org/10151017/diff/22001/chrome/common/metrics/expe...
File chrome/common/metrics/experiments_helper.cc (right):

http://codereview.chromium.org/10151017/diff/22001/chrome/common/metrics/expe...
chrome/common/metrics/experiments_helper.cc:75: const std::string& group_name) {
The win is not that big either way (currently)... and it sounds like you're more
psyched as is....

so...

LGTM.

Thanks for responding!

On 2012/04/27 19:15:13, SteveT wrote:
> I'm currently more concerned about code cleanliness in
> Assosicate/GetGoogleExperimentID rather than about performance*.
> 
> Having to create a new SelectedGroup in those two methods is a  more (and
> duplicated) code than keeping the arguments of MakeNameGroupId as-is). If you
> feel strongly against this, I can go ahead and make your change.
> 
> * Re: Performance: You're right that AssociateGoogleExperimentID is a
> seldom-called API. It gets called once per group per FieldTrial. However,
> GetGoogleExperimentID might be invoked a lot more. We'd eventually like to
fetch
> the ID for each registered FieldTrial to send to the server per request to
> Google, so the API may be called a lot. However, we'll mitigate that by
caching,
> like you suggested.

[SteveT, 2012-04-27 19:34:56]

Thanks all, for the review. I'll be committing this shortly so we can move on
with our variation work.

[commit-bot: I haz the power, 2012-04-27 19:36:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevet@chromium.org/10151017/51001

[commit-bot: I haz the power, 2012-04-27 21:11:06]

Change committed as 134350