Add and implement API to associate Google experiment IDs with FieldTrials.

Add and implement API to associate Google experiment IDs with FieldTrials.

This includes unit tests for the API, but no uses yet.

This change also involves exposing a new MakeGroupNameId static helper in FieldTrial to assist with the usage of this new API.

BUG=121988
TEST=Ensure that unit_tests GoogleExperimentsTest.* pass.


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

[SteveT, 2012-04-20 21:05:03]

This code is the same as http://codereview.chromium.org/9965086/, which was
reverted due to a leak. The leak is now resolved (see delta).

MAD: PTAL at the changes to experiments_helper_unittest. Inspired from
field_trial_unittest.
Will: Rubber-stamp approval for base/ OWNERS, given I haven't touched any of
base/ at all since 9965086.

[willchan no longer on Chromium, 2012-04-20 21:07:56]

base/ lgtm

On 2012/04/20 21:05:03, SteveT wrote:
> This code is the same as http://codereview.chromium.org/9965086/, which was
> reverted due to a leak. The leak is now resolved (see delta).
> 
> MAD: PTAL at the changes to experiments_helper_unittest. Inspired from
> field_trial_unittest.
> Will: Rubber-stamp approval for base/ OWNERS, given I haven't touched any of
> base/ at all since 9965086.

[MAD, 2012-04-20 21:10:52]

LGTM!

[commit-bot: I haz the power, 2012-04-20 21:11:59]

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

[SteveT, 2012-04-21 00:34:28]

cc+Jim

I've reached a bit of a dilemma here. The unit tests were initially failing due
to memory leaks, because we were operating in ExperimentsHelperTest without a
FieldTrialList, which meant FieldTrials created with FactoryGetFieldTrial
weren't getting AddRef'd/Released (ie - were not owned by anyone and leaked).

The solution I tried was to add a FieldTrialList member to
ExperimentsHelperTest, a la FieldTrialTest, to resolve this. This did not work,
however, as earlier tests that ran in the unit_tests excutable were creating
FieldTrials with no FieldTrialList, which set off used_without_global_. When we
instantiated FieldTrialList in our tests, it would DCHECK (see
http://code.google.com/codesearch#OAMlx_jo-ck/src/base/metrics/field_trial.cc...)

So I got rid of the FieldTrialList and managed the returned FieldTrials myself
using a scoped_ptr, but it turns out FieldTrial's dtor is private, so we can't
use it with the scoped_ptr template class.

So what's the right thing to do here? We could add some functionality to
FieldTrialList for tests were we can reset the used_without_global_ flag, but
this is a pretty weak way to "isolate" tests from affecting each other. Another
solution is to friend scoped_ptr<FieldTrial>... not sure how clean that is.

Thoughts?

[MAD, 2012-04-21 01:02:44]

Can you check how the other tests that get used_without_global_ being set
deal with their FieldTrials memory allocations? Unless they only get to
call "FieldTrialList::IsOneTimeRandomizationEnabled()"?

On Fri, Apr 20, 2012 at 8:34 PM, <stevet@chromium.org> wrote:

> cc+Jim
>
> I've reached a bit of a dilemma here. The unit tests were initially
> failing due
> to memory leaks, because we were operating in ExperimentsHelperTest
> without a
> FieldTrialList, which meant FieldTrials created with FactoryGetFieldTrial
> weren't getting AddRef'd/Released (ie - were not owned by anyone and
> leaked).
>
> The solution I tried was to add a FieldTrialList member to
> ExperimentsHelperTest, a la FieldTrialTest, to resolve this. This did not
> work,
> however, as earlier tests that ran in the unit_tests excutable were
> creating
> FieldTrials with no FieldTrialList, which set off used_without_global_.
> When we
> instantiated FieldTrialList in our tests, it would DCHECK (see
> http://code.google.com/**codesearch#OAMlx_jo-ck/src/**
> base/metrics/field_trial.cc&**exact_package=chromium&q=file:**
>
field_trial&type=cs&l=242<http://code.google.com/codesearch#OAMlx_jo-ck/src/base/metrics/field_trial.cc&exact_package=chromium&q=file:field_trial&type=cs&l=242>
> )
>
> So I got rid of the FieldTrialList and managed the returned FieldTrials
> myself
> using a scoped_ptr, but it turns out FieldTrial's dtor is private, so we
> can't
> use it with the scoped_ptr template class.
>
> So what's the right thing to do here? We could add some functionality to
> FieldTrialList for tests were we can reset the used_without_global_ flag,
> but
> this is a pretty weak way to "isolate" tests from affecting each other.
> Another
> solution is to friend scoped_ptr<FieldTrial>... not sure how clean that is.
>
> Thoughts?
>
>
http://codereview.chromium.**org/10165014/<http://codereview.chromium.org/101...
>

[jar (doing other things), 2012-04-21 15:28:10]

Keeping the dtor private is goodness, but it is common for tests, which operate
against the API in "strange" ways, to need to be friends of classes to handle
such cleanup.

I haven't read the details of this code... but just responding to you question:
Would it be possible to manually handle cleanup (in the extreme, by calling
delete), and declare a FRIEND_TEST?

It also wouldn't surprise me if some elements of the field trial classes *need*
to leak, in the same way as histograms leak, and hence a focused suppression
could be the answer, but I'd rather see a friend test declaration, and/or a
helper (private) method for use by tests only.



On 2012/04/21 01:02:44, MAD wrote:
> Can you check how the other tests that get used_without_global_ being set
> deal with their FieldTrials memory allocations? Unless they only get to
> call "FieldTrialList::IsOneTimeRandomizationEnabled()"?
> 
> On Fri, Apr 20, 2012 at 8:34 PM, <mailto:stevet@chromium.org> wrote:
> 
> > cc+Jim
> >
> > I've reached a bit of a dilemma here. The unit tests were initially
> > failing due
> > to memory leaks, because we were operating in ExperimentsHelperTest
> > without a
> > FieldTrialList, which meant FieldTrials created with FactoryGetFieldTrial
> > weren't getting AddRef'd/Released (ie - were not owned by anyone and
> > leaked).
> >
> > The solution I tried was to add a FieldTrialList member to
> > ExperimentsHelperTest, a la FieldTrialTest, to resolve this. This did not
> > work,
> > however, as earlier tests that ran in the unit_tests excutable were
> > creating
> > FieldTrials with no FieldTrialList, which set off used_without_global_.
> > When we
> > instantiated FieldTrialList in our tests, it would DCHECK (see
> > http://code.google.com/**codesearch#OAMlx_jo-ck/src/**
> > base/metrics/field_trial.cc&**exact_package=chromium&q=file:**
> >
>
field_trial&type=cs&l=242<http://code.google.com/codesearch#OAMlx_jo-ck/src/base/metrics/field_trial.cc&exact_package=chromium&q=file:field_trial&type=cs&l=242>
> > )
> >
> > So I got rid of the FieldTrialList and managed the returned FieldTrials
> > myself
> > using a scoped_ptr, but it turns out FieldTrial's dtor is private, so we
> > can't
> > use it with the scoped_ptr template class.
> >
> > So what's the right thing to do here? We could add some functionality to
> > FieldTrialList for tests were we can reset the used_without_global_ flag,
> > but
> > this is a pretty weak way to "isolate" tests from affecting each other.
> > Another
> > solution is to friend scoped_ptr<FieldTrial>... not sure how clean that is.
> >
> > Thoughts?
> >
> >
>
http://codereview.chromium.**org/10165014/%3Chttp://codereview.chromium.org/1...>
> >

[Steve, 2012-04-21 19:36:37]

On Sat, Apr 21, 2012 at 11:28 AM, <jar@chromium.org> wrote:

> Keeping the dtor private is goodness, but it is common for tests, which
> operate
> against the API in "strange" ways, to need to be friends of classes to
> handle
> such cleanup.
>
> I haven't read the details of this code... but just responding to you
> question:
> Would it be possible to manually handle cleanup (in the extreme, by calling
> delete), and declare a FRIEND_TEST?
>

This could work, but it seems wrong to friend a test that lives in
chrome/common/metrics/ with a class that lives in base/... Do you know if
this is OK, dependency-wise?


>
> It also wouldn't surprise me if some elements of the field trial classes
> *need*
> to leak, in the same way as histograms leak, and hence a focused
> suppression
> could be the answer, but I'd rather see a friend test declaration, and/or a
> helper (private) method for use by tests only.


Not familiar with why Histograms need to leak in tests... Out of curiosity,
could you point me at some code or give a brief description of this
requirement?


>
>
>
>
> On 2012/04/21 01:02:44, MAD wrote:
>
>> Can you check how the other tests that get used_without_global_ being set
>> deal with their FieldTrials memory allocations? Unless they only get to
>> call "FieldTrialList::**IsOneTimeRandomizationEnabled(**)"?
>>
>
They don't really do anything at all. It's actually just one
BrowserMainTest that does it:
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/chrome_brows...

The thing is, their code that receives the FT created by
FactoryGetFieldTrial stores it in a scoped_refptr, which means they get
automatic cleanup, despite not having a FieldTrialList declare.
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/chrome_brows...

This works for them because scoped_refptr happens to be friended with
FieldTrial already. We could probably do the same in our *test* code to
handle the cleanup... I'll try this.


>
>  On Fri, Apr 20, 2012 at 8:34 PM, <mailto:stevet@chromium.org> wrote:
>>
>
>  > cc+Jim
>> >
>> > I've reached a bit of a dilemma here. The unit tests were initially
>> > failing due
>> > to memory leaks, because we were operating in ExperimentsHelperTest
>> > without a
>> > FieldTrialList, which meant FieldTrials created with
>> FactoryGetFieldTrial
>> > weren't getting AddRef'd/Released (ie - were not owned by anyone and
>> > leaked).
>> >
>> > The solution I tried was to add a FieldTrialList member to
>> > ExperimentsHelperTest, a la FieldTrialTest, to resolve this. This did
>> not
>> > work,
>> > however, as earlier tests that ran in the unit_tests excutable were
>> > creating
>> > FieldTrials with no FieldTrialList, which set off used_without_global_.
>> > When we
>> > instantiated FieldTrialList in our tests, it would DCHECK (see
>> >
http://code.google.com/****codesearch#OAMlx_jo-ck/src/**<http://code.google.c...
>> > base/metrics/field_trial.cc&****exact_package=chromium&q=file:****
>> >
>>
>
> field_trial&type=cs&l=242<http**://code.google.com/codesearch#**
> OAMlx_jo-ck/src/base/metrics/**field_trial.cc&exact_package=**
>
chromium&q=file:field_trial&**type=cs&l=242<http://code.google.com/codesearch#OAMlx_jo-ck/src/base/metrics/field_trial.cc&exact_package=chromium&q=file:field_trial&type=cs&l=242>
> >
>
>  > )
>> >
>> > So I got rid of the FieldTrialList and managed the returned FieldTrials
>> > myself
>> > using a scoped_ptr, but it turns out FieldTrial's dtor is private, so we
>> > can't
>> > use it with the scoped_ptr template class.
>> >
>> > So what's the right thing to do here? We could add some functionality to
>> > FieldTrialList for tests were we can reset the used_without_global_
>> flag,
>> > but
>> > this is a pretty weak way to "isolate" tests from affecting each other.
>> > Another
>> > solution is to friend scoped_ptr<FieldTrial>... not sure how clean that
>> is.
>> >
>> > Thoughts?
>> >
>> >
>>
>
> http://codereview.chromium.****org/10165014/%3Chttp://coderev**
> iew.chromium.org/10165014/ <http://codereview.chromium.org/10165014/>>
>
>> >
>>
>
>
>
>
http://codereview.chromium.**org/10165014/<http://codereview.chromium.org/101...
>

[SteveT, 2012-04-21 21:15:26]

scoped_refptr seems to be the way to go. It's a bit better than Jim's suggestion
of manually cleaning up the created FieldTrials. It also doesn't interfere with
other tests. Let me know what you think, MAD.

[MAD, 2012-04-23 14:49:54]

LGTM...

The only last nit I might have would be to use test fixture variables (consts?)
for the expiry date... Which could also be set with now() + 1 year, as in
FieldTrial unit tests...

Thanks!

BYE
MAD

[commit-bot: I haz the power, 2012-04-23 16:08:55]

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

[commit-bot: I haz the power, 2012-04-23 17:41:07]

Change committed as 133470