Add a switch for faking channels for Variations filtering.

Add a switch for faking channels for Variations filtering.

BUG=162417
TEST=Run Chromium (unofficial build) with --fake-variations-channel=stable and ensure that a stable experiment, such as "UMA-Dynamic-Binary-Uniformity-Trial" shows up in about:version (after restarting once).


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

[SteveT, 2013-01-03 22:02:58]

PTAL.

Let me know if you can think of a better way of reusing an existing
channel-switch instead of the new one I created (which switches on strings,
which we haven't done before).

[jwd, 2013-01-04 15:37:53]

This will work in official builds too right? Do we want to be able to fake a
channel in this situation? What about only honoring the switch when the real
channel is unknown?

On 2013/01/03 22:02:58, SteveT wrote:
> PTAL.
> 
> Let me know if you can think of a better way of reusing an existing
> channel-switch instead of the new one I created (which switches on strings,
> which we haven't done before).

[SteveT, 2013-01-04 16:12:21]

I like the idea of only overring the channel if it is UNKNOWN. PTAL at my
changes.

[SteveT, 2013-01-04 16:12:56]

Oh, and this thing is blocked on https://codereview.chromium.org/11753014/,
which I sort of need to test this.

[jwd, 2013-01-04 16:19:07]

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

https://codereview.chromium.org/11737025/diff/4001/chrome/common/chrome_switc...
chrome/common/chrome_switches.cc:684: // and "canary".
What about a comment here about it only applying when the real channel is
unknown?

[SteveT, 2013-01-04 16:27:07]

PTAL.

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

https://codereview.chromium.org/11737025/diff/4001/chrome/common/chrome_switc...
chrome/common/chrome_switches.cc:684: // and "canary".
On 2013/01/04 16:19:07, Jesse Doherty wrote:
> What about a comment here about it only applying when the real channel is
> unknown?

Done.

[jwd, 2013-01-04 16:31:06]

LGTM

Still need a committer LGTM though.

[SteveT, 2013-01-04 16:46:24]

R+asvitkine for a committer approval

[Alexei Svitkine (slow), 2013-01-04 19:56:34]

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

https://codereview.chromium.org/11737025/diff/9001/chrome/browser/metrics/var...
chrome/browser/metrics/variations/variations_service.cc:280:
chrome::VersionInfo::Channel VariationsService::GetChannelForVariations() {
Stick this in an anon namespace, since this isn't being used by tests. (The
other static functions are.)

Alternatively, add a test.

https://codereview.chromium.org/11737025/diff/9001/chrome/browser/metrics/var...
chrome/browser/metrics/variations/variations_service.cc:287: if
(!forced_channel.empty()) {
Why this check?

https://codereview.chromium.org/11737025/diff/9001/chrome/browser/metrics/var...
chrome/browser/metrics/variations/variations_service.cc:297: NOTREACHED();
NOTREACHED() should be used for cases that shouldn't ever happen. In this case,
it would happen if someone specified some other string on the command-line, so
instead handle the case.

[SteveT, 2013-01-04 20:09:47]

Back to you.

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

https://codereview.chromium.org/11737025/diff/9001/chrome/browser/metrics/var...
chrome/browser/metrics/variations/variations_service.cc:280:
chrome::VersionInfo::Channel VariationsService::GetChannelForVariations() {
Moved.

https://codereview.chromium.org/11737025/diff/9001/chrome/browser/metrics/var...
chrome/browser/metrics/variations/variations_service.cc:287: if
(!forced_channel.empty()) {
We won't need this anymore with the logging below. It was just to avoid hitting
the NOTREACHED when the switch is not provided.

https://codereview.chromium.org/11737025/diff/9001/chrome/browser/metrics/var...
chrome/browser/metrics/variations/variations_service.cc:297: NOTREACHED();
Yeah you're right. I've DVLOGged instead, which I think is appropriate since
someone using this to test with Debug builds would want to see this output.

[Alexei Svitkine (slow), 2013-01-04 20:17:34]

Thanks. Just one more comment.

https://codereview.chromium.org/11737025/diff/18001/chrome/browser/metrics/va...
File chrome/browser/metrics/variations/variations_service.cc (left):

https://codereview.chromium.org/11737025/diff/18001/chrome/browser/metrics/va...
chrome/browser/metrics/variations/variations_service.cc:285: if
(!CheckStudyChannel(study.filter(), chrome::VersionInfo::GetChannel())) {
VariationsService::CreateTrialsFromSeed() calls this for each study in the seed.
I suggest making the channel a param to ShouldAddStudy() to avoid doing the
command-line parsing for every study.

Alternatively, we could do the command-line parsing only once and store the
resulting chrome::VersionInfo::Channel as a field on the variations_service
(though, that will still need to be passed as a param, since this is a static
function).

[SteveT, 2013-01-04 20:28:45]

Great suggestion. Done. PTAL.

[Alexei Svitkine (slow), 2013-01-04 20:32:16]

LGTM with a nit.

https://codereview.chromium.org/11737025/diff/21001/chrome/browser/metrics/va...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/11737025/diff/21001/chrome/browser/metrics/va...
chrome/browser/metrics/variations/variations_service.cc:168:
CreateTrialFromStudy(seed.study(i), reference_date);
Nit: Align |channel| with seed.study(i) and add {}'s.

[SteveT, 2013-01-07 15:43:05]

Okay, thanks for the review! Will commit once I get the blocking patch in, which
will let me properly test this.

https://codereview.chromium.org/11737025/diff/21001/chrome/browser/metrics/va...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/11737025/diff/21001/chrome/browser/metrics/va...
chrome/browser/metrics/variations/variations_service.cc:168:
CreateTrialFromStudy(seed.study(i), reference_date);
I had a feeling this curly issue would come up. I thought you preferred avoiding
{}s in the case where the statement is one line, but the if condition spans
multiple?

Fixed based on this comment.

[Alexei Svitkine (slow), 2013-01-07 15:51:43]

https://codereview.chromium.org/11737025/diff/21001/chrome/browser/metrics/va...
File chrome/browser/metrics/variations/variations_service.cc (right):

https://codereview.chromium.org/11737025/diff/21001/chrome/browser/metrics/va...
chrome/browser/metrics/variations/variations_service.cc:168:
CreateTrialFromStudy(seed.study(i), reference_date);
On 2013/01/07 15:43:05, SteveT wrote:
> I had a feeling this curly issue would come up. I thought you preferred
avoiding
> {}s in the case where the statement is one line, but the if condition spans
> multiple?
> 
> Fixed based on this comment.

The rule I follow is if either if statement or body spans more than one line,
add them.

[SteveT, 2013-01-07 19:04:58]

Okay cool - I think it was someone else who had a preference for no braces when
the condition is 2+ lines.

Manual test look good! Submitting.

[commit-bot: I haz the power, 2013-01-07 19:06:58]

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

[commit-bot: I haz the power, 2013-01-07 20:52:13]

Retried try job too often on win_aura for step(s) interactive_ui_tests

[commit-bot: I haz the power, 2013-01-07 21:42:32]

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

[commit-bot: I haz the power, 2013-01-08 01:04:43]

Change committed as 175437