Enable the VariationsService on official builds when --variations-service-url is provided.

Enable the VariationsService on official builds when --variations-service-url is provided.

We do this by wrapping the default ctor in a factory Create method, which handles the build-type logic.

Also, group static methods in variations_service.*.
Also, move the testing constructor to protected where it's better... protected.

BUG=162417
TEST=With an unofficial build: Ensure that running Chromium without a command line argument does not result in a request to the Variations server (https://clients4.google.com/chrome-variations/seed). Ensure that providing the --variations-server-url flag with a URL results in a request to that URL.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175352

[SteveT, 2013-01-03 20:01:07]

PTAL at this solution.

I will commit after review and some manual testing, which I haven't done yet due
to poor remote desktop access.

[Alexei Svitkine (slow), 2013-01-03 20:14:06]

https://codereview.chromium.org/11753014/diff/2001/chrome/browser/metrics/var...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/11753014/diff/2001/chrome/browser/metrics/var...
chrome/browser/metrics/variations/variations_service.cc:175: #if
defined(GOOGLE_CHROME_BUILD) && !defined(OS_ANDROID)
Do we have a bug tracking the !ANDROID part? If so, we should put a link to it
here, in a comment.

https://codereview.chromium.org/11753014/diff/2001/chrome/browser/metrics/var...
chrome/browser/metrics/variations/variations_service.cc:176: // Unless the URL
was provided, unsupported builds should return NULL to
I think "unofficial" is the correct term.

https://codereview.chromium.org/11753014/diff/2001/chrome/browser/metrics/var...
File chrome/browser/metrics/variations/variations_service.h (right):

https://codereview.chromium.org/11753014/diff/2001/chrome/browser/metrics/var...
chrome/browser/metrics/variations/variations_service.h:47: void
SetCreateTrialsFromSeedCalledForTesting(bool called);
Are you using this somewhere?

https://codereview.chromium.org/11753014/diff/2001/chrome/common/chrome_switc...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/11753014/diff/2001/chrome/common/chrome_switc...
chrome/common/chrome_switches.cc:1338: // client. Specifying this flag also
forces a Variations seed request to the
I would just say "Specifying this flag enables the variations service on
unofficial builds." and leave the details to comments in the variations service
files.

[SteveT, 2013-01-03 20:35:42]

Back to you.

https://codereview.chromium.org/11753014/diff/2001/chrome/browser/metrics/var...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/11753014/diff/2001/chrome/browser/metrics/var...
chrome/browser/metrics/variations/variations_service.cc:175: #if
defined(GOOGLE_CHROME_BUILD) && !defined(OS_ANDROID)
Done.

https://codereview.chromium.org/11753014/diff/2001/chrome/browser/metrics/var...
chrome/browser/metrics/variations/variations_service.cc:176: // Unless the URL
was provided, unsupported builds should return NULL to
So I originally used unofficial, but I changed the wording to "supported" since
the condition covers official & !android. Think I should change it back?

https://codereview.chromium.org/11753014/diff/2001/chrome/browser/metrics/var...
File chrome/browser/metrics/variations/variations_service.h (right):

https://codereview.chromium.org/11753014/diff/2001/chrome/browser/metrics/var...
chrome/browser/metrics/variations/variations_service.h:47: void
SetCreateTrialsFromSeedCalledForTesting(bool called);
I believe this is used by the tests. It was just moved around in this CL so
static methods are better grouped.

https://codereview.chromium.org/11753014/diff/2001/chrome/common/chrome_switc...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/11753014/diff/2001/chrome/common/chrome_switc...
chrome/common/chrome_switches.cc:1338: // client. Specifying this flag also
forces a Variations seed request to the
On 2013/01/03 20:14:06, Alexei Svitkine wrote:
> I would just say "Specifying this flag enables the variations service on
> unofficial builds." and leave the details to comments in the variations
service
> files.

Done.

[Alexei Svitkine (slow), 2013-01-03 20:40:42]

LGTM with a comment.

https://codereview.chromium.org/11753014/diff/2001/chrome/browser/metrics/var...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/11753014/diff/2001/chrome/browser/metrics/var...
chrome/browser/metrics/variations/variations_service.cc:176: // Unless the URL
was provided, unsupported builds should return NULL to
On 2013/01/03 20:35:42, SteveT wrote:
> So I originally used unofficial, but I changed the wording to "supported"
since
> the condition covers official & !android. Think I should change it back?

Ah, makes sense. OK.

https://codereview.chromium.org/11753014/diff/2001/chrome/browser/metrics/var...
File chrome/browser/metrics/variations/variations_service.h (right):

https://codereview.chromium.org/11753014/diff/2001/chrome/browser/metrics/var...
chrome/browser/metrics/variations/variations_service.h:47: void
SetCreateTrialsFromSeedCalledForTesting(bool called);
On 2013/01/03 20:35:42, SteveT wrote:
> I believe this is used by the tests. It was just moved around in this CL so
> static methods are better grouped.

Ah, I misread the diff before. SGTM.

https://codereview.chromium.org/11753014/diff/9001/chrome/browser/metrics/var...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/11753014/diff/9001/chrome/browser/metrics/var...
chrome/browser/metrics/variations/variations_service.cc:175: // See
http://crbug.com/168224
Indent 2. Make the comment mention android, otherwise its not clear what its
referring to.

[SteveT, 2013-01-03 20:46:38]

Okay, comment updated.

Will do the manual testing shortly, before committing..

https://codereview.chromium.org/11753014/diff/9001/chrome/browser/metrics/var...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/11753014/diff/9001/chrome/browser/metrics/var...
chrome/browser/metrics/variations/variations_service.cc:175: // See
http://crbug.com/168224
On 2013/01/03 20:40:42, Alexei Svitkine wrote:
> Indent 2. Make the comment mention android, otherwise its not clear what its
> referring to.

Done.

[SteveT, 2013-01-03 21:09:29]

Testing looks good. Committing presently.

[commit-bot: I haz the power, 2013-01-03 21:09:45]

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

[commit-bot: I haz the power, 2013-01-03 21:09:52]

Presubmit check for 11753014-15001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 4.3s to calculate.

[SteveT, 2013-01-03 21:12:28]

I made a terrible mistake: I actually had to flip the ifdef condition to get the
right behaviour (we missed a NOT). Tests look good after that. PTAL and let me
know if you'd rather the condition was specified a different way.

Also, R+sky for OWNERS approval.

[SteveT, 2013-01-07 15:57:30]

Pinging this so hopefully Scott sees it when he gets in.  :)

[sky, 2013-01-07 16:00:38]

LGTM

[commit-bot: I haz the power, 2013-01-07 16:03:11]

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

[commit-bot: I haz the power, 2013-01-07 17:30:25]

Change committed as 175352