Pulling user experiment code from BrowserDistribution to a new class.

Pulling user experiment code from BrowserDistribution to a new class. 

This is part #2A of the refactoring task to pull out installation_state from installer_util.

In the old code, code bloat exists is google_chrome_distribution.cc for user experiments.  This brings in lots of dependencies, and prevents BrowserDistribution from being a light-weight information (if not constant) getter class.

In this CL we pull:

enum ToastUIflags{}
struct UserExperiment{}
GetExperimentDetails()
LaunchUserExperiment()
InactiveUserToastExperiment()

from BrowserDistribution, and moved into a new class "class UserExperiment", with the following renames:

struct UserExperiment => struct ExperimentDetails
GetExperimentDetails() => CreateExperimentDetails()

The functions are made into static members of UserExperiment.  The only use on BrowserDistribution was in InactiveUserToastExperiment(), which now receives it as parameter.

Finally, ShouldSetExperimentLabels() is left with BrowserDistributions, since it is light-weight.

BUG=170398
TBR=ben

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

[huangs, 2013-02-21 23:41:36]

PTAL.

https://codereview.chromium.org/12321061/diff/1012/chrome/installer/util/user...
File chrome/installer/util/user_experiment.cc (right):

https://codereview.chromium.org/12321061/diff/1012/chrome/installer/util/user...
chrome/installer/util/user_experiment.cc:56: string16 LocalizeUrl(const wchar_t*
url) {
This routine is from google_chrome_distribution.cc (which also needs it).  I'm
thinking about moving this to GoogleUpdateSettings.

[grt (UTC plus 2), 2013-02-22 16:47:55]

i didn't read the code that was moved very thoroughly. the new user_experiment.h
doesn't comply with Chromium style. maybe it's better to fix that separately so
that it's easier to tell that nothing substantial has changed. what do you
think? for example, the enum constants should be ALL_UPPERCASE rather than
kMixedCase.

https://codereview.chromium.org/12321061/diff/1012/chrome/installer/util/user...
File chrome/installer/util/user_experiment.cc (right):

https://codereview.chromium.org/12321061/diff/1012/chrome/installer/util/user...
chrome/installer/util/user_experiment.cc:56: string16 LocalizeUrl(const wchar_t*
url) {
On 2013/02/21 23:41:36, huangs1 wrote:
> This routine is from google_chrome_distribution.cc (which also needs it).  I'm
> thinking about moving this to GoogleUpdateSettings.

I think InstallUtil is a better place for it.

https://codereview.chromium.org/12321061/diff/1012/chrome/installer/util/user...
File chrome/installer/util/user_experiment.h (right):

https://codereview.chromium.org/12321061/diff/1012/chrome/installer/util/user...
chrome/installer/util/user_experiment.h:25: class UserExperiment {
use a namespace (e.g., "user_experiment") to hold a collection of non-member
functions rather than a class of only static methods.

https://codereview.chromium.org/12321061/diff/1012/chrome/installer/util/user...
chrome/installer/util/user_experiment.h:33: kMakeDefault        = 8     // Has
the 'make it default' checkbox.
nit: trailing comma

https://codereview.chromium.org/12321061/diff/1013/chrome/installer/util/user...
File chrome/installer/util/user_experiment.cc (right):

https://codereview.chromium.org/12321061/diff/1013/chrome/installer/util/user...
chrome/installer/util/user_experiment.cc:37: const wchar_t kChromeGuid[] =
L"{8A69D345-D564-463c-AFF1-A69D9E530F96}";
this and the next three constants appear to be unused.

https://codereview.chromium.org/12321061/diff/1013/chrome/installer/util/user...
chrome/installer/util/user_experiment.cc:82: HANDLE file = ::CreateFileW(path,
0, share, NULL, OPEN_EXISTING,
use base::win::ScopedHandle and remove calls to CloseHandle below

https://codereview.chromium.org/12321061/diff/1013/chrome/installer/util/user...
File chrome/installer/util/user_experiment.h (right):

https://codereview.chromium.org/12321061/diff/1013/chrome/installer/util/user...
chrome/installer/util/user_experiment.h:56: static bool
CreateExperimentDetails(ExperimentDetails* experiment,
out params come last

https://codereview.chromium.org/12321061/diff/2012/chrome/installer/setup/set...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/12321061/diff/2012/chrome/installer/setup/set...
chrome/installer/setup/setup_main.cc:927:
installer::UserExperiment::LaunchUserExperiment(setup_path,
only call this for Google Chrome (not for Chromium, canary, Chrome Frame, the
binaries, etc...).

[gab, 2013-02-22 19:44:53]

1st look.

Can you highlight via comments in your next patch set which method were copied
as is and which ones have been tweaked (to make it easier to review)?

https://codereview.chromium.org/12321061/diff/2012/chrome/installer/setup/set...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/12321061/diff/2012/chrome/installer/setup/set...
chrome/installer/setup/setup_main.cc:927:
installer::UserExperiment::LaunchUserExperiment(setup_path,
On 2013/02/22 16:47:56, grt wrote:
> only call this for Google Chrome (not for Chromium, canary, Chrome Frame, the
> binaries, etc...).

Alternatively we could make LaunchUserExperiment and the likes call into the
current browser distribution to know whether it should run the experiment; given
only the google_chrome_distribution will return true I'm not sure what's
achieved by moving this code out of google_chrome_distribution...

You mentioned bloat, but user_experiment.cc is still in installer_util (or is
the goal to carve it out later?).

Overall I think I prefer having BrowserDistribution::ShouldRunExperimentX() over
having the consumer code (i.e., here) check for #if defined GOOGLE_CHROME_BUILD
and for distribution == CHROME_BROWSER

Maybe even just a generic BrowserDistribution::ShouldRunExperiments() if there
is no need to special case each individual experiments.

[huangs, 2013-02-22 20:32:36]

PTAL.

https://chromiumcodereview.appspot.com/12321061/diff/1012/chrome/installer/ut...
File chrome/installer/util/user_experiment.h (right):

https://chromiumcodereview.appspot.com/12321061/diff/1012/chrome/installer/ut...
chrome/installer/util/user_experiment.h:25: class UserExperiment {
On 2013/02/22 16:47:55, grt wrote:
> use a namespace (e.g., "user_experiment") to hold a collection of non-member
> functions rather than a class of only static methods.

Done.

https://chromiumcodereview.appspot.com/12321061/diff/1012/chrome/installer/ut...
chrome/installer/util/user_experiment.h:33: kMakeDefault        = 8     // Has
the 'make it default' checkbox.
On 2013/02/22 16:47:55, grt wrote:
> nit: trailing comma

Done.

https://chromiumcodereview.appspot.com/12321061/diff/1013/chrome/installer/ut...
File chrome/installer/util/user_experiment.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/1013/chrome/installer/ut...
chrome/installer/util/user_experiment.cc:37: const wchar_t kChromeGuid[] =
L"{8A69D345-D564-463c-AFF1-A69D9E530F96}";
Yes, missed a spot.  Deleted.

https://chromiumcodereview.appspot.com/12321061/diff/1013/chrome/installer/ut...
chrome/installer/util/user_experiment.cc:82: HANDLE file = ::CreateFileW(path,
0, share, NULL, OPEN_EXISTING,
On 2013/02/22 16:47:56, grt wrote:
> use base::win::ScopedHandle and remove calls to CloseHandle below

Done.

https://chromiumcodereview.appspot.com/12321061/diff/1013/chrome/installer/ut...
File chrome/installer/util/user_experiment.h (right):

https://chromiumcodereview.appspot.com/12321061/diff/1013/chrome/installer/ut...
chrome/installer/util/user_experiment.h:56: static bool
CreateExperimentDetails(ExperimentDetails* experiment,
On 2013/02/22 16:47:56, grt wrote:
> out params come last

Done.

https://chromiumcodereview.appspot.com/12321061/diff/2012/chrome/installer/se...
File chrome/installer/setup/setup_main.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/2012/chrome/installer/se...
chrome/installer/setup/setup_main.cc:927:
installer::UserExperiment::LaunchUserExperiment(setup_path,
It seems Canary *does* call the experiment, since the parent class of
GoogleChromeSxSDistribution is GoogleChromeDistribution.

https://chromiumcodereview.appspot.com/12321061/diff/2012/chrome/installer/se...
chrome/installer/setup/setup_main.cc:927:
installer::UserExperiment::LaunchUserExperiment(setup_path,
I ended up defining

enum UserExperiment::ExperimentCode {...}

use

UserExperiment::IsExperimentSupported((BrowserDistribution::Type,
ExperimentCode)

to do the (currently trivial) decision logic, and let the (3 existing) callers
call this routine to decide.

[gab, 2013-02-25 22:09:53]

Some comments below.

Cheers!
Gab

https://codereview.chromium.org/12321061/diff/2012/chrome/installer/setup/set...
File chrome/installer/setup/setup_main.cc (right):

https://codereview.chromium.org/12321061/diff/2012/chrome/installer/setup/set...
chrome/installer/setup/setup_main.cc:927:
installer::UserExperiment::LaunchUserExperiment(setup_path,
On 2013/02/22 20:32:37, huangs1 wrote:
> I ended up defining
> 
> enum UserExperiment::ExperimentCode {...}
> 
> use
> 
> UserExperiment::IsExperimentSupported((BrowserDistribution::Type,
> ExperimentCode)
> 
> to do the (currently trivial) decision logic, and let the (3 existing) callers
> call this routine to decide.

I don't like the paradigm:

if (ShouldIDoCrazyThingX())
  DoCrazyThingX();

as it makes room for the caller not knowing or simply forgetting to check the
condition and end up calling DoCrazyThingX() straight up (which assumes the
caller has made the check); how about we put the check at the top of every
experiment method instead, forcing an early return if we shouldn't do the
experiment?

https://codereview.chromium.org/12321061/diff/4015/chrome/installer/util/user...
File chrome/installer/util/user_experiment.cc (right):

https://codereview.chromium.org/12321061/diff/4015/chrome/installer/util/user...
chrome/installer/util/user_experiment.cc:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
Don't change the date of existing files. Our policy is to only apply the current
year on new files (this has changed in the last couple months so it's possible
you were asked to change it before, but we don't do it anymore -- see
chromium-dev for details).

https://codereview.chromium.org/12321061/diff/4015/chrome/installer/util/user...
chrome/installer/util/user_experiment.cc:295: 
nit: only a single empty line here.

https://codereview.chromium.org/12321061/diff/4015/chrome/installer/util/user...
chrome/installer/util/user_experiment.cc:299: bool res = false;
Why "res"? The short for return value is usually "ret".

https://codereview.chromium.org/12321061/diff/4015/chrome/installer/util/user...
chrome/installer/util/user_experiment.cc:302: || code ==
kExperimentInactiveUserToast) {
|| goes on line above.

https://codereview.chromium.org/12321061/diff/4015/chrome/installer/util/user...
File chrome/installer/util/user_experiment.h (right):

https://codereview.chromium.org/12321061/diff/4015/chrome/installer/util/user...
chrome/installer/util/user_experiment.h:38: kUninstall          = 1,    //
Uninstall radio button.
s/1/1 << 0

https://codereview.chromium.org/12321061/diff/4015/chrome/installer/util/user...
chrome/installer/util/user_experiment.h:39: kDontBugMeAsButton  = 2,    // Don't
bug me is a button, not a radio button.
s/2/1 << 1

https://codereview.chromium.org/12321061/diff/4015/chrome/installer/util/user...
chrome/installer/util/user_experiment.h:40: kWhyLink            = 4,    // Has
the 'why I am seeing this' link.
s/4/1 << 2

https://codereview.chromium.org/12321061/diff/4015/chrome/installer/util/user...
chrome/installer/util/user_experiment.h:41: kMakeDefault        = 8,    // Has
the 'make it default' checkbox.
s/8/1 << 3

This is how bitfields are usually written in Chromium: makes it easier to see
it's a bitfield (and less likely to make a typo).

[grt (UTC plus 2), 2013-02-26 02:42:09]

https://codereview.chromium.org/12321061/diff/4015/chrome/installer/util/user...
File chrome/installer/util/user_experiment.cc (right):

https://codereview.chromium.org/12321061/diff/4015/chrome/installer/util/user...
chrome/installer/util/user_experiment.cc:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
On 2013/02/25 22:09:53, gab wrote:
> Don't change the date of existing files. Our policy is to only apply the
current
> year on new files (this has changed in the last couple months so it's possible
> you were asked to change it before, but we don't do it anymore -- see
> chromium-dev for details).

Please keep the header as you have it since this is a new file. Rietveld and/or
git was simply too aggressive in finding a file from which its contents were
moved.

https://codereview.chromium.org/12321061/diff/4015/chrome/installer/util/user...
chrome/installer/util/user_experiment.cc:299: bool res = false;
On 2013/02/25 22:09:53, gab wrote:
> Why "res"? The short for return value is usually "ret".

these are both bad names. style dictates not to abbreviate. please use "result".

https://codereview.chromium.org/12321061/diff/4015/chrome/installer/util/user...
File chrome/installer/util/user_experiment.h (right):

https://codereview.chromium.org/12321061/diff/4015/chrome/installer/util/user...
chrome/installer/util/user_experiment.h:28: class UserExperiment {
my apologies for not being clear in my previous review. what i meant was that
you should get rid of "class UserExperiment" altogether. instead, the types and
static methods should be non-member functions in the user_experiment namespace.
in other words, delete lines 28-30, 87-88, remove "static" here and there, and
de-indent the rest.

[huangs, 2013-02-26 03:22:25]

PTAL.  The code is behind in sync (so try job has to be specially done), because
I'm scared of the Skia build failure Boogeyman.  :)

https://chromiumcodereview.appspot.com/12321061/diff/2012/chrome/installer/se...
File chrome/installer/setup/setup_main.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/2012/chrome/installer/se...
chrome/installer/setup/setup_main.cc:927:
installer::UserExperiment::LaunchUserExperiment(setup_path,
If there are expensive initialization needed, they can be placed after if(). 
But they don't in this case, so I'll make the change.

Also, should the name be changed, to reflect that experiment may not be run,
e.g., "LaunchUserExperimentIfRequired()"?

https://chromiumcodereview.appspot.com/12321061/diff/4015/chrome/installer/ut...
File chrome/installer/util/user_experiment.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/4015/chrome/installer/ut...
chrome/installer/util/user_experiment.cc:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
Keeping it.  I copied the header from recent email on boilerplate update.

https://chromiumcodereview.appspot.com/12321061/diff/4015/chrome/installer/ut...
chrome/installer/util/user_experiment.cc:295: 
Done, though IsExperimentSupported() is now gone.

https://chromiumcodereview.appspot.com/12321061/diff/4015/chrome/installer/ut...
chrome/installer/util/user_experiment.cc:299: bool res = false;
Okay, but routine is deleted now, in favour of direct check in  the beginning of
LaunchUserExperiment() and InactiveUserToastExperiment().

https://chromiumcodereview.appspot.com/12321061/diff/4015/chrome/installer/ut...
chrome/installer/util/user_experiment.cc:302: || code ==
kExperimentInactiveUserToast) {
Moot; but will do in future.

https://chromiumcodereview.appspot.com/12321061/diff/4015/chrome/installer/ut...
File chrome/installer/util/user_experiment.h (right):

https://chromiumcodereview.appspot.com/12321061/diff/4015/chrome/installer/ut...
chrome/installer/util/user_experiment.h:28: class UserExperiment {
Ah yes, I misread.  Done.

https://chromiumcodereview.appspot.com/12321061/diff/4015/chrome/installer/ut...
chrome/installer/util/user_experiment.h:38: kUninstall          = 1,    //
Uninstall radio button.
s/$/\//.  :)  Done.

https://chromiumcodereview.appspot.com/12321061/diff/4015/chrome/installer/ut...
chrome/installer/util/user_experiment.h:39: kDontBugMeAsButton  = 2,    // Don't
bug me is a button, not a radio button.
On 2013/02/25 22:09:53, gab wrote:
> s/2/1 << 1

Done.

https://chromiumcodereview.appspot.com/12321061/diff/4015/chrome/installer/ut...
chrome/installer/util/user_experiment.h:40: kWhyLink            = 4,    // Has
the 'why I am seeing this' link.
On 2013/02/25 22:09:53, gab wrote:
> s/4/1 << 2

Done.

https://chromiumcodereview.appspot.com/12321061/diff/4015/chrome/installer/ut...
chrome/installer/util/user_experiment.h:41: kMakeDefault        = 8,    // Has
the 'make it default' checkbox.
On 2013/02/25 22:09:53, gab wrote:
> s/8/1 << 3
> 
> This is how bitfields are usually written in Chromium: makes it easier to see
> it's a bitfield (and less likely to make a typo).

Done.

[huangs, 2013-02-26 03:25:16]

https://chromiumcodereview.appspot.com/12321061/diff/6010/chrome/installer/ut...
File chrome/installer/util/user_experiment.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/6010/chrome/installer/ut...
chrome/installer/util/user_experiment.cc:403: void
LaunchUserExperiment(BrowserDistribution* dist,
|dist| can be obtained from |product|, but I think it's better to not assume
that.  Please let me know what you think.

[gab, 2013-02-26 15:33:34]

Looks good. Comments below.

(FYI, I synced yesterday afternoon and didn't get hit by the Skia boogeyman ;)
-- if you do sync please update a patch set just for the sync before (or after)
applying changes (easier for us to see the real diff)).

Cheers!
Gab

https://chromiumcodereview.appspot.com/12321061/diff/2012/chrome/installer/se...
File chrome/installer/setup/setup_main.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/2012/chrome/installer/se...
chrome/installer/setup/setup_main.cc:927:
installer::UserExperiment::LaunchUserExperiment(setup_path,
On 2013/02/26 03:22:25, huangs1 wrote:
> If there are expensive initialization needed, they can be placed after if(). 
> But they don't in this case, so I'll make the change.
> 
> Also, should the name be changed, to reflect that experiment may not be run,
> e.g., "LaunchUserExperimentIfRequired()"?  

Sure.

https://chromiumcodereview.appspot.com/12321061/diff/4015/chrome/installer/ut...
File chrome/installer/util/user_experiment.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/4015/chrome/installer/ut...
chrome/installer/util/user_experiment.cc:1: // Copyright 2013 The Chromium
Authors. All rights reserved.
On 2013/02/26 02:42:09, grt wrote:
> On 2013/02/25 22:09:53, gab wrote:
> > Don't change the date of existing files. Our policy is to only apply the
> current
> > year on new files (this has changed in the last couple months so it's
possible
> > you were asked to change it before, but we don't do it anymore -- see
> > chromium-dev for details).
> 
> Please keep the header as you have it since this is a new file. Rietveld
and/or
> git was simply too aggressive in finding a file from which its contents were
> moved.

Oh right, my bad... somehow forgot this was a new file because of the nice
"copy+diff" :)!

https://chromiumcodereview.appspot.com/12321061/diff/6010/chrome/browser/firs...
File chrome/browser/first_run/try_chrome_dialog_view.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/6010/chrome/browser/firs...
chrome/browser/first_run/try_chrome_dialog_view.cc:214: != 0);
nit: "!=" goes at the end of the previous line.... if that doesn't fit you'll
need to use another indent (note that the external brackets are superfluous).

https://chromiumcodereview.appspot.com/12321061/diff/6010/chrome/installer/ut...
File chrome/installer/util/user_experiment.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/6010/chrome/installer/ut...
chrome/installer/util/user_experiment.cc:403: void
LaunchUserExperiment(BrowserDistribution* dist,
On 2013/02/26 03:25:16, huangs1 wrote:
> |dist| can be obtained from |product|, but I think it's better to not assume
> that.  Please let me know what you think.

I think it's fine to take the dist from the product. I don't see how a caller
would pass a |diff| that isn't |product.distribution()|...
Please do that.

[huangs, 2013-02-26 16:44:11]

PTAL.

https://chromiumcodereview.appspot.com/12321061/diff/2012/chrome/installer/se...
File chrome/installer/setup/setup_main.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/2012/chrome/installer/se...
chrome/installer/setup/setup_main.cc:927:
installer::UserExperiment::LaunchUserExperiment(setup_path,
Done (changed to *IfRequired()).

https://chromiumcodereview.appspot.com/12321061/diff/6010/chrome/browser/firs...
File chrome/browser/first_run/try_chrome_dialog_view.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/6010/chrome/browser/firs...
chrome/browser/first_run/try_chrome_dialog_view.cc:214: != 0);
Using !! to cast to bool (used elsewhere in Chrome, too).

https://chromiumcodereview.appspot.com/12321061/diff/6010/chrome/installer/ut...
File chrome/installer/util/user_experiment.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/6010/chrome/installer/ut...
chrome/installer/util/user_experiment.cc:403: void
LaunchUserExperiment(BrowserDistribution* dist,
Removed.

[gab, 2013-02-26 18:09:53]

Looks really good, looks like you have linking problems though.

https://codereview.chromium.org/12321061/diff/2016/chrome/installer/util/user...
File chrome/installer/util/user_experiment.cc (right):

https://codereview.chromium.org/12321061/diff/2016/chrome/installer/util/user...
chrome/installer/util/user_experiment.cc:47: const wchar_t kToastUDDirFailure[]
=           L"40";
These two strings are the same... looks weird to me... this is beyond the scope
of this CL, but it would be nice to look into this.

https://codereview.chromium.org/12321061/diff/2016/chrome/installer/util/user...
chrome/installer/util/user_experiment.cc:511: BrowserDistribution* dist,
Please remove the |dist| parameter here as well.

[huangs, 2013-02-26 18:52:51]

Sorry, forgot to save file...  Now it builds.  PTAL.

https://chromiumcodereview.appspot.com/12321061/diff/2016/chrome/installer/ut...
File chrome/installer/util/user_experiment.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/2016/chrome/installer/ut...
chrome/installer/util/user_experiment.cc:47: const wchar_t kToastUDDirFailure[]
=           L"40";
I don't know much about the Toast experiment, but maybe kToastActiveGroup (being
active) choose from {kToastUDDirFailure, kToastExpBaseGroup} ??

https://chromiumcodereview.appspot.com/12321061/diff/2016/chrome/installer/ut...
chrome/installer/util/user_experiment.cc:511: BrowserDistribution* dist,
Problem was, I forgot to save -- this also caused the link error.  Fixed.

[gab, 2013-02-26 18:54:56]

On 2013/02/26 18:52:51, huangs1 wrote:
> Sorry, forgot to save file...  Now it builds.  PTAL.

Ah :), lgtm!

[grt (UTC plus 2), 2013-02-27 02:13:33]

let's talk tomorrow. i don't like the "IfRequired" suffix or the fact that
policy is now mixed with implementation. it also looks like this will now try to
do work for Chromium, which wasn't previously the case.

[huangs, 2013-03-04 21:47:49]

Here's another preliminary fix.  Some uncertainties:

- Looking at the old code, it appears that Chrome / Chromium / Chrome SXS *all*
run the experiments?!  My understanding is, GoogleChromeDistribution is for
Chrome and Chromium, then GoogleChromeSxSDistribution inherits from
GoogleChromeDistribution.
- So right now experiments runs in all 3.
- Also, I'm adding ProductOperations::ShouldLaunchUserExperiment() to make the
decision, which is invoked by the decider/wrapper
Product::LaunchUserExperiment().  I don't know if this was what grt@ asked for.
- InactiveUserToastExperiment() is just called directly, since it is involved by
a switch, which was invoked via LaunchUserExperiment().

PTAL.

https://chromiumcodereview.appspot.com/12321061/diff/25001/chrome/installer/u...
File chrome/installer/util/google_chrome_distribution_dummy.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/25001/chrome/installer/u...
chrome/installer/util/google_chrome_distribution_dummy.cc:146: bool
GoogleChromeDistribution::ShouldSetExperimentLabels() {
This is just code movement.

[gab, 2013-03-04 22:07:25]

On 2013/03/04 21:47:49, huangs1 wrote:
> Here's another preliminary fix.  Some uncertainties:
> 
> - Looking at the old code, it appears that Chrome / Chromium / Chrome SXS
*all*
> run the experiments?!  My understanding is, GoogleChromeDistribution is for
> Chrome and Chromium, then GoogleChromeSxSDistribution inherits from
> GoogleChromeDistribution.

No, BrowserDistribution is Chromium (yes this is ugly: i.e., there should be a
base class and then two implementations for Chromium and Google chrome, but this
is not the case).

Looks like you're right about Canary though (although we likely never push any
experiments to Canary so this is irrelevant...).

I'll wait for you to fix your first assumption to review further.

Cheers!
Gab


> - So right now experiments runs in all 3.
> - Also, I'm adding ProductOperations::ShouldLaunchUserExperiment() to make the
> decision, which is invoked by the decider/wrapper
> Product::LaunchUserExperiment().  I don't know if this was what grt@ asked
for.
> - InactiveUserToastExperiment() is just called directly, since it is involved
by
> a switch, which was invoked via LaunchUserExperiment().
> 
> PTAL.
> 
>
https://chromiumcodereview.appspot.com/12321061/diff/25001/chrome/installer/u...
> File chrome/installer/util/google_chrome_distribution_dummy.cc (right):
> 
>
https://chromiumcodereview.appspot.com/12321061/diff/25001/chrome/installer/u...
> chrome/installer/util/google_chrome_distribution_dummy.cc:146: bool
> GoogleChromeDistribution::ShouldSetExperimentLabels() {
> This is just code movement.

[huangs, 2013-03-04 23:10:33]

Ah, I see...  PTAL.

[gab, 2013-03-04 23:40:30]

Some comments, will wait for grt to chime in.

Cheers!
Gab

https://codereview.chromium.org/12321061/diff/21006/chrome/installer/util/pro...
File chrome/installer/util/product.cc (right):

https://codereview.chromium.org/12321061/diff/21006/chrome/installer/util/pro...
chrome/installer/util/product.cc:170: if
(operations_->ShouldLaunchUserExperiment()) {
I'm really not a fan of having these two entry points into
LaunchUserExperiments(), only one of which (this one, not the direct call)
checks ShouldLaunchUserExperiment().

I feel having the ShouldLaunchUserExperiment() check at the top of the
experiment methods was better than this, I'll let grt chime in since he's the
one who objected to having the policy at the top of the impl.

https://codereview.chromium.org/12321061/diff/21006/chrome/installer/util/use...
File chrome/installer/util/user_experiment.cc (right):

https://codereview.chromium.org/12321061/diff/21006/chrome/installer/util/use...
chrome/installer/util/user_experiment.cc:78: base::win::ScopedHandle
file(CreateFileW(path, 0, share, NULL, OPEN_EXISTING,
Keep the leading :: for CreateFileW, this guarantees that you are using the one
defined in the global namespace (and tends to be common style when using methods
from the Windows API).

(I also think the 'W' in the method name is unnecessary, but grt (our
C++/Windows guru) would definitely know more than I on this (IIRC CreateFile is
#defined to the right impl at compile time -- hardcoding it to one impl isn't
necessary))

[grt (UTC plus 2), 2013-03-05 14:33:21]

https://codereview.chromium.org/12321061/diff/21006/chrome/installer/util/pro...
File chrome/installer/util/product.cc (right):

https://codereview.chromium.org/12321061/diff/21006/chrome/installer/util/pro...
chrome/installer/util/product.cc:170: if
(operations_->ShouldLaunchUserExperiment()) {
what i had in mind is:
  if (distribution_->HasUserExperiments())
    operations_->LaunchUserExperiment(...)
this puts the policy in BrowserDistribution (where all distribution-specific
policy belongs, imo) and the implementation in ProductOperations. decoupling the
two means that future changes in policy don't need to touch the implementation.
also, the implementation can conceivably be tested without having to fake the
correct policy.

https://codereview.chromium.org/12321061/diff/21006/chrome/installer/util/use...
File chrome/installer/util/user_experiment.h (right):

https://codereview.chromium.org/12321061/diff/21006/chrome/installer/util/use...
chrome/installer/util/user_experiment.h:25: namespace user_experiment {
looking at how this is used, i'm now of the mind that these types and functions
should just be in the installer namespace. there's no risk of name collision as
far as i can tell, and seeing user_experiment doesn't really help the reader all
that much given that all functions have "Experiment" in their names. so, with
that, please nix the user_experiment namespace altogether unless someone
strongly objects.

[gab, 2013-03-05 14:58:00]

SGTM.

https://codereview.chromium.org/12321061/diff/21006/chrome/installer/util/pro...
File chrome/installer/util/product.cc (right):

https://codereview.chromium.org/12321061/diff/21006/chrome/installer/util/pro...
chrome/installer/util/product.cc:170: if
(operations_->ShouldLaunchUserExperiment()) {
On 2013/03/05 14:33:22, grt wrote:
> what i had in mind is:
>   if (distribution_->HasUserExperiments())
>     operations_->LaunchUserExperiment(...)
> this puts the policy in BrowserDistribution (where all distribution-specific
> policy belongs, imo) and the implementation in ProductOperations. decoupling
the
> two means that future changes in policy don't need to touch the
implementation.
> also, the implementation can conceivably be tested without having to fake the
> correct policy.

Ah ok, the downside is that callers have to remember to check the policy, but I
agree that being able to test without faking the policy and being able to tweak
the policy without changing the impl is nice (although you get the latter if the
policy is just a method call at the top of the impl).

https://codereview.chromium.org/12321061/diff/21006/chrome/installer/util/use...
File chrome/installer/util/user_experiment.h (right):

https://codereview.chromium.org/12321061/diff/21006/chrome/installer/util/use...
chrome/installer/util/user_experiment.h:25: namespace user_experiment {
On 2013/03/05 14:33:22, grt wrote:
> looking at how this is used, i'm now of the mind that these types and
functions
> should just be in the installer namespace. there's no risk of name collision
as
> far as i can tell, and seeing user_experiment doesn't really help the reader
all
> that much given that all functions have "Experiment" in their names. so, with
> that, please nix the user_experiment namespace altogether unless someone
> strongly objects.

sgtm.

[huangs, 2013-03-05 15:38:00]

https://chromiumcodereview.appspot.com/12321061/diff/21006/chrome/installer/u...
File chrome/installer/util/product.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/21006/chrome/installer/u...
chrome/installer/util/product.cc:170: if
(operations_->ShouldLaunchUserExperiment()) {
Personally I'd prefer NOT changing Operations, since this just means we'll have
a lot of stub implemenetations.  Also, installer::LaunchUserExperiment() takes a
Products as parameter.  Making ChromeBrowserOperations aware of Products seems
wrong.

[grt (UTC plus 2), 2013-03-05 16:30:49]

https://chromiumcodereview.appspot.com/12321061/diff/21006/chrome/installer/u...
File chrome/installer/util/product.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/21006/chrome/installer/u...
chrome/installer/util/product.cc:170: if
(operations_->ShouldLaunchUserExperiment()) {
On 2013/03/05 15:38:00, huangs1 wrote:
> Personally I'd prefer NOT changing Operations, since this just means we'll
have
> a lot of stub implemenetations.

If you want to avoid creating stub implementations, then make an abstract
ProductOperationsImpl or somesuch that noops all methods and have the concrete
classes derive from it and override only those that they care about.

> Also, installer::LaunchUserExperiment() takes a
> Products as parameter.  Making ChromeBrowserOperations aware of Products seems
> wrong.

Then something else needs to be done. The policy should not be in
ProductOperations as you have it right now. If the current experiment code only
ever applies to the browser, then perhaps it can be refactored so that a Product
isn't passed down into the ProductOperations. Otherwise, look at why Product is
passed down and work out a way to avoid that.

[huangs, 2013-03-05 16:48:34]

This is an intermediate update, as a "lite" version without dependency on
ProductOperations.

I'm going to add ProductOperationsImpl first, then return to this CL.

https://chromiumcodereview.appspot.com/12321061/diff/21006/chrome/installer/u...
File chrome/installer/util/product.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/21006/chrome/installer/u...
chrome/installer/util/product.cc:170: if
(operations_->ShouldLaunchUserExperiment()) {
Okay, I'm for making ProductOperationsImpl!  Let's do this as a prerequesite
cleanup for this CL.

installer::LauncherUserExperiment() uses the following parts of Product:

AppendProductFlags()  =>  can get from ProductOperations.
distribution()->GetAppShortCutName()
GetUserDataPaths()  =>  Part of Product(), but can be directly injected.

I'll have to think some more.  Meanwhile, let's add ProductOperationImpl first.

https://chromiumcodereview.appspot.com/12321061/diff/21006/chrome/installer/u...
File chrome/installer/util/user_experiment.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/21006/chrome/installer/u...
chrome/installer/util/user_experiment.cc:78: base::win::ScopedHandle
file(CreateFileW(path, 0, share, NULL, OPEN_EXISTING,
On 2013/03/04 23:40:30, gab wrote:
> Keep the leading :: for CreateFileW, this guarantees that you are using the
one
> defined in the global namespace (and tends to be common style when using
methods
> from the Windows API).
> 
> (I also think the 'W' in the method name is unnecessary, but grt (our
> C++/Windows guru) would definitely know more than I on this (IIRC CreateFile
is
> #defined to the right impl at compile time -- hardcoding it to one impl isn't
> necessary))

Done.

https://chromiumcodereview.appspot.com/12321061/diff/21006/chrome/installer/u...
File chrome/installer/util/user_experiment.h (right):

https://chromiumcodereview.appspot.com/12321061/diff/21006/chrome/installer/u...
chrome/installer/util/user_experiment.h:25: namespace user_experiment {
Done.  Also changing members of ToastUiFlags (just renamed from ToastUIflags) to
have prefix kToastUi*.

[huangs, 2013-03-05 17:04:57]

Wait, installer::LaunchUserExperiment() only has 2 callers in setup_main.cc, and
is not likely to get more.  I really think it suffices to let the caller check
dist->HasUserExperiments(), then make the call to
installer::LaunchUserExperiment().  If we insist on a wrapper, then semantically
"Product::LaunchUserExperiment()" would be a misnomer, since it is possible that
nothing gets launched.

If we insist on not making caller decide then we can have a stand-alone
"LaunchUserExperimentIfRequired()" wrapper that lives in setup_main.cc or
user_experiments.cc.

[gab, 2013-03-05 20:08:05]

On 2013/03/05 17:04:57, huangs1 wrote:
> Wait, installer::LaunchUserExperiment() only has 2 callers in setup_main.cc,
and
> is not likely to get more.  I really think it suffices to let the caller check
> dist->HasUserExperiments(), then make the call to
> installer::LaunchUserExperiment().  If we insist on a wrapper, then
semantically
> "Product::LaunchUserExperiment()" would be a misnomer, since it is possible
that
> nothing gets launched.
> 
> If we insist on not making caller decide then we can have a stand-alone
> "LaunchUserExperimentIfRequired()" wrapper that lives in setup_main.cc or
> user_experiments.cc.

I think we decided on having the if dist->HasUserExperiments() in the caller
already, no?

(I objected to this originally, but grt made good points, I'm fine with it now.)

[huangs, 2013-03-05 20:21:28]

That's what I want as well.  CMIIW, the impression I'm getting from grt@ is that
we should have Product::LaunchUserExperiment(), which then calls

   if (distribution_->HasUserExperiments())
     operations_->LaunchUserExperiment(...)

so both BrowserDistribution and ProductOperation do dispatch?

[grt (UTC plus 2), 2013-03-05 21:18:24]

On 2013/03/05 17:04:57, huangs1 wrote:
> Wait, installer::LaunchUserExperiment() only has 2 callers in setup_main.cc,
and
> is not likely to get more.  I really think it suffices to let the caller check
> dist->HasUserExperiments(), then make the call to
> installer::LaunchUserExperiment().  If we insist on a wrapper, then
semantically
> "Product::LaunchUserExperiment()" would be a misnomer, since it is possible
that
> nothing gets launched.

I think it's fine for "if the product and distribution support experiments" to
be implied. If you search through the codebase I imagine you'll find many
instances of virtual methods that are noops under some circumstances.

> If we insist on not making caller decide then we can have a stand-alone
> "LaunchUserExperimentIfRequired()" wrapper that lives in setup_main.cc or
> user_experiments.cc.

We have an installer that during its general flow of installing products has a
point where product-specific experiments should be launched. The installer
itself has no reason to know what products run experiments or what those
experiments are. I think the current code in InstallProductsHelper makes sense:
loop over all products and fire off their respective experiments. The open
questions, as far as I can tell, are: 1) where does the experiment code live,
and 2) where does the policy live.

For #1: We have a product-specific virtual dispatch mechanism in place (via the
Product/ProductOperations pair). It makes sense to me that a browser-specific
experiment be launched through the ChromeBrowserOperations class. If we decide
to make an experiment for the app launcher someday (as an example), its
implementation would naturally be in the appropriate Operations class. Do you
guys agree on this?

For #2: It's fragile to expect all callers to make a policy check before
invoking a function. This is bound to get messed up someday and it's ugly. The
policy check could either be at the top of the impl in ChromeBrowserOperations
(which I don't like because of my aversion to mixing policy and implementation),
or in Product since that's the only path for a caller to get to a
ProductOperations's experiment. Do you guys feel strongly one way or another. If
so, why?

[gab, 2013-03-05 21:26:37]

On 2013/03/05 21:18:24, grt wrote:
> On 2013/03/05 17:04:57, huangs1 wrote:
> > Wait, installer::LaunchUserExperiment() only has 2 callers in setup_main.cc,
> and
> > is not likely to get more.  I really think it suffices to let the caller
check
> > dist->HasUserExperiments(), then make the call to
> > installer::LaunchUserExperiment().  If we insist on a wrapper, then
> semantically
> > "Product::LaunchUserExperiment()" would be a misnomer, since it is possible
> that
> > nothing gets launched.
> 
> I think it's fine for "if the product and distribution support experiments" to
> be implied. If you search through the codebase I imagine you'll find many
> instances of virtual methods that are noops under some circumstances.
> 
> > If we insist on not making caller decide then we can have a stand-alone
> > "LaunchUserExperimentIfRequired()" wrapper that lives in setup_main.cc or
> > user_experiments.cc.
> 
> We have an installer that during its general flow of installing products has a
> point where product-specific experiments should be launched. The installer
> itself has no reason to know what products run experiments or what those
> experiments are. I think the current code in InstallProductsHelper makes
sense:
> loop over all products and fire off their respective experiments. The open
> questions, as far as I can tell, are: 1) where does the experiment code live,
> and 2) where does the policy live.

SGTM

> 
> For #1: We have a product-specific virtual dispatch mechanism in place (via
the
> Product/ProductOperations pair). It makes sense to me that a browser-specific
> experiment be launched through the ChromeBrowserOperations class. If we decide
> to make an experiment for the app launcher someday (as an example), its
> implementation would naturally be in the appropriate Operations class. Do you
> guys agree on this?
> 
> For #2: It's fragile to expect all callers to make a policy check before
> invoking a function. This is bound to get messed up someday and it's ugly. The
> policy check could either be at the top of the impl in ChromeBrowserOperations
> (which I don't like because of my aversion to mixing policy and
implementation),
> or in Product since that's the only path for a caller to get to a
> ProductOperations's experiment. Do you guys feel strongly one way or another.
If
> so, why?

I very much agree that having the callers check the policy is bound to get
messed up someday and that's why I opposed to that paradigm initially.

If the only way to invoke an experiment is through the call in Product, then it
makes sense to me to have the policy check there. This also allows circumventing
the policy for testing by calling directly into a specific ProductOperations'
impl.

(i.e., I would like it to be impossible to invoke experiments without going
through the policy (unless bypassing virtual dispatch -- e.g., calling an impl
directly -- which is useful for testing as grt mentioned earlier)).

[huangs, 2013-03-05 21:40:45]

On 2013/03/05 21:18:24, grt wrote:
> On 2013/03/05 17:04:57, huangs1 wrote:
> > Wait, installer::LaunchUserExperiment() only has 2 callers in setup_main.cc,
> and
> > is not likely to get more.  I really think it suffices to let the caller
check
> > dist->HasUserExperiments(), then make the call to
> > installer::LaunchUserExperiment().  If we insist on a wrapper, then
> semantically
> > "Product::LaunchUserExperiment()" would be a misnomer, since it is possible
> that
> > nothing gets launched.
> 
> I think it's fine for "if the product and distribution support experiments" to
> be implied. If you search through the codebase I imagine you'll find many
> instances of virtual methods that are noops under some circumstances.

Okay, no problem with naming.  I also agree that having caller perform policy
check before calling is bad, so we need to do the check and call at exactly one
place in the code -- but this can be a stand-alone routine or in Product.  I'm
fine either way, really.

> > If we insist on not making caller decide then we can have a stand-alone
> > "LaunchUserExperimentIfRequired()" wrapper that lives in setup_main.cc or
> > user_experiments.cc.
> 
> We have an installer that during its general flow of installing products has a
> point where product-specific experiments should be launched. The installer
> itself has no reason to know what products run experiments or what those
> experiments are. I think the current code in InstallProductsHelper makes
sense:
> loop over all products and fire off their respective experiments. The open
> questions, as far as I can tell, are: 1) where does the experiment code live,
> and 2) where does the policy live.
> 
> For #1: We have a product-specific virtual dispatch mechanism in place (via
the
> Product/ProductOperations pair). It makes sense to me that a browser-specific
> experiment be launched through the ChromeBrowserOperations class. If we decide
> to make an experiment for the app launcher someday (as an example), its
> implementation would naturally be in the appropriate Operations class. Do you
> guys agree on this?

If there are drastically different sets of experiments, then virtual dispatch
make perfect sense.  But right now there's only one experiment (shared between
Chrome and ChromeSxS).  I think trying to use virtual dispatch now would be
over-designing; we can do this when/if the need arises.

> For #2: It's fragile to expect all callers to make a policy check before
> invoking a function. This is bound to get messed up someday and it's ugly. The
> policy check could either be at the top of the impl in ChromeBrowserOperations
> (which I don't like because of my aversion to mixing policy and
implementation),
> or in Product since that's the only path for a caller to get to a
> ProductOperations's experiment. Do you guys feel strongly one way or another.
If
> so, why?

Again, having distributed policy check is bad.  I'd prefer to have the check and
the call (directly to UserExperiments) in Product, and leave ProductOperations
alone.  The assumption I'm making is:
- Product serves as a glue code, not just wrapper for operations_ and
istribution_.
- A single check-and-call, as seen in Product::LaunchUserExperiment() in Patch
Set 13, is acceptable as glue; there's no need to go one layer deeper, and
dispatch the call in ProductOperations' sub classes (while passing multiple
parameters, requiring more #includes, etc.)

[grt (UTC plus 2), 2013-03-05 21:46:54]

On 2013/03/05 21:40:45, huangs1 wrote:
> On 2013/03/05 21:18:24, grt wrote:
> > On 2013/03/05 17:04:57, huangs1 wrote:
> > > Wait, installer::LaunchUserExperiment() only has 2 callers in
setup_main.cc,
> > and
> > > is not likely to get more.  I really think it suffices to let the caller
> check
> > > dist->HasUserExperiments(), then make the call to
> > > installer::LaunchUserExperiment().  If we insist on a wrapper, then
> > semantically
> > > "Product::LaunchUserExperiment()" would be a misnomer, since it is
possible
> > that
> > > nothing gets launched.
> > 
> > I think it's fine for "if the product and distribution support experiments"
to
> > be implied. If you search through the codebase I imagine you'll find many
> > instances of virtual methods that are noops under some circumstances.
> 
> Okay, no problem with naming.  I also agree that having caller perform policy
> check before calling is bad, so we need to do the check and call at exactly
one
> place in the code -- but this can be a stand-alone routine or in Product.  I'm
> fine either way, really.
> 
> > > If we insist on not making caller decide then we can have a stand-alone
> > > "LaunchUserExperimentIfRequired()" wrapper that lives in setup_main.cc or
> > > user_experiments.cc.
> > 
> > We have an installer that during its general flow of installing products has
a
> > point where product-specific experiments should be launched. The installer
> > itself has no reason to know what products run experiments or what those
> > experiments are. I think the current code in InstallProductsHelper makes
> sense:
> > loop over all products and fire off their respective experiments. The open
> > questions, as far as I can tell, are: 1) where does the experiment code
live,
> > and 2) where does the policy live.
> > 
> > For #1: We have a product-specific virtual dispatch mechanism in place (via
> the
> > Product/ProductOperations pair). It makes sense to me that a
browser-specific
> > experiment be launched through the ChromeBrowserOperations class. If we
decide
> > to make an experiment for the app launcher someday (as an example), its
> > implementation would naturally be in the appropriate Operations class. Do
you
> > guys agree on this?
> 
> If there are drastically different sets of experiments, then virtual dispatch
> make perfect sense.  But right now there's only one experiment (shared between
> Chrome and ChromeSxS).  I think trying to use virtual dispatch now would be
> over-designing; we can do this when/if the need arises.

I disagree. The code you are refactoring uses virtual dispatch and is neither
ugly nor overkill (although that's subjective, so maybe you disagree). If you
remove the virtual dispatch you're left with both runtime and compile-time
conditionals to only call the experiment for Chrome and Canary. This, imo, is
quite a bit uglier.

> > For #2: It's fragile to expect all callers to make a policy check before
> > invoking a function. This is bound to get messed up someday and it's ugly.
The
> > policy check could either be at the top of the impl in
ChromeBrowserOperations
> > (which I don't like because of my aversion to mixing policy and
> implementation),
> > or in Product since that's the only path for a caller to get to a
> > ProductOperations's experiment. Do you guys feel strongly one way or
another.
> If
> > so, why?
> 
> Again, having distributed policy check is bad.  I'd prefer to have the check
and
> the call (directly to UserExperiments) in Product, and leave ProductOperations
> alone.  The assumption I'm making is:
> - Product serves as a glue code, not just wrapper for operations_ and
> istribution_.
> - A single check-and-call, as seen in Product::LaunchUserExperiment() in Patch
> Set 13, is acceptable as glue; there's no need to go one layer deeper, and
> dispatch the call in ProductOperations' sub classes (while passing multiple
> parameters, requiring more #includes, etc.)

I dislike the practice of adding more compile time and/or runtime conditionals
in the installer.

[huangs, 2013-03-05 21:58:45]

Okay, new plan:

- Replace UserExperiment::LaunchUserExperiment() parameter Product with what
it's used for; 3 things:
  1. BrowserDistribution to get name for logs.
  2. ProductOperation for AppendProductFlags()
  3. FilePath, to replace need for product.GetUserDataPaths().
- Add ProductOperations::LaunchUserExperiment() with the same interface.
- Implement ProductOperations in each product (just 5), with 3 dummies and 2
pass-throughs.
- Keep policy checking in Product, but now call ProductOperations routine.

Note that I won't worry about ProductOperationsImpl now.  Is the plan
reasonable?

[huangs, 2013-03-07 20:57:15]

Changes:
- To have installer::LaunchUserExperiment() not take Product, the parameters got
streamlined.  In particular, we pass |base_command|, which has the .exe with the
product-specific flags added already.
- Product::GetUserDataPaths() is copied to BrowserDistribution (but not deleted
to maintain interface).

PTAL.

[grt (UTC plus 2), 2013-03-12 14:32:18]

i like this a lot more. almost there!

https://codereview.chromium.org/12321061/diff/49001/chrome/installer/util/bro...
File chrome/installer/util/browser_distribution.h (right):

https://codereview.chromium.org/12321061/diff/49001/chrome/installer/util/bro...
chrome/installer/util/browser_distribution.h:137: virtual bool
HasUserExperiments() const;
although it pains me to say it, un-constify this for consistency with the rest
of the class.

https://codereview.chromium.org/12321061/diff/49001/chrome/installer/util/chr...
File chrome/installer/util/chrome_app_host_operations.cc (right):

https://codereview.chromium.org/12321061/diff/49001/chrome/installer/util/chr...
chrome/installer/util/chrome_app_host_operations.cc:129: NOTREACHED() << "Chrome
App Launcher does not launch user experiments.";
please switch to:
  // Explanatory comment.
  NOTREACHED();
here and elsewhere. i am now of the mind that these strings on NOTREACHED() add
no value. thanks.

https://codereview.chromium.org/12321061/diff/49001/chrome/installer/util/goo...
File chrome/installer/util/google_chrome_sxs_distribution.h (right):

https://codereview.chromium.org/12321061/diff/49001/chrome/installer/util/goo...
chrome/installer/util/google_chrome_sxs_distribution.h:34: virtual bool
HasUserExperiments() const OVERRIDE;
this override is unnecessary. have you added it just to make it explicit?

https://codereview.chromium.org/12321061/diff/49001/chrome/installer/util/use...
File chrome/installer/util/user_experiment.cc (right):

https://codereview.chromium.org/12321061/diff/49001/chrome/installer/util/use...
chrome/installer/util/user_experiment.cc:106: bool system_level_toast) {
nit: move this to the previous line.

https://codereview.chromium.org/12321061/diff/49001/chrome/installer/util/use...
chrome/installer/util/user_experiment.cc:110: if
(current_cmd_line.HasSwitch(installer::switches::kVerboseLogging))
remove "installer::" everywhere possible since this is now in the installer
namespace (and adjust indentation where necessary).

https://codereview.chromium.org/12321061/diff/49001/chrome/installer/util/use...
chrome/installer/util/user_experiment.cc:279: int flavor,
nit: un-wrap this and the next line.

https://codereview.chromium.org/12321061/diff/49001/chrome/installer/util/use...
chrome/installer/util/user_experiment.cc:388: void LaunchUserExperiment(const
CommandLine& base_cmd_line,
This function seems specific to the browser now. I think it's appropriate to
make this formal. How about renaming it to LaunchBrowserUserExperiment and don't
pass a BD, but rather use
GetSpecificDistribution(BrowserDistribution::CHROME_BROWSER) in the body?

https://codereview.chromium.org/12321061/diff/49001/chrome/installer/util/use...
chrome/installer/util/user_experiment.cc:392: VLOG(1) << "LaunchUserExperiment
status: " << status << " product: "
I think it makes sense to move this logging into product.cc's
LaunchUserExperiment so that it doesn't need to appear in the per-product
implementations.

https://codereview.chromium.org/12321061/diff/49001/chrome/installer/util/use...
chrome/installer/util/user_experiment.cc:439:
dist->GetUserDataPaths(&user_data_dirs);
Please call GetChromeUserDataPaths from helper.h directly here and revert the
addition of this function to BD.

https://codereview.chromium.org/12321061/diff/49001/chrome/installer/util/use...
chrome/installer/util/user_experiment.cc:541: dist->GetType()));
use installation.distribution()->GetType() here and remove |dist| from the
arguments to this function.

https://codereview.chromium.org/12321061/diff/49001/chrome/installer/util/use...
File chrome/installer/util/user_experiment.h (right):

https://codereview.chromium.org/12321061/diff/49001/chrome/installer/util/use...
chrome/installer/util/user_experiment.h:12: #include
"chrome/installer/util/browser_distribution.h"
remove this or the forward decl on line 15.

https://codereview.chromium.org/12321061/diff/49001/chrome/installer/util/use...
chrome/installer/util/user_experiment.h:53: ExperimentDetails* experiment);
un-wrap

[huangs, 2013-03-12 18:03:07]

PTAL.

https://chromiumcodereview.appspot.com/12321061/diff/49001/chrome/installer/u...
File chrome/installer/util/browser_distribution.h (right):

https://chromiumcodereview.appspot.com/12321061/diff/49001/chrome/installer/u...
chrome/installer/util/browser_distribution.h:137: virtual bool
HasUserExperiments() const;
On 2013/03/12 14:32:18, grt wrote:
> although it pains me to say it, un-constify this for consistency with the rest
> of the class.

Done.

https://chromiumcodereview.appspot.com/12321061/diff/49001/chrome/installer/u...
File chrome/installer/util/chrome_app_host_operations.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/49001/chrome/installer/u...
chrome/installer/util/chrome_app_host_operations.cc:129: NOTREACHED() << "Chrome
App Launcher does not launch user experiments.";
On 2013/03/12 14:32:18, grt wrote:
> please switch to:
>   // Explanatory comment.
>   NOTREACHED();
> here and elsewhere. i am now of the mind that these strings on NOTREACHED()
add
> no value. thanks.

Done.

https://chromiumcodereview.appspot.com/12321061/diff/49001/chrome/installer/u...
File chrome/installer/util/google_chrome_sxs_distribution.h (right):

https://chromiumcodereview.appspot.com/12321061/diff/49001/chrome/installer/u...
chrome/installer/util/google_chrome_sxs_distribution.h:34: virtual bool
HasUserExperiments() const OVERRIDE;
On 2013/03/12 14:32:18, grt wrote:
> this override is unnecessary. have you added it just to make it explicit?

Yes, to assert that Canary also runs the experiment (as intended in the original
code).

https://chromiumcodereview.appspot.com/12321061/diff/49001/chrome/installer/u...
File chrome/installer/util/user_experiment.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/49001/chrome/installer/u...
chrome/installer/util/user_experiment.cc:106: bool system_level_toast) {
On 2013/03/12 14:32:18, grt wrote:
> nit: move this to the previous line.

Done.

https://chromiumcodereview.appspot.com/12321061/diff/49001/chrome/installer/u...
chrome/installer/util/user_experiment.cc:106: bool system_level_toast) {
On 2013/03/12 14:32:18, grt wrote:
> nit: move this to the previous line.

Done.

https://chromiumcodereview.appspot.com/12321061/diff/49001/chrome/installer/u...
chrome/installer/util/user_experiment.cc:110: if
(current_cmd_line.HasSwitch(installer::switches::kVerboseLogging))
Done, with line wraps.

https://chromiumcodereview.appspot.com/12321061/diff/49001/chrome/installer/u...
chrome/installer/util/user_experiment.cc:279: int flavor,
On 2013/03/12 14:32:18, grt wrote:
> nit: un-wrap this and the next line.

Done.

https://chromiumcodereview.appspot.com/12321061/diff/49001/chrome/installer/u...
chrome/installer/util/user_experiment.cc:388: void LaunchUserExperiment(const
CommandLine& base_cmd_line,
Done.  Since BD is no longer required, also removing it as parameter for
PO::LauncherUserExperiment().

https://chromiumcodereview.appspot.com/12321061/diff/49001/chrome/installer/u...
chrome/installer/util/user_experiment.cc:392: VLOG(1) << "LaunchUserExperiment
status: " << status << " product: "
Done.

https://chromiumcodereview.appspot.com/12321061/diff/49001/chrome/installer/u...
chrome/installer/util/user_experiment.cc:439:
dist->GetUserDataPaths(&user_data_dirs);
On 2013/03/12 14:32:18, grt wrote:
> Please call GetChromeUserDataPaths from helper.h directly here and revert the
> addition of this function to BD.

Done.

https://chromiumcodereview.appspot.com/12321061/diff/49001/chrome/installer/u...
File chrome/installer/util/user_experiment.h (right):

https://chromiumcodereview.appspot.com/12321061/diff/49001/chrome/installer/u...
chrome/installer/util/user_experiment.h:12: #include
"chrome/installer/util/browser_distribution.h"
Moved to .cc file.

https://chromiumcodereview.appspot.com/12321061/diff/49001/chrome/installer/u...
chrome/installer/util/user_experiment.h:53: ExperimentDetails* experiment);
On 2013/03/12 14:32:18, grt wrote:
> un-wrap

Done.

[grt (UTC plus 2), 2013-03-12 19:36:18]

Please be sure to test that the toast still works. Here are some instructions
for testing system-level toasts:

https://docs.google.com/document/d/1rDnDbzzAeR-4Wkq2_kL-La3oQE2bEOfKSKeMhVUeU...

https://chromiumcodereview.appspot.com/12321061/diff/49001/chrome/installer/u...
File chrome/installer/util/user_experiment.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/49001/chrome/installer/u...
chrome/installer/util/user_experiment.cc:541: dist->GetType()));
On 2013/03/12 14:32:18, grt wrote:
> use installation.distribution()->GetType() here and remove |dist| from the
> arguments to this function.

ping

https://chromiumcodereview.appspot.com/12321061/diff/56001/chrome/browser/fir...
File chrome/browser/first_run/try_chrome_dialog_view.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/56001/chrome/browser/fir...
chrome/browser/first_run/try_chrome_dialog_view.cc:178: if
(!installer::CreateExperimentDetails(flavor_, &experiment) ||
  if (!BrowserDistribution::GetDistribution()->HasUserExperiments() ||
      !installer::CreateExperimentDetails(flavor_, &experiment) ||
      !experiment.heading) {

https://chromiumcodereview.appspot.com/12321061/diff/56001/chrome/installer/u...
File chrome/installer/util/chrome_browser_operations.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/56001/chrome/installer/u...
chrome/installer/util/chrome_browser_operations.cc:145: BrowserDistribution*
dist = BrowserDistribution::GetSpecificDistribution(
please move into the if(){} block in product.cc as per my original request.

https://chromiumcodereview.appspot.com/12321061/diff/56001/chrome/installer/u...
File chrome/installer/util/user_experiment.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/56001/chrome/installer/u...
chrome/installer/util/user_experiment.cc:255:
base::StringToInt(cmd_line.GetSwitchValueASCII(switches::kToastResultsKey)
i think this is more readable like this:
      base::StringToInt(
          cmd_line.GetSwitchValueNative(switches::kToastResultsKey),
          &reg_key_handle);
i think neither the conversion to ASCII nor the call to c_str() is needed.

https://chromiumcodereview.appspot.com/12321061/diff/56001/chrome/installer/u...
chrome/installer/util/user_experiment.cc:387: BrowserDistribution* dist =
BrowserDistribution::GetSpecificDistribution(
move this closer to line 432 where it's used.

[huangs, 2013-03-14 17:20:44]

Also verified that the System-level toast appears.  PTAL.

https://chromiumcodereview.appspot.com/12321061/diff/49001/chrome/installer/u...
File chrome/installer/util/user_experiment.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/49001/chrome/installer/u...
chrome/installer/util/user_experiment.cc:541: dist->GetType()));
Sorry, missed a spot.  Done.

https://chromiumcodereview.appspot.com/12321061/diff/56001/chrome/browser/fir...
File chrome/browser/first_run/try_chrome_dialog_view.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/56001/chrome/browser/fir...
chrome/browser/first_run/try_chrome_dialog_view.cc:178: if
(!installer::CreateExperimentDetails(flavor_, &experiment) ||
On 2013/03/12 19:36:18, grt wrote:
>   if (!BrowserDistribution::GetDistribution()->HasUserExperiments() ||
>       !installer::CreateExperimentDetails(flavor_, &experiment) ||
>       !experiment.heading) {

Done.

https://chromiumcodereview.appspot.com/12321061/diff/56001/chrome/installer/u...
File chrome/installer/util/chrome_browser_operations.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/56001/chrome/installer/u...
chrome/installer/util/chrome_browser_operations.cc:145: BrowserDistribution*
dist = BrowserDistribution::GetSpecificDistribution(
On 2013/03/12 19:36:18, grt wrote:
> please move into the if(){} block in product.cc as per my original request.

Done.  Sorry, misread!

https://chromiumcodereview.appspot.com/12321061/diff/56001/chrome/installer/u...
File chrome/installer/util/user_experiment.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/56001/chrome/installer/u...
chrome/installer/util/user_experiment.cc:255:
base::StringToInt(cmd_line.GetSwitchValueASCII(switches::kToastResultsKey)
On 2013/03/12 19:36:18, grt wrote:
> i think this is more readable like this:
>       base::StringToInt(
>           cmd_line.GetSwitchValueNative(switches::kToastResultsKey),
>           &reg_key_handle);
> i think neither the conversion to ASCII nor the call to c_str() is needed.

Done.

https://chromiumcodereview.appspot.com/12321061/diff/56001/chrome/installer/u...
chrome/installer/util/user_experiment.cc:387: BrowserDistribution* dist =
BrowserDistribution::GetSpecificDistribution(
On 2013/03/12 19:36:18, grt wrote:
> move this closer to line 432 where it's used.

Done.  Not inlining, as that doesn't seem to reduce # of lines.

[grt (UTC plus 2), 2013-03-14 18:59:35]

lgtm. nice work, sam.

[huangs, 2013-03-14 23:12:41]

OWNER review to ben@ for

chrome/chrome_installer_util.gypi

[grt (UTC plus 2), 2013-03-15 13:21:27]

Hi Sam. Since the chrome/ change is just to the .gypi file, you can TBR one of
the chrome OWNERS. Please make it build before trying to commit, though.

https://chromiumcodereview.appspot.com/12321061/diff/49028/chrome/installer/u...
File chrome/installer/util/user_experiment.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/49028/chrome/installer/u...
chrome/installer/util/user_experiment.cc:7: #include <sddl.h>
i think you need #include <windows.h> before this line.

[huangs, 2013-03-15 14:33:46]

Yup, I always check.  :)

[huangs, 2013-03-15 14:56:05]

Ah I see.  Strange that it compiles on my machine.  Let me double check.

[huangs, 2013-03-15 15:14:12]

https://chromiumcodereview.appspot.com/12321061/diff/49028/chrome/installer/u...
File chrome/installer/util/user_experiment.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/49028/chrome/installer/u...
chrome/installer/util/user_experiment.cc:7: #include <sddl.h>
On 2013/03/15 13:21:28, grt wrote:
> i think you need #include <windows.h> before this line.

Done.

[commit-bot: I haz the power, 2013-03-15 16:06:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/huangs@chromium.org/12321061/88001

[commit-bot: I haz the power, 2013-03-15 16:06:54]

Presubmit check for 12321061-88001 failed and returned exit status 1.

INFO:root:Found 23 file(s).

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

** Presubmit Messages **
--tbr was specified, skipping OWNERS check

** Presubmit Warnings **
New code should not use wstrings.  If you are calling a cross-platform API that
accepts a wstring, fix the API.
    chrome/installer/util/chrome_app_host_operations.cc:125
    chrome/installer/util/chrome_app_host_operations.h:55
    chrome/installer/util/chrome_binaries_operations.cc:89
    chrome/installer/util/chrome_binaries_operations.h:55
    chrome/installer/util/chrome_browser_operations.cc:142
    chrome/installer/util/chrome_browser_operations.h:54
    chrome/installer/util/chrome_frame_operations.cc:157
    chrome/installer/util/chrome_frame_operations.h:54
    chrome/installer/util/product_operations.h:90

Presubmit checks took 10.5s to calculate.

[huangs, 2013-03-15 20:39:09]

This is to eliminate presubmit error.

Note that
#include "base/string16.h"

is the only include (the other dependencies are transitive).  PTAL.

[commit-bot: I haz the power, 2013-03-15 20:39:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/huangs@chromium.org/12321061/93001

[commit-bot: I haz the power, 2013-03-15 22:49:14]

Change committed as 188494

[gab, 2013-03-21 14:53:28]

Post-commit comments (sorry was on vacation last week).

Cheers!
Gab

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/s...
File chrome/installer/setup/setup_main.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/s...
chrome/installer/setup/setup_main.cc:1432:
installer::InactiveUserToastExperiment(
This will now be called for all products (it used to only be via the
GoogleChromeDistribution)... why doesn't this go through |product| like
LaunchUserExperiment()?

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/u...
File chrome/installer/util/user_experiment.cc (left):

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/u...
chrome/installer/util/user_experiment.cc:238:
product.AppendProductFlags(&cmd_line);
Why are you no longer adding product flags?

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/u...
File chrome/installer/util/user_experiment.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/u...
chrome/installer/util/user_experiment.cc:197: bool
LaunchSetupAsConsoleUser(CommandLine* cmd_line) {
I do not like this change, this method used to take only const params, now it
takes an out-param (modifiable pointer) although it only needs to modify it for
internal use...

Perhaps take a const CommandLine& base_command_line; param which you can copy to
some internal CommandLine object to augment internally (as you are doing in
LaunchBrowserUserExperiment()).

I don't find the argument of re-using the same object to save a copy is valid to
use an unnecessary out-param.

The only other option I see to avoid a copy is to receive a
scoped_ptr<CommandLine> which callers explicitly hand you via .Pass(), but I
don't think there is precedence for this paradigm in Chrome.

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/u...
chrome/installer/util/user_experiment.cc:398: if ((NEW_VERSION_UPDATED !=
status) && (REENTRY_SYS_UPDATE != status)) {
nit: 
if (status != NEW_VERSION_UPDATED && status != REENTRY_SYS_UPDATE)

(i.e., no need for the extra brackets and always put the constants on the RHS of
an equality check)

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/u...
chrome/installer/util/user_experiment.cc:432:
BrowserDistribution::CHROME_BROWSER);
I know that only the chrome_product_operations call into this method for now,
but other products (e.g., Chrome Frame) could call this one day no?

Why hardcode the distribution type here instead of using
product.GetUserDataPaths() as before?

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/u...
chrome/installer/util/user_experiment.cc:487: const Product& installation,
nit: Why not call this parameter |product| like all other Product params (avoids
having to look up at the params when reading the impl; which I just had to do)?

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/u...
chrome/installer/util/user_experiment.cc:534:
installation.distribution()->GetType()));
I find this indentation hard to read, I think this fits if you replace
|installation| by |product| as suggested above:
   CommandLine cmd(InstallUtil::GetChromeUninstallCmd(
                       system_level_toast, product.distribution()->GetType()));

[grt (UTC plus 2), 2013-03-21 17:23:49]

Good lookin' out, Gab. Here are my comments on your comments since I had some of
the same thoughts during my review(s).

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/s...
File chrome/installer/setup/setup_main.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/s...
chrome/installer/setup/setup_main.cc:1432:
installer::InactiveUserToastExperiment(
On 2013/03/21 14:53:28, gab wrote:
> This will now be called for all products (it used to only be via the
> GoogleChromeDistribution)... why doesn't this go through |product| like
> LaunchUserExperiment()?

This will only be called for products that had gone through
product.LaunchUserExperiment (which relaunch setup.exe with
--inactive-user-toast), so there's no real need to do it again.

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/u...
File chrome/installer/util/user_experiment.cc (left):

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/u...
chrome/installer/util/user_experiment.cc:238:
product.AppendProductFlags(&cmd_line);
On 2013/03/21 14:53:28, gab wrote:
> Why are you no longer adding product flags?

They're added earlier.

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/u...
File chrome/installer/util/user_experiment.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/u...
chrome/installer/util/user_experiment.cc:197: bool
LaunchSetupAsConsoleUser(CommandLine* cmd_line) {
On 2013/03/21 14:53:28, gab wrote:
> I do not like this change, this method used to take only const params, now it
> takes an out-param (modifiable pointer) although it only needs to modify it
for
> internal use...

I wrestled with this a bit, too, but decided it was okay since it's consistent
with LaunchSetup above. I didn't want to delay Sam's commit by asking him to
rejigger more. :-(

> Perhaps take a const CommandLine& base_command_line; param which you can copy
to
> some internal CommandLine object to augment internally (as you are doing in
> LaunchBrowserUserExperiment()).
> 
> I don't find the argument of re-using the same object to save a copy is valid
to
> use an unnecessary out-param.
> 
> The only other option I see to avoid a copy is to receive a
> scoped_ptr<CommandLine> which callers explicitly hand you via .Pass(), but I
> don't think there is precedence for this paradigm in Chrome.

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/u...
chrome/installer/util/user_experiment.cc:432:
BrowserDistribution::CHROME_BROWSER);
On 2013/03/21 14:53:28, gab wrote:
> I know that only the chrome_product_operations call into this method for now,
> but other products (e.g., Chrome Frame) could call this one day no?
> 
> Why hardcode the distribution type here instead of using
> product.GetUserDataPaths() as before?

I think this code is sufficiently Chrome-the-browser specific that this is okay
to do here.

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/u...
chrome/installer/util/user_experiment.cc:487: const Product& installation,
On 2013/03/21 14:53:28, gab wrote:
> nit: Why not call this parameter |product| like all other Product params
(avoids
> having to look up at the params when reading the impl; which I just had to
do)?

History.

[huangs, 2013-03-21 17:31:25]

Thanks for the comments.  After I push out my big code review to erikwright@
today, I'll be able to do some rejiggling.

[gab, 2013-03-21 19:59:07]

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/s...
File chrome/installer/setup/setup_main.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/s...
chrome/installer/setup/setup_main.cc:1432:
installer::InactiveUserToastExperiment(
On 2013/03/21 17:23:49, grt wrote:
> On 2013/03/21 14:53:28, gab wrote:
> > This will now be called for all products (it used to only be via the
> > GoogleChromeDistribution)... why doesn't this go through |product| like
> > LaunchUserExperiment()?
> 
> This will only be called for products that had gone through
> product.LaunchUserExperiment (which relaunch setup.exe with
> --inactive-user-toast), so there's no real need to do it again.

Ah I see, the code review tool was hiding the lines above so I didn't realize
this was inside that flag... perhaps add a comment above this line to make sure
no one else is further puzzled by this?

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/u...
File chrome/installer/util/user_experiment.cc (left):

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/u...
chrome/installer/util/user_experiment.cc:238:
product.AppendProductFlags(&cmd_line);
On 2013/03/21 17:23:49, grt wrote:
> On 2013/03/21 14:53:28, gab wrote:
> > Why are you no longer adding product flags?
> 
> They're added earlier.

Where? The caller of LaunchSetupAsConsoleUser() doesn't seem to add this.

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/u...
File chrome/installer/util/user_experiment.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/u...
chrome/installer/util/user_experiment.cc:197: bool
LaunchSetupAsConsoleUser(CommandLine* cmd_line) {
On 2013/03/21 17:23:49, grt wrote:
> On 2013/03/21 14:53:28, gab wrote:
> > I do not like this change, this method used to take only const params, now
it
> > takes an out-param (modifiable pointer) although it only needs to modify it
> for
> > internal use...
> 
> I wrestled with this a bit, too, but decided it was okay since it's consistent
> with LaunchSetup above. I didn't want to delay Sam's commit by asking him to
> rejigger more. :-(
> 
> > Perhaps take a const CommandLine& base_command_line; param which you can
copy
> to
> > some internal CommandLine object to augment internally (as you are doing in
> > LaunchBrowserUserExperiment()).
> > 
> > I don't find the argument of re-using the same object to save a copy is
valid
> to
> > use an unnecessary out-param.
> > 
> > The only other option I see to avoid a copy is to receive a
> > scoped_ptr<CommandLine> which callers explicitly hand you via .Pass(), but I
> > don't think there is precedence for this paradigm in Chrome.

Hmmm ok, but I don't think existing bad code is an excuse for allowing its style
:).

At least the big patch is in, I'd appreciate if this could be addressed in the
follow-up CL.

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/u...
chrome/installer/util/user_experiment.cc:432:
BrowserDistribution::CHROME_BROWSER);
On 2013/03/21 17:23:49, grt wrote:
> On 2013/03/21 14:53:28, gab wrote:
> > I know that only the chrome_product_operations call into this method for
now,
> > but other products (e.g., Chrome Frame) could call this one day no?
> > 
> > Why hardcode the distribution type here instead of using
> > product.GetUserDataPaths() as before?
> 
> I think this code is sufficiently Chrome-the-browser specific that this is
okay
> to do here.

Sure, especially that it was renamed to LaunchBrowserUserExperiments(). We
already have the BrowserDistribution a couple of calls above though, couldn't we
simply pass it down... is it really worth saving a parameter to re-derive it
here..?

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/u...
chrome/installer/util/user_experiment.cc:487: const Product& installation,
On 2013/03/21 17:23:49, grt wrote:
> On 2013/03/21 14:53:28, gab wrote:
> > nit: Why not call this parameter |product| like all other Product params
> (avoids
> > having to look up at the params when reading the impl; which I just had to
> do)?
> 
> History.

Right, would be nice to correct it in a follow-up CL (if only to be able to
address the comment below...).

[grt (UTC plus 2), 2013-03-26 02:49:14]

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/u...
File chrome/installer/util/user_experiment.cc (left):

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/u...
chrome/installer/util/user_experiment.cc:238:
product.AppendProductFlags(&cmd_line);
On 2013/03/21 19:59:07, gab wrote:
> On 2013/03/21 17:23:49, grt wrote:
> > On 2013/03/21 14:53:28, gab wrote:
> > > Why are you no longer adding product flags?
> > 
> > They're added earlier.
> 
> Where? The caller of LaunchSetupAsConsoleUser() doesn't seem to add this.

See ChromeBrowserOperations::LaunchUserExperiment.

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/u...
File chrome/installer/util/user_experiment.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/u...
chrome/installer/util/user_experiment.cc:432:
BrowserDistribution::CHROME_BROWSER);
On 2013/03/21 19:59:07, gab wrote:
> On 2013/03/21 17:23:49, grt wrote:
> > On 2013/03/21 14:53:28, gab wrote:
> > > I know that only the chrome_product_operations call into this method for
> now,
> > > but other products (e.g., Chrome Frame) could call this one day no?
> > > 
> > > Why hardcode the distribution type here instead of using
> > > product.GetUserDataPaths() as before?
> > 
> > I think this code is sufficiently Chrome-the-browser specific that this is
> okay
> > to do here.
> 
> Sure, especially that it was renamed to LaunchBrowserUserExperiments(). We
> already have the BrowserDistribution a couple of calls above though, couldn't
we
> simply pass it down... is it really worth saving a parameter to re-derive it
> here..?

My logic was thus: this is a browser-specific function. It doesn't make sense to
invoke it for other types of distributions, and would actually be harmful to do
so.  The use of BD here is an implementation detail rather than part of the
function's interface. Hence, grab it rather than take it as an in-param. To get
rid of the grab at global state, an alternative to consider would be to make the
data dir(s) part of the func's interface. The caller (which has the BD) can be
responsible for calling GetChromeUserDataPaths. This is just off the top of my
head, so take it with a grain.

[gab, 2013-03-26 15:02:00]

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/u...
File chrome/installer/util/user_experiment.cc (left):

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/u...
chrome/installer/util/user_experiment.cc:238:
product.AppendProductFlags(&cmd_line);
On 2013/03/26 02:49:15, grt wrote:
> On 2013/03/21 19:59:07, gab wrote:
> > On 2013/03/21 17:23:49, grt wrote:
> > > On 2013/03/21 14:53:28, gab wrote:
> > > > Why are you no longer adding product flags?
> > > 
> > > They're added earlier.
> > 
> > Where? The caller of LaunchSetupAsConsoleUser() doesn't seem to add this.
> 
> See ChromeBrowserOperations::LaunchUserExperiment.

Aaah, makes sense that it'd be there :)!

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/u...
File chrome/installer/util/user_experiment.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/u...
chrome/installer/util/user_experiment.cc:432:
BrowserDistribution::CHROME_BROWSER);
On 2013/03/26 02:49:15, grt wrote:
> On 2013/03/21 19:59:07, gab wrote:
> > On 2013/03/21 17:23:49, grt wrote:
> > > On 2013/03/21 14:53:28, gab wrote:
> > > > I know that only the chrome_product_operations call into this method for
> > now,
> > > > but other products (e.g., Chrome Frame) could call this one day no?
> > > > 
> > > > Why hardcode the distribution type here instead of using
> > > > product.GetUserDataPaths() as before?
> > > 
> > > I think this code is sufficiently Chrome-the-browser specific that this is
> > okay
> > > to do here.
> > 
> > Sure, especially that it was renamed to LaunchBrowserUserExperiments(). We
> > already have the BrowserDistribution a couple of calls above though,
couldn't
> we
> > simply pass it down... is it really worth saving a parameter to re-derive it
> > here..?
> 
> My logic was thus: this is a browser-specific function. It doesn't make sense
to
> invoke it for other types of distributions, and would actually be harmful to
do
> so.  The use of BD here is an implementation detail rather than part of the
> function's interface. Hence, grab it rather than take it as an in-param. To
get
> rid of the grab at global state, an alternative to consider would be to make
the
> data dir(s) part of the func's interface. The caller (which has the BD) can be
> responsible for calling GetChromeUserDataPaths. This is just off the top of my
> head, so take it with a grain.

Ok, your proposal sounds good, but I don't think it's worth spending much time
on if we are sure it always makes sense for the distribution to be
CHROME_BROWSER here.

[huangs, 2013-03-26 22:00:24]

Follow-up review for the simpler changes is here

https://codereview.chromium.org/12998005/

The more complex changes (re. user experiments and cmd_line) will be done in a
different CL.

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/s...
File chrome/installer/setup/setup_main.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/s...
chrome/installer/setup/setup_main.cc:1432:
installer::InactiveUserToastExperiment(
Added comments (in new CL).

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/u...
File chrome/installer/util/user_experiment.cc (left):

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/u...
chrome/installer/util/user_experiment.cc:238:
product.AppendProductFlags(&cmd_line);
No-op.

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/u...
File chrome/installer/util/user_experiment.cc (right):

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/u...
chrome/installer/util/user_experiment.cc:197: bool
LaunchSetupAsConsoleUser(CommandLine* cmd_line) {
I think the entire UserExperiment stuff should be cleaned up. But I want to do
low-risk changes first to get them out of the way!

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/u...
chrome/installer/util/user_experiment.cc:398: if ((NEW_VERSION_UPDATED !=
status) && (REENTRY_SYS_UPDATE != status)) {
On 2013/03/21 14:53:28, gab wrote:
> nit: 
> if (status != NEW_VERSION_UPDATED && status != REENTRY_SYS_UPDATE)
> 
> (i.e., no need for the extra brackets and always put the constants on the RHS
of
> an equality check)

Done.

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/u...
chrome/installer/util/user_experiment.cc:432:
BrowserDistribution::CHROME_BROWSER);
No-op.

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/u...
chrome/installer/util/user_experiment.cc:487: const Product& installation,
Done (since this is a smaller CL, should be harmless).

https://chromiumcodereview.appspot.com/12321061/diff/93001/chrome/installer/u...
chrome/installer/util/user_experiment.cc:534:
installation.distribution()->GetType()));
On 2013/03/21 14:53:28, gab wrote:
> I find this indentation hard to read, I think this fits if you replace
> |installation| by |product| as suggested above:
>    CommandLine cmd(InstallUtil::GetChromeUninstallCmd(
>                        system_level_toast,
product.distribution()->GetType()));

Done.