Implement an AutoLaunch experiment for Chrome for certain brand codes.

Implement an AutoLaunch experiment for Chrome for certain brand codes. This is for Windows only.

BUG=95971
TEST=This requires a special branded build to test (and master_preferences set to have auto_launch_chrome: true or specify mini_installer param --auto-launch-chrome), but after installing it you should see an item under the Wrench \ Options \ Basics that lets you configure Chrome to not auto-launch. If you instead, log out and log in again, Chrome should start with an infobar saying that it was auto-launched. The infobar should stay for a max of 5 launches and then not appear again. Pressing "Cut it out!" on the infobar should turn off this feature. If you just want to test the infobar, launch chrome with --auto-launch-at-startup. Also, uninstall and make sure Chrome is not auto-launched (a Windows error about Chrome not being found should not be shown).
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114621

[Finnur, 2011-11-29 18:14:41]

This is work in progress (needs some installer changes, hence looping in grt).

[grt (UTC plus 2), 2011-11-29 18:55:45]

LG.  A handful of early comments mostly related to the installer stuff.

http://codereview.chromium.org/8729009/diff/3004/chrome/browser/auto_launch_t...
File chrome/browser/auto_launch_trial.cc (right):

http://codereview.chromium.org/8729009/diff/3004/chrome/browser/auto_launch_t...
chrome/browser/auto_launch_trial.cc:39: } // namespace
}  // namespace auto_launch_trial

http://codereview.chromium.org/8729009/diff/3004/chrome/browser/auto_launch_t...
File chrome/browser/auto_launch_trial.h (right):

http://codereview.chromium.org/8729009/diff/3004/chrome/browser/auto_launch_t...
chrome/browser/auto_launch_trial.h:43: }
}  // namespace auto_launch_trial

http://codereview.chromium.org/8729009/diff/3004/chrome/browser/ui/browser_in...
File chrome/browser/ui/browser_init.cc (right):

http://codereview.chromium.org/8729009/diff/3004/chrome/browser/ui/browser_in...
chrome/browser/ui/browser_init.cc:131: explicit
AutolaunchInfoBarDelegate(InfoBarTabHelper* infobar_helper,
remove "explicit"

http://codereview.chromium.org/8729009/diff/3004/chrome/browser/ui/browser_in...
chrome/browser/ui/browser_init.cc:135: virtual ~AutolaunchInfoBarDelegate();
why private dtor?

http://codereview.chromium.org/8729009/diff/3004/chrome/browser/ui/browser_in...
chrome/browser/ui/browser_init.cc:170:
ALLOW_THIS_IN_INITIALIZER_LIST(method_factory_(this)) {
indent?

http://codereview.chromium.org/8729009/diff/3004/chrome/browser/ui/browser_in...
chrome/browser/ui/browser_init.cc:343: virtual void Run();
OVERRIDE

http://codereview.chromium.org/8729009/diff/3004/chrome/installer/setup/setup...
File chrome/installer/setup/setup_main.cc (right):

http://codereview.chromium.org/8729009/diff/3004/chrome/installer/setup/setup...
chrome/installer/setup/setup_main.cc:723: // Add auto-launch key if specified in
master preferences.
i think this belongs in install.cc's InstallOrUpdateProduct.

also, this will only be hit for new installs (not for updates).  is that
intended?

add corresponding code to remove the value in uninstall.cc's UninstallProduct. 
you may need to take care to remove it only if it indicates the
being-uninstalled Chrome (i.e., you wouldn't want to remove it if user-level
Chrome was being uninstalled and the AutoRun entry for the user pointed to a
system-level Chrome install).

http://codereview.chromium.org/8729009/diff/3004/chrome/installer/util/auto_l...
File chrome/installer/util/auto_launch_util.cc (right):

http://codereview.chromium.org/8729009/diff/3004/chrome/installer/util/auto_l...
chrome/installer/util/auto_launch_util.cc:16: static const wchar_t kRunKey[] =
can you use base::win::*AutoRun methods instead of making new ones here (see
base/win/win_util.h)?

http://codereview.chromium.org/8729009/diff/3004/chrome/installer/util/auto_l...
chrome/installer/util/auto_launch_util.cc:16: static const wchar_t kRunKey[] =
prefer the unnamed namespace over "static".  for example:

namespace {
const wchar_t kBlarch[] = L"..";
const wchar_t kFlamboozle[] = L"...";
}

http://codereview.chromium.org/8729009/diff/3004/chrome/installer/util/auto_l...
chrome/installer/util/auto_launch_util.cc:47: }  // namespace
}  // namespace auto_launch_util

http://codereview.chromium.org/8729009/diff/3004/chrome/installer/util/auto_l...
File chrome/installer/util/auto_launch_util.h (right):

http://codereview.chromium.org/8729009/diff/3004/chrome/installer/util/auto_l...
chrome/installer/util/auto_launch_util.h:11: // A file containing the platform
specific implementation of setting Chrome to
i believe file-level comments (which rock!) belong above the #ifndef mess.

http://codereview.chromium.org/8729009/diff/3004/chrome/installer/util/auto_l...
chrome/installer/util/auto_launch_util.h:16: bool IsAutoLaunched();
for some reason i can't quite articulate, i prefer "WillLaunchAtLogin" over
"IsAutoLaunched".

http://codereview.chromium.org/8729009/diff/3004/chrome/installer/util/auto_l...
chrome/installer/util/auto_launch_util.h:23: }  // namespace
}  // namespace auto_launch_util

http://codereview.chromium.org/8729009/diff/3004/chrome/installer/util/master...
File chrome/installer/util/master_preferences_constants.h (right):

http://codereview.chromium.org/8729009/diff/3004/chrome/installer/util/master...
chrome/installer/util/master_preferences_constants.h:25: extern const char
kAutoLaunchChrome[];
consider making a corresponding command line switch for the installer, and
adding this pair to the magic in
master_preferences.cc:InitializeFromCommandLine.

[Finnur, 2011-11-29 23:48:30]

I'm splitting this review in three between rogerta, stuartmorgan and grt, based
on areas of expertise...

rogerta, can you review the infobar and the use of field trials and uma?
chrome/browser/auto_launch_trial.h
chrome/browser/auto_launch_trial.cc
chrome/browser/chrome_browser_main.h
chrome/browser/chrome_browser_main.cc
chrome/browser/prefs/browser_prefs.cc
chrome/browser/ui/browser_init.h
chrome/browser/ui/browser_init.cc
chrome/chrome_browser.gypi
chrome/common/chrome_switches.h
chrome/common/chrome_switches.cc
chrome/common/pref_names.h
chrome/common/pref_names.cc

stuartmorgan, can you review the changes to the settings page?
chrome/app/generated_resources.grd
chrome/browser/resources/options/browser_options.html
chrome/browser/resources/options/browser_options.js
chrome/browser/ui/webui/options/browser_options_handler.h
chrome/browser/ui/webui/options/browser_options_handler.cc

grt, can you review the installer changes (and see my comments below)?
chrome/chrome_installer_util.gypi
chrome/installer/setup/install.cc
chrome/installer/setup/uninstall.cc
chrome/installer/util/auto_launch_util.h
chrome/installer/util/auto_launch_util.cc
chrome/installer/util/install_util.cc
chrome/installer/util/master_preferences.cc
chrome/installer/util/master_preferences_constants.h
chrome/installer/util/master_preferences_constants.cc
chrome/installer/util/util_constants.h
chrome/installer/util/util_constants.cc

http://codereview.chromium.org/8729009/diff/3004/chrome/browser/ui/browser_in...
File chrome/browser/ui/browser_init.cc (right):

http://codereview.chromium.org/8729009/diff/3004/chrome/browser/ui/browser_in...
chrome/browser/ui/browser_init.cc:135: virtual ~AutolaunchInfoBarDelegate();
Hmm... I don't have a good reason besides the other infobar in this class having
it. I guess I'll change it.

On 2011/11/29 18:55:45, grt wrote:
> why private dtor?

http://codereview.chromium.org/8729009/diff/3004/chrome/installer/setup/setup...
File chrome/installer/setup/setup_main.cc (right):

http://codereview.chromium.org/8729009/diff/3004/chrome/installer/setup/setup...
chrome/installer/setup/setup_main.cc:723: // Add auto-launch key if specified in
master preferences.
On 2011/11/29 18:55:45, grt wrote:
> i think this belongs in install.cc's InstallOrUpdateProduct.

Moved.

> 
> also, this will only be hit for new installs (not for updates).  is that
> intended?

Yes. This is only activated with new brand codes. I don't see the point in
drafting already existing users into the experiment.

> 
> add corresponding code to remove the value in uninstall.cc's UninstallProduct.

> you may need to take care to remove it only if it indicates the
> being-uninstalled Chrome (i.e., you wouldn't want to remove it if user-level
> Chrome was being uninstalled and the AutoRun entry for the user pointed to a
> system-level Chrome install).

We only put this in the Run key for the user installing the product, not all
users. So, I've added code to just remove it if it is found.

http://codereview.chromium.org/8729009/diff/3004/chrome/installer/util/auto_l...
File chrome/installer/util/auto_launch_util.h (right):

http://codereview.chromium.org/8729009/diff/3004/chrome/installer/util/auto_l...
chrome/installer/util/auto_launch_util.h:11: // A file containing the platform
specific implementation of setting Chrome to
Despite the word 'file' there, I see this as namespace-level comments, like
class-level comments. I'm worried that they get lost in the noise up at the
top... I've changed |file|->|namespace| to reflect this. Is that OK?

http://codereview.chromium.org/8729009/diff/3004/chrome/installer/util/auto_l...
chrome/installer/util/auto_launch_util.h:16: bool IsAutoLaunched();
I'm fine with that.

[Finnur, 2011-11-30 01:21:50]

grt: Two more for you (that I left out in the last CL):
base/win/win_util.h
base/win/win_util.cc

[Finnur, 2011-12-01 00:15:30]

ping?

On Tue, Nov 29, 2011 at 17:21, <finnur@chromium.org> wrote:

> grt: Two more for you (that I left out in the last CL):
> base/win/win_util.h
> base/win/win_util.cc
>
>
http://codereview.chromium.**org/8729009/<http://codereview.chromium.org/8729...
>

[grt (UTC plus 2), 2011-12-01 04:45:02]

Apologies for the delay.  Did you consider putting a shortcut in the user's
Start Menu\Programs\Startup directory (CSIDL_STARTUP) rather than using the
registry entry technique implemented here?  Doing so would be more transparent
and user-friendly, IMO.

http://codereview.chromium.org/8729009/diff/17001/base/win/win_util.h
File base/win/win_util.h (right):

http://codereview.chromium.org/8729009/diff/17001/base/win/win_util.h#newcode49
base/win/win_util.h:49: extern const wchar_t kRunKey[];
i think this is the wrong direction.  please see my comments in the impl in
install util for an alternative to making this external.

http://codereview.chromium.org/8729009/diff/17001/base/win/win_util.h#newcode51
base/win/win_util.h:51: // The name of the Chrome Auto-launch key under the Run
key.
move this back into install util.

http://codereview.chromium.org/8729009/diff/17001/chrome/browser/ui/browser_i...
File chrome/browser/ui/browser_init.cc (right):

http://codereview.chromium.org/8729009/diff/17001/chrome/browser/ui/browser_i...
chrome/browser/ui/browser_init.cc:223: FilePath chrome_exe;
chrome_exe -> chrome_dir, or somesuch

http://codereview.chromium.org/8729009/diff/17001/chrome/browser/ui/browser_i...
chrome/browser/ui/browser_init.cc:226:
auto_launch_util::SetWillLaunchAtLogin(auto_launch, chrome_exe);
this will hit the registry.  is IO allowed from this thread?

http://codereview.chromium.org/8729009/diff/17001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

http://codereview.chromium.org/8729009/diff/17001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/browser_options_handler.cc:495: // Make sure we
keep track of how many disable and how many enable.
this is almost duplicate code.  could it be shared better?

http://codereview.chromium.org/8729009/diff/17001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/browser_options_handler.cc:498: FilePath
chrome_exe;
chrome_exe -> chrome_dir, or somesuch

http://codereview.chromium.org/8729009/diff/17001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/browser_options_handler.cc:502:
auto_launch_util::SetWillLaunchAtLogin(enable, chrome_exe);
this will touch the registry.  is IO allowed from whatever thread this is
running in (UI?).

http://codereview.chromium.org/8729009/diff/17001/chrome/installer/setup/inst...
File chrome/installer/setup/install.cc (right):

http://codereview.chromium.org/8729009/diff/17001/chrome/installer/setup/inst...
chrome/installer/setup/install.cc:414: if (result == FIRST_INSTALL_SUCCESS) {
move this new code below the call to RegisterChromeOnMachine since it breaks the
flow leading up to RCOM as it is now.

it would also be groovy if you'd add a new call to UpdateStage with a
corresponding new stage identifier.

http://codereview.chromium.org/8729009/diff/17001/chrome/installer/setup/unin...
File chrome/installer/setup/uninstall.cc (right):

http://codereview.chromium.org/8729009/diff/17001/chrome/installer/setup/unin...
chrome/installer/setup/uninstall.cc:741:
auto_launch_util::SetWillLaunchAtLogin(false, FilePath());
this has the potential to remove a launch entry for a different chrome than the
one being uninstalled.  it's safer to only remove it if the path matches the
chrome being uninstalled.  here's one way it could do the wrong thing:
1. user has a pre-existing user-level chrome install
2. system-level chrome is magically installed on the machine
3. user runs system-level chrome and sets it to launch at login
4. user runs user-level chrome, which notices the existence of system-level and
self-destructs (it runs its own uninstaller)
5. user-level chrome's uninstaller nukes the user's auto-launch entry for the
system-level chrome
6. user no longer has the auto-launch love

this issue will be resolved if you take my advice and make WillLaunchAtLogin()
take the path to a Chrome, and only return true if that Chrome will launch at
login.

http://codereview.chromium.org/8729009/diff/17001/chrome/installer/util/auto_...
File chrome/installer/util/auto_launch_util.cc (right):

http://codereview.chromium.org/8729009/diff/17001/chrome/installer/util/auto_...
chrome/installer/util/auto_launch_util.cc:19: base::win::RegKey
key(HKEY_CURRENT_USER, base::win::kRunKey, KEY_QUERY_VALUE);
replace lines 19-23 with:
string16 autolaunch;
if (!base::win::ReadCommandFromAutoRun(HKEY_CURRENT_USER,
base::win::kAutolaunchKeyValue, &autolaunch))

http://codereview.chromium.org/8729009/diff/17001/chrome/installer/util/auto_...
chrome/installer/util/auto_launch_util.cc:27:
ASCIIToUTF16(switches::kAutoLaunchAtStartup)) != string16::npos;
why this extra check?  isn't using our own name for the value sufficient?

http://codereview.chromium.org/8729009/diff/17001/chrome/installer/util/auto_...
chrome/installer/util/auto_launch_util.cc:35: string16 cmd_line = L"\"";
i know this works on windows, but is combining wchar_t literals with string16's
recommended?  i think i've seen ASCIIToUTF16 used for this sort of thing.

http://codereview.chromium.org/8729009/diff/17001/chrome/installer/util/auto_...
chrome/installer/util/auto_launch_util.cc:41:
key.WriteValue(base::win::kAutolaunchKeyValue, cmd_line.c_str());
use base::win:: AddCommandToAutoRun

http://codereview.chromium.org/8729009/diff/17001/chrome/installer/util/auto_...
chrome/installer/util/auto_launch_util.cc:43:
key.DeleteValue(base::win::kAutolaunchKeyValue);
use base::win::RemoveCommandFromAutoRun

http://codereview.chromium.org/8729009/diff/17001/chrome/installer/util/auto_...
File chrome/installer/util/auto_launch_util.h (right):

http://codereview.chromium.org/8729009/diff/17001/chrome/installer/util/auto_...
chrome/installer/util/auto_launch_util.h:16: bool WillLaunchAtLogin();
i think this should take the path to Chrome and return true only if *that*
chrome will be launched at login.

http://codereview.chromium.org/8729009/diff/17001/chrome/installer/util/auto_...
chrome/installer/util/auto_launch_util.h:21: void SetWillLaunchAtLogin(bool
auto_launch, const FilePath& application_path);
in the current impl, |application_path| is the directory holding the binary.  i
think it's more natural for it to be the path to the binary (as above).  if you
take this suggestion, please ignore my chrome_exe -> chrome_dir comments
elsewhere, since you'll have to switch those to use FILE_EXE rather than
DIR_EXE.

http://codereview.chromium.org/8729009/diff/17001/chrome/installer/util/insta...
File chrome/installer/util/install_util.cc (right):

http://codereview.chromium.org/8729009/diff/17001/chrome/installer/util/insta...
chrome/installer/util/install_util.cc:177: KEY_QUERY_VALUE);
thank you!

[stuartmorgan, 2011-12-01 10:11:44]

http://codereview.chromium.org/8729009/diff/17001/chrome/browser/resources/op...
File chrome/browser/resources/options/browser_options.html (right):

http://codereview.chromium.org/8729009/diff/17001/chrome/browser/resources/op...
chrome/browser/resources/options/browser_options.html:124: <span
id="autoLaunchOption" hidden>
This should almost certainly be a <div class="checkbox"> rather than a span.

http://codereview.chromium.org/8729009/diff/17001/chrome/browser/resources/op...
chrome/browser/resources/options/browser_options.html:127: </span>
Labels need to use the <label> construct; see elsewhere in the file for
examples.

http://codereview.chromium.org/8729009/diff/17001/chrome/browser/resources/op...
File chrome/browser/resources/options/browser_options.js (right):

http://codereview.chromium.org/8729009/diff/17001/chrome/browser/resources/op...
chrome/browser/resources/options/browser_options.js:310: * Set or clear whether
Chrome should Auto-launch on computer startup.
s/Set/Sets/ (the previous two functions are incorrect)

http://codereview.chromium.org/8729009/diff/17001/chrome/browser/resources/op...
chrome/browser/resources/options/browser_options.js:344: * auto-launch option
should be visible and what its value should be.
This comment doesn't match the implementation. If it should be communicating
visible state, that should be a bool. If not, the comment should say what it
actually does, which is show it unconditionally.

Also, this comment does follow the style guide for comment syntax; it should
start with a verb; see the previous two functions for examples.

http://codereview.chromium.org/8729009/diff/17001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

http://codereview.chromium.org/8729009/diff/17001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/browser_options_handler.cc:499: if
(!PathService::Get(base::DIR_EXE, &chrome_exe))
Seems a bit odd to me that this code lives here and not in a helper; it's not
WebUI-specific, and if we have other UI for this, it'll be duplicated.

[Finnur, 2011-12-02 23:47:58]

rogerta: ping
stuartmorgan, grt: PTAL

grt: Your idea on adding a shortcut instead of the run key is a good one, but
since the user already has a user-friendly way of turning this off (checkbox in
first page of settings) and this is a limited experiment, I'd like to punt on
that in the interest of time.

[grt (UTC plus 2), 2011-12-03 04:43:38]

lookin' good.  if it makes it from experiment to feature i'll push for it to be
a shortcut in Programs/Startup since the Run key is black magic to most of the
population.  for now, though, i'm fine with it as-is.

http://codereview.chromium.org/8729009/diff/33002/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

http://codereview.chromium.org/8729009/diff/33002/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/browser_options_handler.cc:172:
base::FundamentalValue enabled(auto_launch_util::WillLaunchAtLogin());
i can't remember if i remarked that this will hit the registry, so maybe it,
too, needs to be done on the FILE thread.  won't this cause a DCHECK in the
thread checker thingamaboob?

http://codereview.chromium.org/8729009/diff/33002/chrome/installer/setup/unin...
File chrome/installer/setup/uninstall.cc (right):

http://codereview.chromium.org/8729009/diff/33002/chrome/installer/setup/unin...
chrome/installer/setup/uninstall.cc:740: if
(auto_launch_util::WillLaunchAtLogin())
this will always return false since the current executable is setup.exe a few
directories below where chrome.exe lives.

http://codereview.chromium.org/8729009/diff/33002/chrome/installer/util/auto_...
File chrome/installer/util/auto_launch_util.cc (right):

http://codereview.chromium.org/8729009/diff/33002/chrome/installer/util/auto_...
chrome/installer/util/auto_launch_util.cc:37: void SetWillLaunchAtLogin(bool
auto_launch, FilePath application_path) {
const FilePath&

http://codereview.chromium.org/8729009/diff/33002/chrome/installer/util/auto_...
File chrome/installer/util/auto_launch_util.h (right):

http://codereview.chromium.org/8729009/diff/33002/chrome/installer/util/auto_...
chrome/installer/util/auto_launch_util.h:15: // Returns whether Chrome is set to
auto-launch at computer startup.
// Returns whether the current executable is set to auto-launch at computer
startup.

http://codereview.chromium.org/8729009/diff/33002/chrome/installer/util/auto_...
chrome/installer/util/auto_launch_util.h:18: // Configures whether Chrome should
auto-launch at computer startup.
// Configures whether chrome.exe in |application_path| (or the directory of
current executable, if |application_path| is empty) should...

http://codereview.chromium.org/8729009/diff/33002/chrome/installer/util/auto_...
chrome/installer/util/auto_launch_util.h:23: void SetWillLaunchAtLogin(bool
auto_launch, FilePath application_path);
const FilePath&

[stuartmorgan, 2011-12-05 14:55:47]

Options LGTM

[Roger Tawa OOO till Jul 10th, 2011-12-07 15:38:37]

Sorry for the delay Finnur, I had lost track of this review.  Some
comments/questions below.

http://codereview.chromium.org/8729009/diff/33002/chrome/browser/auto_launch_...
File chrome/browser/auto_launch_trial.cc (right):

http://codereview.chromium.org/8729009/diff/33002/chrome/browser/auto_launch_...
chrome/browser/auto_launch_trial.cc:20: }
Shouldn't you also check the master prefs file here too?

http://codereview.chromium.org/8729009/diff/33002/chrome/browser/auto_launch_...
chrome/browser/auto_launch_trial.cc:29: UMA_HISTOGRAM_ENUMERATION(
assert response < 3 ?

http://codereview.chromium.org/8729009/diff/33002/chrome/browser/auto_launch_...
File chrome/browser/auto_launch_trial.h (right):

http://codereview.chromium.org/8729009/diff/33002/chrome/browser/auto_launch_...
chrome/browser/auto_launch_trial.h:26: extern const char
kAutoLaunchTrial_ControlGroup[];
I had originally used _ in the name for the default app experiments, but this
does not follow the coding standard.  I have since fixed this
(http://codereview.chromium.org/8805022/).  You might want to fix this now
before it get into the code.

http://codereview.chromium.org/8729009/diff/33002/chrome/browser/ui/browser_i...
File chrome/browser/ui/browser_init.cc (right):

http://codereview.chromium.org/8729009/diff/33002/chrome/browser/ui/browser_i...
chrome/browser/ui/browser_init.cc:184:
auto_launch_trial::UpdateInfobarResponseMetric(2);
Use an enum instead of hardcoding value.

http://codereview.chromium.org/8729009/diff/33002/chrome/browser/ui/browser_i...
chrome/browser/ui/browser_init.cc:220: bool auto_launch = false;
bool --> const bool ?

http://codereview.chromium.org/8729009/diff/33002/chrome/installer/util/util_...
File chrome/installer/util/util_constants.h (right):

http://codereview.chromium.org/8729009/diff/33002/chrome/installer/util/util_...
chrome/installer/util/util_constants.h:116: NUM_STAGES                   // 16:
The number of stages.
16 --> 17 ?

http://codereview.chromium.org/8729009/diff/33002/chrome/installer/util/util_...
chrome/installer/util/util_constants.h:122:
never_ever_ever_change_InstallerStage_values_bang);
how does this compile assert help ensure that the enum order above is always
preserved?

[grt (UTC plus 2), 2011-12-07 15:56:34]

http://codereview.chromium.org/8729009/diff/33002/chrome/installer/util/util_...
File chrome/installer/util/util_constants.h (right):

http://codereview.chromium.org/8729009/diff/33002/chrome/installer/util/util_...
chrome/installer/util/util_constants.h:116: NUM_STAGES                   // 16:
The number of stages.
On 2011/12/07 15:38:37, Roger Tawa wrote:
> 16 --> 17 ?

nice catch

http://codereview.chromium.org/8729009/diff/33002/chrome/installer/util/util_...
chrome/installer/util/util_constants.h:122:
never_ever_ever_change_InstallerStage_values_bang);
On 2011/12/07 15:38:37, Roger Tawa wrote:
> how does this compile assert help ensure that the enum order above is always
> preserved?

it's weak, but it's better than nothing at all.  the assert will fire if someone
fails to read the comment above the enum and inserts a value in the middle.  i
think this is the common case we want to protect against.  the integer values
are included in the comments as an additional hint to editors that they
shouldn't be toyed with.  do you think some additional checks are called for? 
how would you do it?

[Roger Tawa OOO till Jul 10th, 2011-12-07 16:17:16]

http://codereview.chromium.org/8729009/diff/33002/chrome/installer/util/util_...
File chrome/installer/util/util_constants.h (right):

http://codereview.chromium.org/8729009/diff/33002/chrome/installer/util/util_...
chrome/installer/util/util_constants.h:122:
never_ever_ever_change_InstallerStage_values_bang);
On 2011/12/07 15:56:35, grt wrote:
> On 2011/12/07 15:38:37, Roger Tawa wrote:
> > how does this compile assert help ensure that the enum order above is always
> > preserved?
> 
> it's weak, but it's better than nothing at all.  the assert will fire if
someone
> fails to read the comment above the enum and inserts a value in the middle.  i
> think this is the common case we want to protect against.  the integer values
> are included in the comments as an additional hint to editors that they
> shouldn't be toyed with.  do you think some additional checks are called for? 
> how would you do it?

OK makes sense Greg.  Yeah, tricky problem to solve without being too onerous on
the developer.

[Finnur, 2011-12-13 15:53:24]

PTAL

http://codereview.chromium.org/8729009/diff/33002/chrome/browser/auto_launch_...
File chrome/browser/auto_launch_trial.cc (right):

http://codereview.chromium.org/8729009/diff/33002/chrome/browser/auto_launch_...
chrome/browser/auto_launch_trial.cc:20: }
On 2011/12/07 15:38:37, Roger Tawa wrote:
> Shouldn't you also check the master prefs file here too?

Done.

http://codereview.chromium.org/8729009/diff/33002/chrome/browser/auto_launch_...
chrome/browser/auto_launch_trial.cc:29: UMA_HISTOGRAM_ENUMERATION(
Converted to enum.

On 2011/12/07 15:38:37, Roger Tawa wrote:
> assert response < 3 ?

http://codereview.chromium.org/8729009/diff/33002/chrome/browser/auto_launch_...
File chrome/browser/auto_launch_trial.h (right):

http://codereview.chromium.org/8729009/diff/33002/chrome/browser/auto_launch_...
chrome/browser/auto_launch_trial.h:26: extern const char
kAutoLaunchTrial_ControlGroup[];
On 2011/12/07 15:38:37, Roger Tawa wrote:
> I had originally used _ in the name for the default app experiments, but this
> does not follow the coding standard.  I have since fixed this
> (http://codereview.chromium.org/8805022/).  You might want to fix this now
> before it get into the code.

Done.

http://codereview.chromium.org/8729009/diff/33002/chrome/browser/ui/browser_i...
File chrome/browser/ui/browser_init.cc (right):

http://codereview.chromium.org/8729009/diff/33002/chrome/browser/ui/browser_i...
chrome/browser/ui/browser_init.cc:184:
auto_launch_trial::UpdateInfobarResponseMetric(2);
On 2011/12/07 15:38:37, Roger Tawa wrote:
> Use an enum instead of hardcoding value.

Done.

http://codereview.chromium.org/8729009/diff/33002/chrome/browser/ui/browser_i...
chrome/browser/ui/browser_init.cc:220: bool auto_launch = false;
On 2011/12/07 15:38:37, Roger Tawa wrote:
> bool --> const bool ?

Done.

http://codereview.chromium.org/8729009/diff/33002/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

http://codereview.chromium.org/8729009/diff/33002/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/browser_options_handler.cc:172:
base::FundamentalValue enabled(auto_launch_util::WillLaunchAtLogin());
It does. I just didn't see it because I built mini_installer in release so that
I didn't have to setup all the debug runtimes in order to test it on a clean VM
machine. Fixed.

On 2011/12/03 04:43:38, grt wrote:
> i can't remember if i remarked that this will hit the registry, so maybe it,
> too, needs to be done on the FILE thread.  won't this cause a DCHECK in the
> thread checker thingamaboob?

http://codereview.chromium.org/8729009/diff/33002/chrome/installer/setup/unin...
File chrome/installer/setup/uninstall.cc (right):

http://codereview.chromium.org/8729009/diff/33002/chrome/installer/setup/unin...
chrome/installer/setup/uninstall.cc:740: if
(auto_launch_util::WillLaunchAtLogin())
Good catch. Fixed.

On 2011/12/03 04:43:38, grt wrote:
> this will always return false since the current executable is setup.exe a few
> directories below where chrome.exe lives.

http://codereview.chromium.org/8729009/diff/33002/chrome/installer/util/auto_...
File chrome/installer/util/auto_launch_util.cc (right):

http://codereview.chromium.org/8729009/diff/33002/chrome/installer/util/auto_...
chrome/installer/util/auto_launch_util.cc:37: void SetWillLaunchAtLogin(bool
auto_launch, FilePath application_path) {
On 2011/12/03 04:43:38, grt wrote:
> const FilePath&

Done.

http://codereview.chromium.org/8729009/diff/33002/chrome/installer/util/auto_...
File chrome/installer/util/auto_launch_util.h (right):

http://codereview.chromium.org/8729009/diff/33002/chrome/installer/util/auto_...
chrome/installer/util/auto_launch_util.h:15: // Returns whether Chrome is set to
auto-launch at computer startup.
On 2011/12/03 04:43:38, grt wrote:
> // Returns whether the current executable is set to auto-launch at computer
> startup.

Done.

http://codereview.chromium.org/8729009/diff/33002/chrome/installer/util/auto_...
chrome/installer/util/auto_launch_util.h:18: // Configures whether Chrome should
auto-launch at computer startup.
On 2011/12/03 04:43:38, grt wrote:
> // Configures whether chrome.exe in |application_path| (or the directory of
> current executable, if |application_path| is empty) should...

Done.

http://codereview.chromium.org/8729009/diff/33002/chrome/installer/util/auto_...
chrome/installer/util/auto_launch_util.h:23: void SetWillLaunchAtLogin(bool
auto_launch, FilePath application_path);
On 2011/12/03 04:43:38, grt wrote:
> const FilePath&

Done.

http://codereview.chromium.org/8729009/diff/33002/chrome/installer/util/util_...
File chrome/installer/util/util_constants.h (right):

http://codereview.chromium.org/8729009/diff/33002/chrome/installer/util/util_...
chrome/installer/util/util_constants.h:116: NUM_STAGES                   // 16:
The number of stages.
On 2011/12/07 15:56:35, grt wrote:
> On 2011/12/07 15:38:37, Roger Tawa wrote:
> > 16 --> 17 ?
> 
> nice catch

Done.

[Roger Tawa OOO till Jul 10th, 2011-12-13 17:05:19]

lgtm, with one comment below.

http://codereview.chromium.org/8729009/diff/46001/chrome/browser/auto_launch_...
File chrome/browser/auto_launch_trial.cc (right):

http://codereview.chromium.org/8729009/diff/46001/chrome/browser/auto_launch_...
chrome/browser/auto_launch_trial.cc:42: == kAutoLaunchTrialAutoLaunchGroup;
After looking at this again, I believe my original comment was not correct.  You
don't need to check the master prefs here.

[grt (UTC plus 2), 2011-12-13 17:08:55]

http://codereview.chromium.org/8729009/diff/46001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

http://codereview.chromium.org/8729009/diff/46001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/browser_options_handler.cc:175:
weak_ptr_factory_for_ui_.DetachFromThread();
i'm not familiar enough with WeakPtr machinery to comment on the correctness of
this.  please have another reviewer double-check it.  thanks.

http://codereview.chromium.org/8729009/diff/46001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/browser_options_handler.cc:181: // Pass in weak
pointer to this to avoid race if BrowserOptionsHandler is
should the contents of this and CheckAutoLaunchCallback be #if defined(OS_WIN)?

http://codereview.chromium.org/8729009/diff/46001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/browser_options_handler.cc:191: bool
is_in_auto_launch_group, bool will_launch_at_login) {
newline after comma

http://codereview.chromium.org/8729009/diff/46001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/browser_options_handler.h (right):

http://codereview.chromium.org/8729009/diff/46001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/browser_options_handler.h:147: // Used to get
|weak_ptr_| to self on the IO thread.
IO -> File

http://codereview.chromium.org/8729009/diff/46001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/browser_options_handler.h:148:
base::WeakPtrFactory<BrowserOptionsHandler> weak_ptr_factory_for_io_;
io -> file

http://codereview.chromium.org/8729009/diff/46001/chrome/installer/setup/unin...
File chrome/installer/setup/uninstall.cc (right):

http://codereview.chromium.org/8729009/diff/46001/chrome/installer/setup/unin...
chrome/installer/setup/uninstall.cc:741: if
(auto_launch_util::WillLaunchAtLogin(installer_state.target_path()))
i believe this passes the path to the install directory rather than the path to
chrome.exe itself.  it's a bit confusing since WillLaunchAtLogin's parameter is
named "executable".  it will happen to work since its implementation does a
prefix match.

http://codereview.chromium.org/8729009/diff/46001/chrome/installer/util/auto_...
File chrome/installer/util/auto_launch_util.cc (right):

http://codereview.chromium.org/8729009/diff/46001/chrome/installer/util/auto_...
chrome/installer/util/auto_launch_util.cc:28: FilePath chrome_exe = executable;
i think this style is preferred:
FilePath chrome_exe(executable);

http://codereview.chromium.org/8729009/diff/46001/chrome/installer/util/auto_...
chrome/installer/util/auto_launch_util.cc:42: FilePath path = application_path;
same comment as above

http://codereview.chromium.org/8729009/diff/46001/chrome/installer/util/auto_...
File chrome/installer/util/auto_launch_util.h (right):

http://codereview.chromium.org/8729009/diff/46001/chrome/installer/util/auto_...
chrome/installer/util/auto_launch_util.h:23: // default to the chrome
application path), but can be overridden if needed. If
i find this NOTE confusing.  |application_path| may be blank ONLY if the caller
knows that they're running in a process that's in the same directory as
chrome.exe, no?

[Finnur, 2011-12-14 16:34:04]

Joi, can you take a look at the weakptr changes in
browser_options_handler.cc/.h?

[Jói, 2011-12-14 16:44:13]

browser_options_handler.(cc|h) LGTM with some nits.

I would suggest doing something like [ gcl try
--bot=linux_valgrind,linux_tsan,linux_asan ] before committing, or at least
watching those bots after commit.

Cheers,
Jói

http://codereview.chromium.org/8729009/diff/53001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

http://codereview.chromium.org/8729009/diff/53001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/browser_options_handler.cc:180:
base::WeakPtr<BrowserOptionsHandler> weak_this) {
Add this:

DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));

http://codereview.chromium.org/8729009/diff/53001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/browser_options_handler.cc:195: #if
defined(OS_WIN)
Add:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

http://codereview.chromium.org/8729009/diff/53001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/browser_options_handler.h (right):

http://codereview.chromium.org/8729009/diff/53001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/browser_options_handler.h:99: void
CheckAutoLaunch(base::WeakPtr<BrowserOptionsHandler> weak_this);
I would add a comment saying a weak pointer to this is passed in as a parameter
to avoid the need to lock between this function and the destructor.

http://codereview.chromium.org/8729009/diff/53001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/browser_options_handler.h:100: // Sets up (on
the UI thread) the necessary bindings for toggling auto-launch
Suggest a blank line before this.

http://codereview.chromium.org/8729009/diff/53001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/browser_options_handler.h:147: // Used to get
|weak_ptr_| to self on the File thread.
to self on the File thread -> to self for use on the File thread

[grt (UTC plus 2), 2011-12-14 16:57:53]

I like the symmetry in the auto_launch_util methods.  Installer changes LGTM. 
Good luck!