Move Field Trial Utils from chrome/common/variations to components/variations/field_trial_util

Move Field Trial Utils from chrome/common/variations to components/variations/field_trial_config

This allows iOS to use fieldtrial_testing_config.json for their
waterfall.

BUG=660554
TBR=rch as this is moving an existing use of net/base.

Committed: https://crrev.com/48f456a2e965153e5fc85dca7bff30ae972d5be5
Cr-Commit-Position: refs/heads/master@{#429935}

[robliao, 2016-10-31 17:05:20]

Description was changed from

==========
Move Fieldtrial Utils from chrome/common/variations to components/variations

This allows iOS to use fieldtrial_testing_config.json for their
waterfall.

BUG=660554
==========

to

==========
Move Fieldtrial Utils from chrome/common/variations to components/variations

This allows iOS to use fieldtrial_testing_config.json for their
waterfall.

BUG=660554
==========

[robliao, 2016-10-31 17:05:20]

robliao@chromium.org changed reviewers:
+ asvitkine@chromium.org

[robliao, 2016-10-31 17:06:01]

asvitkine: Please review this CL. If there's a more appropriate home for the
fieldtrial utils, let me know and I'll move them. Thanks!

[robliao, 2016-10-31 17:06:08]

The CQ bit was checked by robliao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-31 17:06:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Alexei Svitkine (slow), 2016-10-31 17:22:38]

Thanks for working on this!

https://codereview.chromium.org/2465953002/diff/1/components/variations/DEPS
File components/variations/DEPS (right):

https://codereview.chromium.org/2465953002/diff/1/components/variations/DEPS#...
components/variations/DEPS:11: "+net/base",
Why do we need this? I guess for the URL escaping?

I'd rather we don't depend on net/ since the metrics target is used by other
things - such as the ChromeOS daemon and having extra dependencies would bloat
that.

Can the escaping be moving to its own component?

https://codereview.chromium.org/2465953002/diff/1/components/variations/field...
File components/variations/fieldtrial_util_unittest.cc (right):

https://codereview.chromium.org/2465953002/diff/1/components/variations/field...
components/variations/fieldtrial_util_unittest.cc:20: class FieldtrialUtilTest :
public ::testing::Test {
Nit: FieldTrialUtilTest and name the file field_trial_util to match e.g.
base/metrics/field_trial.cc.

Also, I'm not a big fan of the util name. If this is specific to field trial
config, how about field_trial_config_util.cc?

[robliao, 2016-10-31 17:26:25]

https://codereview.chromium.org/2465953002/diff/1/components/variations/DEPS
File components/variations/DEPS (right):

https://codereview.chromium.org/2465953002/diff/1/components/variations/DEPS#...
components/variations/DEPS:11: "+net/base",
On 2016/10/31 17:22:38, Alexei Svitkine (slow) wrote:
> Why do we need this? I guess for the URL escaping?
> 
> I'd rather we don't depend on net/ since the metrics target is used by other
> things - such as the ChromeOS daemon and having extra dependencies would bloat
> that.
> 
> Can the escaping be moving to its own component?

Yes, this is specifically for URL escaping. We could use a separate component
for this, but I couldn't come up with an appropriate name and
components/variations seemed like an appropriate home.

Got a name? components/fieldtrial_utils or something?

https://codereview.chromium.org/2465953002/diff/1/components/variations/field...
File components/variations/fieldtrial_util_unittest.cc (right):

https://codereview.chromium.org/2465953002/diff/1/components/variations/field...
components/variations/fieldtrial_util_unittest.cc:20: class FieldtrialUtilTest :
public ::testing::Test {
On 2016/10/31 17:22:38, Alexei Svitkine (slow) wrote:
> Nit: FieldTrialUtilTest and name the file field_trial_util to match e.g.
> base/metrics/field_trial.cc.
> 
> Also, I'm not a big fan of the util name. If this is specific to field trial
> config, how about field_trial_config_util.cc?

Works for me. I was just going off of the fieldtrial_testing_config pattern.
Will do after we resolve the location.

[Alexei Svitkine (slow), 2016-10-31 17:29:19]

https://codereview.chromium.org/2465953002/diff/1/components/variations/DEPS
File components/variations/DEPS (right):

https://codereview.chromium.org/2465953002/diff/1/components/variations/DEPS#...
components/variations/DEPS:11: "+net/base",
On 2016/10/31 17:26:25, robliao wrote:
> On 2016/10/31 17:22:38, Alexei Svitkine (slow) wrote:
> > Why do we need this? I guess for the URL escaping?
> > 
> > I'd rather we don't depend on net/ since the metrics target is used by other
> > things - such as the ChromeOS daemon and having extra dependencies would
bloat
> > that.
> > 
> > Can the escaping be moving to its own component?
> 
> Yes, this is specifically for URL escaping. We could use a separate component
> for this, but I couldn't come up with an appropriate name and
> components/variations seemed like an appropriate home.
> 
> Got a name? components/fieldtrial_utils or something?

I was actually thinking the escaping from net/ could be it's own component. e.g.
components/url_escape

Alternatively, this can be a sub-directory inside components/metrics - like we
have service/ already which does depend on net. 

For example, components/metrics/field_trial_configs/

[robliao, 2016-10-31 17:35:12]

https://codereview.chromium.org/2465953002/diff/1/components/variations/DEPS
File components/variations/DEPS (right):

https://codereview.chromium.org/2465953002/diff/1/components/variations/DEPS#...
components/variations/DEPS:11: "+net/base",
On 2016/10/31 17:29:18, Alexei Svitkine (slow) wrote:
> On 2016/10/31 17:26:25, robliao wrote:
> > On 2016/10/31 17:22:38, Alexei Svitkine (slow) wrote:
> > > Why do we need this? I guess for the URL escaping?
> > > 
> > > I'd rather we don't depend on net/ since the metrics target is used by
other
> > > things - such as the ChromeOS daemon and having extra dependencies would
> bloat
> > > that.
> > > 
> > > Can the escaping be moving to its own component?
> > 
> > Yes, this is specifically for URL escaping. We could use a separate
component
> > for this, but I couldn't come up with an appropriate name and
> > components/variations seemed like an appropriate home.
> > 
> > Got a name? components/fieldtrial_utils or something?
> 
> I was actually thinking the escaping from net/ could be it's own component.
e.g.
> components/url_escape
> 
> Alternatively, this can be a sub-directory inside components/metrics - like we
> have service/ already which does depend on net. 
> 
> For example, components/metrics/field_trial_configs/

components/metrics/field_trial_configs sgtm.

Re: Refactoring URL Escaping
That will certainly be a more invasive change. (On the order of 40+ sites need
to be updated). 

+net/base seems to be the way to specifically depend on things in net/base
https://cs.chromium.org/search/?q=%5C%2Bnet/base+file:components&sq=package:c...

I suspect all of those components just rely on dead code elimination to remove
the unused parts of net from their components. What do you think?

[commit-bot: I haz the power, 2016-10-31 17:44:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-31 17:44:41]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[robliao, 2016-11-01 00:05:27]

The CQ bit was checked by robliao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-01 00:05:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-01 01:23:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-01 01:23:26]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2016-11-01 14:48:15]

On 2016/10/31 17:35:12, robliao wrote:
> https://codereview.chromium.org/2465953002/diff/1/components/variations/DEPS
> File components/variations/DEPS (right):
> 
>
https://codereview.chromium.org/2465953002/diff/1/components/variations/DEPS#...
> components/variations/DEPS:11: "+net/base",
> On 2016/10/31 17:29:18, Alexei Svitkine (slow) wrote:
> > On 2016/10/31 17:26:25, robliao wrote:
> > > On 2016/10/31 17:22:38, Alexei Svitkine (slow) wrote:
> > > > Why do we need this? I guess for the URL escaping?
> > > > 
> > > > I'd rather we don't depend on net/ since the metrics target is used by
> other
> > > > things - such as the ChromeOS daemon and having extra dependencies would
> > bloat
> > > > that.
> > > > 
> > > > Can the escaping be moving to its own component?
> > > 
> > > Yes, this is specifically for URL escaping. We could use a separate
> component
> > > for this, but I couldn't come up with an appropriate name and
> > > components/variations seemed like an appropriate home.
> > > 
> > > Got a name? components/fieldtrial_utils or something?
> > 
> > I was actually thinking the escaping from net/ could be it's own component.
> e.g.
> > components/url_escape
> > 
> > Alternatively, this can be a sub-directory inside components/metrics - like
we
> > have service/ already which does depend on net. 
> > 
> > For example, components/metrics/field_trial_configs/
> 
> components/metrics/field_trial_configs sgtm.
> 
> Re: Refactoring URL Escaping
> That will certainly be a more invasive change. (On the order of 40+ sites need
> to be updated). 
> 
> +net/base seems to be the way to specifically depend on things in net/base
>
https://cs.chromium.org/search/?q=%5C%2Bnet/base+file:components&sq=package:c...
> 
> I suspect all of those components just rely on dead code elimination to remove
> the unused parts of net from their components. What do you think?

Right. Well, let's go with components/metrics/field_trial_configs. If it's a
separate target, I'm fine with it depending on net/base - as long as the core
component doesn't. Thanks!

[robliao, 2016-11-01 21:50:42]

Description was changed from

==========
Move Fieldtrial Utils from chrome/common/variations to components/variations

This allows iOS to use fieldtrial_testing_config.json for their
waterfall.

BUG=660554
==========

to

==========
Move Fieldtrial Utils from chrome/common/variations to
components/metrics/field_trial_util

This allows iOS to use fieldtrial_testing_config.json for their
waterfall.

BUG=660554
==========

[robliao, 2016-11-01 23:48:15]

Patchset #2 (id:20001) has been deleted

[robliao, 2016-11-01 23:57:00]

Patchset #2 (id:40001) has been deleted

[robliao, 2016-11-02 00:00:28]

Now moved Fieldtrial Utils from chrome/common/variations to
components/metrics/field_trial_util

https://codereview.chromium.org/2465953002/diff/1/components/variations/DEPS
File components/variations/DEPS (right):

https://codereview.chromium.org/2465953002/diff/1/components/variations/DEPS#...
components/variations/DEPS:11: "+net/base",
On 2016/10/31 17:35:12, robliao wrote:
> On 2016/10/31 17:29:18, Alexei Svitkine (slow) wrote:
> > On 2016/10/31 17:26:25, robliao wrote:
> > > On 2016/10/31 17:22:38, Alexei Svitkine (slow) wrote:
> > > > Why do we need this? I guess for the URL escaping?
> > > > 
> > > > I'd rather we don't depend on net/ since the metrics target is used by
> other
> > > > things - such as the ChromeOS daemon and having extra dependencies would
> > bloat
> > > > that.
> > > > 
> > > > Can the escaping be moving to its own component?
> > > 
> > > Yes, this is specifically for URL escaping. We could use a separate
> component
> > > for this, but I couldn't come up with an appropriate name and
> > > components/variations seemed like an appropriate home.
> > > 
> > > Got a name? components/fieldtrial_utils or something?
> > 
> > I was actually thinking the escaping from net/ could be it's own component.
> e.g.
> > components/url_escape
> > 
> > Alternatively, this can be a sub-directory inside components/metrics - like
we
> > have service/ already which does depend on net. 
> > 
> > For example, components/metrics/field_trial_configs/
> 
> components/metrics/field_trial_configs sgtm.
> 
> Re: Refactoring URL Escaping
> That will certainly be a more invasive change. (On the order of 40+ sites need
> to be updated). 
> 
> +net/base seems to be the way to specifically depend on things in net/base
>
https://cs.chromium.org/search/?q=%5C%2Bnet/base+file:components&sq=package:c...
> 
> I suspect all of those components just rely on dead code elimination to remove
> the unused parts of net from their components. What do you think?

Done.

https://codereview.chromium.org/2465953002/diff/1/components/variations/field...
File components/variations/fieldtrial_util_unittest.cc (right):

https://codereview.chromium.org/2465953002/diff/1/components/variations/field...
components/variations/fieldtrial_util_unittest.cc:20: class FieldtrialUtilTest :
public ::testing::Test {
On 2016/10/31 17:26:25, robliao wrote:
> On 2016/10/31 17:22:38, Alexei Svitkine (slow) wrote:
> > Nit: FieldTrialUtilTest and name the file field_trial_util to match e.g.
> > base/metrics/field_trial.cc.
> > 
> > Also, I'm not a big fan of the util name. If this is specific to field trial
> > config, how about field_trial_config_util.cc?
> 
> Works for me. I was just going off of the fieldtrial_testing_config pattern.
> Will do after we resolve the location.

Done.

[robliao, 2016-11-02 00:01:12]

The CQ bit was checked by robliao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 00:02:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[robliao, 2016-11-02 00:11:38]

Patchset #2 (id:60001) has been deleted

[robliao, 2016-11-02 00:11:55]

The CQ bit was checked by robliao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 00:12:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[robliao, 2016-11-02 01:03:02]

Description was changed from

==========
Move Fieldtrial Utils from chrome/common/variations to
components/metrics/field_trial_util

This allows iOS to use fieldtrial_testing_config.json for their
waterfall.

BUG=660554
==========

to

==========
Move Field Trial Utils from chrome/common/variations to
components/metrics/field_trial_util

This allows iOS to use fieldtrial_testing_config.json for their
waterfall.

BUG=660554
==========

[robliao, 2016-11-02 17:01:45]

Patchset #2 (id:80001) has been deleted

[robliao, 2016-11-02 17:01:51]

The CQ bit was checked by robliao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 17:02:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Alexei Svitkine (slow), 2016-11-02 17:44:49]

Sorry, I meant components/variations/field_trial_config rather than
components/metrics/field_trial_config. Apologies for making you do the extra
move to the wrong place. :(

[robliao, 2016-11-02 18:18:54]

Description was changed from

==========
Move Field Trial Utils from chrome/common/variations to
components/metrics/field_trial_util

This allows iOS to use fieldtrial_testing_config.json for their
waterfall.

BUG=660554
==========

to

==========
Move Field Trial Utils from chrome/common/variations to
components/variations/field_trial_util

This allows iOS to use fieldtrial_testing_config.json for their
waterfall.

BUG=660554
==========

[robliao, 2016-11-02 18:21:53]

On 2016/11/02 17:44:49, Alexei Svitkine (slow) wrote:
> Sorry, I meant components/variations/field_trial_config rather than
> components/metrics/field_trial_config. Apologies for making you do the extra
> move to the wrong place. :(

No worries. Move to components/variations/field_trial_config done.

[robliao, 2016-11-02 18:22:02]

The CQ bit was checked by robliao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 18:22:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Alexei Svitkine (slow), 2016-11-02 18:24:21]

LGTM - thanks!

[Alexei Svitkine (slow), 2016-11-02 18:24:38]

Description was changed from

==========
Move Field Trial Utils from chrome/common/variations to
components/variations/field_trial_util

This allows iOS to use fieldtrial_testing_config.json for their
waterfall.

BUG=660554
==========

to

==========
Move Field Trial Utils from chrome/common/variations to
components/variations/field_trial_config

This allows iOS to use fieldtrial_testing_config.json for their
waterfall.

BUG=660554
==========

[robliao, 2016-11-02 18:32:31]

robliao@chromium.org changed reviewers:
+ brettw@chromium.org

[robliao, 2016-11-02 18:32:32]

brettw@chromium.org: Please review the refactoring changes in chrome/*. Thanks!

[commit-bot: I haz the power, 2016-11-02 19:37:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-02 19:37:03]

Dry run: This issue passed the CQ dry run.

[robliao, 2016-11-02 20:39:26]

robliao@chromium.org changed reviewers:
+ rch@chromium.org

[robliao, 2016-11-02 20:39:27]

rch: Please review the +net/base in 
components/variations/field_trial_config/DEPS.

The specific call is net::UnescapeURLComponent taken from chrome/common and
moved to components/variations/field_trial_config. Thanks!

[robliao, 2016-11-03 18:27:36]

The CQ bit was checked by robliao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 18:28:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[brettw, 2016-11-03 20:18:24]

lgtm

[robliao, 2016-11-03 20:32:27]

Description was changed from

==========
Move Field Trial Utils from chrome/common/variations to
components/variations/field_trial_config

This allows iOS to use fieldtrial_testing_config.json for their
waterfall.

BUG=660554
==========

to

==========
Move Field Trial Utils from chrome/common/variations to
components/variations/field_trial_config

This allows iOS to use fieldtrial_testing_config.json for their
waterfall.

BUG=660554

TBR=rch as this is moving an existing use of net/base.
==========

[robliao, 2016-11-03 20:32:41]

The CQ bit was unchecked by robliao@chromium.org

[robliao, 2016-11-03 20:32:46]

The CQ bit was checked by robliao@chromium.org

[commit-bot: I haz the power, 2016-11-03 20:33:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-03 22:19:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-03 22:19:07]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[robliao, 2016-11-03 22:52:10]

The CQ bit was checked by robliao@chromium.org

[commit-bot: I haz the power, 2016-11-03 22:53:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-04 01:13:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-04 01:13:23]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[robliao, 2016-11-04 16:45:43]

The CQ bit was checked by robliao@chromium.org

[commit-bot: I haz the power, 2016-11-04 16:46:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-04 17:35:32]

Description was changed from

==========
Move Field Trial Utils from chrome/common/variations to
components/variations/field_trial_config

This allows iOS to use fieldtrial_testing_config.json for their
waterfall.

BUG=660554

TBR=rch as this is moving an existing use of net/base.
==========

to

==========
Move Field Trial Utils from chrome/common/variations to
components/variations/field_trial_config

This allows iOS to use fieldtrial_testing_config.json for their
waterfall.

BUG=660554

TBR=rch as this is moving an existing use of net/base.
==========

[commit-bot: I haz the power, 2016-11-04 17:35:37]

Committed patchset #3 (id:120001)

[commit-bot: I haz the power, 2016-11-04 17:47:20]

Description was changed from

==========
Move Field Trial Utils from chrome/common/variations to
components/variations/field_trial_config

This allows iOS to use fieldtrial_testing_config.json for their
waterfall.

BUG=660554

TBR=rch as this is moving an existing use of net/base.
==========

to

==========
Move Field Trial Utils from chrome/common/variations to
components/variations/field_trial_config

This allows iOS to use fieldtrial_testing_config.json for their
waterfall.

BUG=660554

TBR=rch as this is moving an existing use of net/base.

Committed: https://crrev.com/48f456a2e965153e5fc85dca7bff30ae972d5be5
Cr-Commit-Position: refs/heads/master@{#429935}
==========

[commit-bot: I haz the power, 2016-11-04 17:47:22]

Patchset 3 (id:??) landed as
https://crrev.com/48f456a2e965153e5fc85dca7bff30ae972d5be5
Cr-Commit-Position: refs/heads/master@{#429935}