Support serial number requests in variations service.

Support serial number requests in variations service.

BUG=140073
TEST=manual using inserted logging

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

[SteveT, 2012-08-02 17:31:50]

LGTM.

TEST= is meant to be instructions for QA, not necessarily what you did. Could
you provide some instructions that QA could use to test this?

http://codereview.chromium.org/10828106/diff/4003/chrome/browser/metrics/vari...
File chrome/browser/metrics/variations_service.cc (right):

http://codereview.chromium.org/10828106/diff/4003/chrome/browser/metrics/vari...
chrome/browser/metrics/variations_service.cc:169: variations_serial_number_);
nit: This spans two lines, so I believe we want to wrap it in braces.

[Alexei Svitkine (slow), 2012-08-02 18:37:51]

> TEST= is meant to be instructions for QA, not necessarily what you did. Could
> you provide some instructions that QA could use to test this?

I'm not sure there's any way for QA to test this given their current practices.
(i.e. it's unreasonable to expect them to bring up their own servers and
validate the communicate stream between the two endpoints). QA mostly checks for
visual / user experience issues, not how Chrome internals work.

The TEST= line is highly debated and I'm on the side where it's better to have
it there to document how the change was tested then to omit it in the case where
the change can't be verified by QA.

http://codereview.chromium.org/10828106/diff/4003/chrome/browser/metrics/vari...
File chrome/browser/metrics/variations_service.cc (right):

http://codereview.chromium.org/10828106/diff/4003/chrome/browser/metrics/vari...
chrome/browser/metrics/variations_service.cc:169: variations_serial_number_);
On 2012/08/02 17:31:51, SteveT wrote:
> nit: This spans two lines, so I believe we want to wrap it in braces.

Done.

[commit-bot: I haz the power, 2012-08-02 18:39:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/10828106/2002

[commit-bot: I haz the power, 2012-08-02 18:39:16]

Presubmit check for 10828106-2002 failed and returned exit status 1.

Running presubmit commit checks ...

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

Presubmit checks took 1.1s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Alexei Svitkine (slow), 2012-08-02 18:42:27]

+jar for chrome/browser/metrics/OWNERS

[Ilya Sherman, 2012-08-02 23:40:48]

LGTM, with a couple of questions inline.

https://chromiumcodereview.appspot.com/10828106/diff/2002/chrome/browser/metr...
File chrome/browser/metrics/variations_service.cc (right):

https://chromiumcodereview.appspot.com/10828106/diff/2002/chrome/browser/metr...
chrome/browser/metrics/variations_service.cc:108:
create_trials_from_seed_called_ = true;
nit: Is it ok that this method can return |false| after this line?  Is the
DCHECK below that uses this variable just making sure that we don't lose the
optimization we gain in non-exceptional cases from caching the seed?

https://chromiumcodereview.appspot.com/10828106/diff/2002/chrome/browser/metr...
File chrome/browser/metrics/variations_service.h (right):

https://chromiumcodereview.appspot.com/10828106/diff/2002/chrome/browser/metr...
chrome/browser/metrics/variations_service.h:136: bool
create_trials_from_seed_called_;
nit: This seems a little hacky.  Is there any way to more directly check whether
CreateTrialsFromSeed() initialized whatever data you're expecting to be
initialized?

[Alexei Svitkine (slow), 2012-08-02 23:51:02]

Thanks, Ilya.

I still need an OWNERS approval from chrome/browser/metrics/OWNERS (you're not
listed there).

https://chromiumcodereview.appspot.com/10828106/diff/2002/chrome/browser/metr...
File chrome/browser/metrics/variations_service.cc (right):

https://chromiumcodereview.appspot.com/10828106/diff/2002/chrome/browser/metr...
chrome/browser/metrics/variations_service.cc:108:
create_trials_from_seed_called_ = true;
On 2012/08/02 23:40:49, Ilya Sherman wrote:
> nit: Is it ok that this method can return |false| after this line?  Is the
> DCHECK below that uses this variable just making sure that we don't lose the
> optimization we gain in non-exceptional cases from caching the seed?

Right, it's just making sure we don't lose the optimization. It's not necessary
that |variations_serial_number_| actually got set (and it's valid for it not to
be - e.g. on first run).

https://chromiumcodereview.appspot.com/10828106/diff/2002/chrome/browser/metr...
File chrome/browser/metrics/variations_service.h (right):

https://chromiumcodereview.appspot.com/10828106/diff/2002/chrome/browser/metr...
chrome/browser/metrics/variations_service.h:136: bool
create_trials_from_seed_called_;
On 2012/08/02 23:40:49, Ilya Sherman wrote:
> nit: This seems a little hacky.  Is there any way to more directly check
whether
> CreateTrialsFromSeed() initialized whatever data you're expecting to be
> initialized?

I can't think of a good way - the issue is that it's perfectly valid for
CreateTrialsFromSeed() to return early if there's no seed stored in prefs yet.

[commit-bot: I haz the power, 2012-08-03 20:15:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/10828106/2002

[commit-bot: I haz the power, 2012-08-03 22:42:42]

Try job failure for 10828106-2002 (retry) (retry) on win_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-08-03 22:44:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/10828106/2002

[commit-bot: I haz the power, 2012-08-04 00:02:22]

Change committed as 149979