Support variations_seed parameter in MasterPrefs.

Support variations_seed parameter in MasterPrefs.

Also, cleans up SetupMasterPrefsFromInstallPrefs() to be more conformant
to the style guide and adds a unit test.

BUG=175476
TEST=Test that specifying a custom variations_seed in a master prefs file to
the installer (via --installerdata=path), results in that seed being used on first
run.

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

[Alexei Svitkine (slow), 2013-02-11 22:30:02]

Gab, Steve - please take a look.

I still need to test this manually by passing my own custom variations seed to
an installer (which I'll do before landing), but I believe the code should be
doing the right thing.

[gab, 2013-02-11 23:44:01]

Some questions:

1) Why is this required? Is it to get first run Finch experiments (i.e. without
even having to contact the server first)?

2) Who will be in charge of randomizing this seed? The download server? Omaha?

3) Are you aware that this exposes a backdoor into enabling/disabling specific
Finch experiments (i.e. a user could constantly modify this seed and re-trigger
first run until he gets the desired set of experiments)? (I think one of the
goal of Finch is that it isn't user controllable -- although I guess
uninstalling/reinstalling Chrome or wiping his User Data directory is also a
current backdoor?)

Cheers!
Gab

https://codereview.chromium.org/12223062/diff/9009/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/12223062/diff/9009/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:813: // (i.e., even if --no-first-run is
passed).
ref.

https://codereview.chromium.org/12223062/diff/9009/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:958: if
(!master_prefs_->variations_seed.empty()) {
See referenced comment above, this should probably be gated on
|do_first_run_tasks_| instead of |is_first_run|.

https://codereview.chromium.org/12223062/diff/9009/chrome/browser/first_run/f...
File chrome/browser/first_run/first_run_unittest.cc (right):

https://codereview.chromium.org/12223062/diff/9009/chrome/browser/first_run/f...
chrome/browser/first_run/first_run_unittest.cc:16: namespace first_run {
I don't remember seeing tests inside a namespace in Chromium. Usually the tests
for a class in a namespace are not in the namespace (this has often struck me in
current code as it makes the test code convoluted with namespace prefixes) -- I
don't know if there is a good reason for that however... can you find examples
of other tests that are written inside their corresponding namespaces?

https://codereview.chromium.org/12223062/diff/9009/chrome/browser/first_run/f...
chrome/browser/first_run/first_run_unittest.cc:51: }  // namespace first_run
nit: Does this file end with a new line? The linter seems to think it doesn't,
but it's often wrong...

[Alexei Svitkine (slow), 2013-02-11 23:54:35]

Please refer to go/finch-omaha-first-run for the overall design. I'll answer
your questions below briefly, but you can get more info from the design doc.

On 2013/02/11 23:44:01, gab wrote:
> Some questions:
> 
> 1) Why is this required? Is it to get first run Finch experiments (i.e.
without
> even having to contact the server first)?

Yes.

> 
> 2) Who will be in charge of randomizing this seed? The download server? Omaha?

The seed will come from Omaha. It won't be randomized, all clients at a given
point in time will get the same seed. Randomization is done on the client.

> 
> 3) Are you aware that this exposes a backdoor into enabling/disabling specific
> Finch experiments (i.e. a user could constantly modify this seed and
re-trigger
> first run until he gets the desired set of experiments)? (I think one of the
> goal of Finch is that it isn't user controllable -- although I guess
> uninstalling/reinstalling Chrome or wiping his User Data directory is also a
> current backdoor?)

This can already be done via --reset-variation-state command-line flag by
design, so this isn't a concern.

[Alexei Svitkine (slow), 2013-02-12 00:11:31]

https://codereview.chromium.org/12223062/diff/9009/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/12223062/diff/9009/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:958: if
(!master_prefs_->variations_seed.empty()) {
On 2013/02/11 23:44:01, gab wrote:
> See referenced comment above, this should probably be gated on
> |do_first_run_tasks_| instead of |is_first_run|.

Done. Can you explain the difference between the two? In what real world
scenarios would doing it in do_first_run_tasks_ not be the same as doing it
here?

https://codereview.chromium.org/12223062/diff/9009/chrome/browser/first_run/f...
File chrome/browser/first_run/first_run_unittest.cc (right):

https://codereview.chromium.org/12223062/diff/9009/chrome/browser/first_run/f...
chrome/browser/first_run/first_run_unittest.cc:16: namespace first_run {
On 2013/02/11 23:44:01, gab wrote:
> I don't remember seeing tests inside a namespace in Chromium. Usually the
tests
> for a class in a namespace are not in the namespace (this has often struck me
in
> current code as it makes the test code convoluted with namespace prefixes) --
I
> don't know if there is a good reason for that however... can you find examples
> of other tests that are written inside their corresponding namespaces?

Sure, for example the majority of tests under gfx/ do this (and have been moving
to this direction more recently):

cat ui/gfx/*test.cc | grep -c "namespace gfx"
41

https://codereview.chromium.org/12223062/diff/9009/chrome/browser/first_run/f...
chrome/browser/first_run/first_run_unittest.cc:51: }  // namespace first_run
On 2013/02/11 23:44:01, gab wrote:
> nit: Does this file end with a new line? The linter seems to think it doesn't,
> but it's often wrong...

No, it's a rietveld bug.

[gab, 2013-02-12 00:31:06]

Will take a full look again tomorrow.

Cheers,
Gab

https://codereview.chromium.org/12223062/diff/9009/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/12223062/diff/9009/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:958: if
(!master_prefs_->variations_seed.empty()) {
On 2013/02/12 00:11:31, Alexei Svitkine wrote:
> On 2013/02/11 23:44:01, gab wrote:
> > See referenced comment above, this should probably be gated on
> > |do_first_run_tasks_| instead of |is_first_run|.
> 
> Done. Can you explain the difference between the two? In what real world
> scenarios would doing it in do_first_run_tasks_ not be the same as doing it
> here?

No difference in the real world; using |do_first_run_tasks_| will however
prevent this from happening in a situation where the First Run sentinel is
absent (i.e., it "is" first run as far as the browser is concerned), but
--no-first-run is on the command-line; this mainly makes sure that tests do not
end up running first run code.

I tried to unify the two variables in December IIRC as part of fixing master
preferences bugs for the fixit, but there was one thing holding me off (I
forget, something that needs to be initialized no matter what I think).

https://codereview.chromium.org/12223062/diff/9009/chrome/browser/first_run/f...
File chrome/browser/first_run/first_run_unittest.cc (right):

https://codereview.chromium.org/12223062/diff/9009/chrome/browser/first_run/f...
chrome/browser/first_run/first_run_unittest.cc:16: namespace first_run {
On 2013/02/12 00:11:31, Alexei Svitkine wrote:
> On 2013/02/11 23:44:01, gab wrote:
> > I don't remember seeing tests inside a namespace in Chromium. Usually the
> tests
> > for a class in a namespace are not in the namespace (this has often struck
me
> in
> > current code as it makes the test code convoluted with namespace prefixes)
--
> I
> > don't know if there is a good reason for that however... can you find
examples
> > of other tests that are written inside their corresponding namespaces?
> 
> Sure, for example the majority of tests under gfx/ do this (and have been
moving
> to this direction more recently):
> 
> cat ui/gfx/*test.cc | grep -c "namespace gfx"
> 41

Sounds good, I personally prefer this syntax too FWIW :)!

[SteveT, 2013-02-12 02:01:56]

Question inline.

https://codereview.chromium.org/12223062/diff/3017/chrome/browser/first_run/f...
File chrome/browser/first_run/first_run_unittest.cc (right):

https://codereview.chromium.org/12223062/diff/3017/chrome/browser/first_run/f...
chrome/browser/first_run/first_run_unittest.cc:49: }
Is it worth testing the negative result?

IE - Prefs without variations_seed doesn't set out_prefs.variations_seed

[Alexei Svitkine (slow), 2013-02-12 02:10:02]

https://codereview.chromium.org/12223062/diff/3017/chrome/browser/first_run/f...
File chrome/browser/first_run/first_run_unittest.cc (right):

https://codereview.chromium.org/12223062/diff/3017/chrome/browser/first_run/f...
chrome/browser/first_run/first_run_unittest.cc:49: }
On 2013/02/12 02:01:56, SteveT wrote:
> Is it worth testing the negative result?
> 
> IE - Prefs without variations_seed doesn't set out_prefs.variations_seed

Done.

[SteveT, 2013-02-12 02:12:30]

LGTM

https://codereview.chromium.org/12223062/diff/28001/chrome/browser/first_run/...
File chrome/browser/first_run/first_run_unittest.cc (right):

https://codereview.chromium.org/12223062/diff/28001/chrome/browser/first_run/...
chrome/browser/first_run/first_run_unittest.cc:56: EXPECT_EQ("",
out_prefs.variations_seed);
nit:

std::string()

instead of

""

?

[gab, 2013-02-12 18:33:53]

Some comments directly on doc.

[gab, 2013-02-13 19:07:22]

As I mentioned on the doc, not a fan of this approach (master_prefs ->
user_prefs -> local_state).

Re-iterating my thoughts here for reference.

Ideally master_prefs would be mapped, at first run, to:
 - Nothing in most cases.
 - User prefs when appropriate
 - Local state when appropriate
instead of overwriting the full user prefs on first run (see crbug.com/125124
for reference to problems this has already caused).

This change however is not the cause of this, nor is it making it much worse...
so LGTM.

[Alexei Svitkine (slow), 2013-02-13 23:58:36]

https://codereview.chromium.org/12223062/diff/28001/chrome/browser/first_run/...
File chrome/browser/first_run/first_run_unittest.cc (right):

https://codereview.chromium.org/12223062/diff/28001/chrome/browser/first_run/...
chrome/browser/first_run/first_run_unittest.cc:56: EXPECT_EQ("",
out_prefs.variations_seed);
On 2013/02/12 02:12:30, SteveT wrote:
> nit:
> 
> std::string()
> 
> instead of
> 
> ""
> 
> ?

Even better: .empty()

Done.

[commit-bot: I haz the power, 2013-02-14 00:02:37]

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

[commit-bot: I haz the power, 2013-02-14 00:02:42]

Presubmit check for 12223062-31001 failed and returned exit status 1.

INFO:root:Found 7 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py

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

Presubmit checks took 1.7s to calculate.

[Alexei Svitkine (slow), 2013-02-14 01:21:41]

+sky for chrome/ OWNERS

[sky, 2013-02-14 15:51:57]

LGTM

[commit-bot: I haz the power, 2013-02-14 22:36:35]

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

[commit-bot: I haz the power, 2013-02-15 06:36:06]

Change committed as 182631