Make the auto-launch experiment profile-aware.

Make the auto-launch experiment profile-aware. 

When writing to the registry key, append a hash of the profile directory path, so as to not clobber settings between profiles and add the --profile-directory=foo component to the directory as well. 

BUG=112118
TEST=Test this as you would test the autolaunch experiment normally (see original autolaunch bug), but make sure to test it with multiple profiles and side-by-side installations. Note: Only the Default profile gets to set this value at present time.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=121860

[Finnur, 2012-01-31 18:42:06]

jhawkins, can you take the browser_options file changes?
grt, can you take the rest?

[Finnur, 2012-02-01 17:15:59]

grt, jhawkins: ping

[grt (UTC plus 2), 2012-02-01 18:45:15]

Sorry for the delay.  LG overall, but I think the hashing strategy is fragile;
see comments below.

https://chromiumcodereview.appspot.com/9317002/diff/3004/chrome/browser/ui/br...
File chrome/browser/ui/browser_init.cc (right):

https://chromiumcodereview.appspot.com/9317002/diff/3004/chrome/browser/ui/br...
chrome/browser/ui/browser_init.cc:1494: if
(profile->GetPath().BaseName().value() != ASCIIToUTF16("Default"))
"Default" -> chrome::kInitialProfile

https://chromiumcodereview.appspot.com/9317002/diff/3004/chrome/browser/ui/br...
chrome/browser/ui/browser_init.cc:1505:
BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
it just occurred to me that this should never run for Chrome Frame.  i believe
that the first run sentinel will already have been created in
ChromeBrowserMainParts::PreCreateThreadsImpl since GCF launches Chrome with
--no-first-run, so IsChromeFirstRun() should be false.  can you confirm this?

https://chromiumcodereview.appspot.com/9317002/diff/3004/chrome/installer/set...
File chrome/installer/setup/install.cc (right):

https://chromiumcodereview.appspot.com/9317002/diff/3004/chrome/installer/set...
chrome/installer/setup/install.cc:426:
chrome_install->GetUserDataPath().AppendASCII("Default");
#include "chrome/common/chrome_constants.h"
"Default" -> chrome::kInitialProfile
(installer_util depends on common_constants, so i believe this is safe)

https://chromiumcodereview.appspot.com/9317002/diff/3004/chrome/installer/set...
chrome/installer/setup/install.cc:428: true, installer_state.target_path(),
nit: move |path| up to this line

https://chromiumcodereview.appspot.com/9317002/diff/3004/chrome/installer/set...
File chrome/installer/setup/uninstall.cc (right):

https://chromiumcodereview.appspot.com/9317002/diff/3004/chrome/installer/set...
chrome/installer/setup/uninstall.cc:741: FilePath path =
product.GetUserDataPath().AppendASCII("Default");
"Default" -> chrome::kInitialProfile

https://chromiumcodereview.appspot.com/9317002/diff/3004/chrome/installer/set...
chrome/installer/setup/uninstall.cc:741: FilePath path =
product.GetUserDataPath().AppendASCII("Default");
nit: path -> profile_path or somesuch?

https://chromiumcodereview.appspot.com/9317002/diff/3004/chrome/installer/uti...
File chrome/installer/util/auto_launch_util.cc (right):

https://chromiumcodereview.appspot.com/9317002/diff/3004/chrome/installer/uti...
chrome/installer/util/auto_launch_util.cc:29: string16 ProfileToKeyName(const
FilePath& profile_path) {
i'm concerned that profile_path may be equivalent from the file system's point
of view but not bit-for-bit identical across multiple calls (some from Chrome,
some from setup.exe).  can you think of any less fragile way to uniquely
identify an installation and profile?  auto-launch could be scoped down to a
Chrome-only feature (not Chromium, not SxS/canary), so that the profile is the
only thing you'd need to use as the key.

https://chromiumcodereview.appspot.com/9317002/diff/3004/chrome/installer/uti...
chrome/installer/util/auto_launch_util.cc:30: std::string input =
profile_path.AsUTF8Unsafe();
nit: input(profile_path.AsUTF8Unsafe());

https://chromiumcodereview.appspot.com/9317002/diff/3004/chrome/installer/uti...
chrome/installer/util/auto_launch_util.cc:35: ASCIIToWide("_") +
ASCIIToWide(hash_string);
nit: either indent 4 from the previous line, or put the terms in parens so you
can align the 'A' with the 's' on the previous line.

https://chromiumcodereview.appspot.com/9317002/diff/3004/chrome/installer/uti...
chrome/installer/util/auto_launch_util.cc:40: string16 key_name =
ProfileToKeyName(profile_path);
nit: key_name(ProfileToKeyName(profile_path));

https://chromiumcodereview.appspot.com/9317002/diff/3004/chrome/installer/uti...
chrome/installer/util/auto_launch_util.cc:62: string16 key_name =
ProfileToKeyName(profile_path);
nit: key_name(...);

https://chromiumcodereview.appspot.com/9317002/diff/3004/chrome/installer/uti...
File chrome/installer/util/auto_launch_util.h (right):

https://chromiumcodereview.appspot.com/9317002/diff/3004/chrome/installer/uti...
chrome/installer/util/auto_launch_util.h:18: // the current executable).
|profile_path| is the path to the current profile.
"current profile" -> "profile that should be opened at launch" or somesuch

https://chromiumcodereview.appspot.com/9317002/diff/3004/chrome/installer/uti...
chrome/installer/util/auto_launch_util.h:28: // the path to the current profile.
same as above

[Finnur, 2012-02-03 14:56:58]

jhawkins: ping?
grt: hold on, working on your comments.

[Finnur, 2012-02-06 16:30:36]

grt: All addressed, comments below.
jhawkins: Still awaiting your comments.

https://chromiumcodereview.appspot.com/9317002/diff/3004/chrome/browser/ui/br...
File chrome/browser/ui/browser_init.cc (right):

https://chromiumcodereview.appspot.com/9317002/diff/3004/chrome/browser/ui/br...
chrome/browser/ui/browser_init.cc:1505:
BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
This is already guarded against by the IsInAutoLaunchGroup call as we don't
issue ChromeFrame builds with the special brand code needed for the experiment.

But, why rely on that or the sentinel creation or whatever? Why not just
explicitly state we shouldn't do this for ChromeFrame? :)

On 2012/02/01 18:45:15, grt wrote:
> it just occurred to me that this should never run for Chrome Frame.  i believe
> that the first run sentinel will already have been created in
> ChromeBrowserMainParts::PreCreateThreadsImpl since GCF launches Chrome with
> --no-first-run, so IsChromeFirstRun() should be false.  can you confirm this?

https://chromiumcodereview.appspot.com/9317002/diff/3004/chrome/installer/uti...
File chrome/installer/util/auto_launch_util.cc (right):

https://chromiumcodereview.appspot.com/9317002/diff/3004/chrome/installer/uti...
chrome/installer/util/auto_launch_util.cc:29: string16 ProfileToKeyName(const
FilePath& profile_path) {
I'm hesitant to make it Chrome only, as that makes it harder to test. It kind of
doesn't make sense to exclude the Canary which is very valuable for testing nor
(in the future, if this feature takes off) the SXS.

Any kind of unique identifier we create and store, for example, in the profile
is going to be met with suspicion.

I also think the failure case here (in the unlikely event that this would
happen) is not all that serious. Chrome would always auto-launch (unless you
removed the registry key manually). It wouldn't be left behind either, as it
would get deleted on uninstall.

However, what do you think about just getting the path from the same source as
the installer (GetUserDataPath(), like shown in the latest CL)?

On 2012/02/01 18:45:15, grt wrote:
> i'm concerned that profile_path may be equivalent from the file system's point
> of view but not bit-for-bit identical across multiple calls (some from Chrome,
> some from setup.exe).  can you think of any less fragile way to uniquely
> identify an installation and profile?  auto-launch could be scoped down to a
> Chrome-only feature (not Chromium, not SxS/canary), so that the profile is the
> only thing you'd need to use as the key.

[grt (UTC plus 2), 2012-02-06 17:54:48]

https://chromiumcodereview.appspot.com/9317002/diff/3004/chrome/browser/ui/br...
File chrome/browser/ui/browser_init.cc (right):

https://chromiumcodereview.appspot.com/9317002/diff/3004/chrome/browser/ui/br...
chrome/browser/ui/browser_init.cc:1505:
BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
On 2012/02/06 16:30:36, Finnur wrote:
> This is already guarded against by the IsInAutoLaunchGroup call as we don't
> issue ChromeFrame builds with the special brand code needed for the
experiment.

Since Chrome and Chrome Frame can use the same bits on the system, I'm concerned
that Chrome's brand code could be picked up by Chrome Frame under some
circumstances.

> But, why rely on that or the sentinel creation or whatever? Why not just
> explicitly state we shouldn't do this for ChromeFrame? :)

Sounds good.  Please add a check for switches::kChromeFrame as done elsewhere in
the code.  Thanks.

https://chromiumcodereview.appspot.com/9317002/diff/9001/chrome/installer/uti...
File chrome/installer/util/auto_launch_util.cc (right):

https://chromiumcodereview.appspot.com/9317002/diff/9001/chrome/installer/uti...
chrome/installer/util/auto_launch_util.cc:32: string16 ProfileToKeyName(const
FilePath& profile_path) {
should this be changed from profile_path to a string that's just the name of the
profile directory?  i think the signatures and comments for WillLaunchAtLogin
and SetWillLaunchAtLogin need to be updated now, too.

https://chromiumcodereview.appspot.com/9317002/diff/9001/chrome/installer/uti...
chrome/installer/util/auto_launch_util.cc:36: path =
command_line.GetSwitchValuePath(switches::kUserDataDir);
shouldn't the BaseName of |profile_path| be appended here as well?  (or am i
mistaken in understanding that the user data dir is the container of the
profile-specific dirs?)

[James Hawkins, 2012-02-06 19:46:10]

Need to make the proper changes to the old options/ code as well.  options2 is
not going to beta or stable for M17.

[Finnur, 2012-02-06 21:03:38]

Can you be more specific, James? I am making identical changes in
BrowserOptionsHandler and the .html and .js changes are already in for the old
code.

[James Hawkins, 2012-02-06 21:04:26]

On 2012/02/06 21:03:38, Finnur wrote:
> Can you be more specific, James? I am making identical changes in
> BrowserOptionsHandler and the .html and .js changes are already in for the old
> code.

Sorry, didn't know that. Reviewing now.

[James Hawkins, 2012-02-06 21:08:01]

https://chromiumcodereview.appspot.com/9317002/diff/9001/chrome/browser/resou...
File chrome/browser/resources/options2/browser_options.html (right):

https://chromiumcodereview.appspot.com/9317002/diff/9001/chrome/browser/resou...
chrome/browser/resources/options2/browser_options.html:253: <div
id="autoLaunchOption" class="checkbox" hidden>
nit: auto-launch-option

I have a CL out for review that fixes all the style issues on this page, would
be nice to not conflict.

https://chromiumcodereview.appspot.com/9317002/diff/9001/chrome/browser/resou...
chrome/browser/resources/options2/browser_options.html:254: <label
id="autoLaunchLabel">
nit: auto-launch-label

https://chromiumcodereview.appspot.com/9317002/diff/9001/chrome/browser/resou...
chrome/browser/resources/options2/browser_options.html:255: <input
id="autoLaunch" type="checkbox">
nit: auto-launch

https://chromiumcodereview.appspot.com/9317002/diff/9001/chrome/browser/resou...
File chrome/browser/resources/options2/browser_options.js (right):

https://chromiumcodereview.appspot.com/9317002/diff/9001/chrome/browser/resou...
chrome/browser/resources/options2/browser_options.js:217:
$('autoLaunch').addEventListener('click',
nit: Use $('xx').onclick = this.handle...

https://chromiumcodereview.appspot.com/9317002/diff/9001/chrome/browser/resou...
chrome/browser/resources/options2/browser_options.js:461: * Sets or clear
whether Chrome should Auto-launch on computer startup.
@private

https://chromiumcodereview.appspot.com/9317002/diff/9001/chrome/browser/resou...
chrome/browser/resources/options2/browser_options.js:464:
chrome.send('toggleAutoLaunch', [Boolean($('autoLaunch').checked)]);
Correct me if I'm wrong, but I don't think you need to cast to Boolean.

https://chromiumcodereview.appspot.com/9317002/diff/9001/chrome/browser/ui/br...
File chrome/browser/ui/browser_init.cc (right):

https://chromiumcodereview.appspot.com/9317002/diff/9001/chrome/browser/ui/br...
chrome/browser/ui/browser_init.cc:169: Profile* profile_;
Document the variable, ownership.

https://chromiumcodereview.appspot.com/9317002/diff/9001/chrome/browser/ui/br...
chrome/browser/ui/browser_init.cc:1496: return false;
Braces required due to multi-line logic statement.

https://chromiumcodereview.appspot.com/9317002/diff/9001/chrome/browser/ui/we...
File chrome/browser/ui/webui/options/browser_options_handler.h (right):

https://chromiumcodereview.appspot.com/9317002/diff/9001/chrome/browser/ui/we...
chrome/browser/ui/webui/options/browser_options_handler.h:102: const FilePath&
profile_path);
Document |profile_path|.

[Finnur, 2012-02-07 12:29:01]

All comments addressed, please take another look.

https://chromiumcodereview.appspot.com/9317002/diff/9001/chrome/browser/ui/br...
File chrome/browser/ui/browser_init.cc (right):

https://chromiumcodereview.appspot.com/9317002/diff/9001/chrome/browser/ui/br...
chrome/browser/ui/browser_init.cc:1496: return false;
I used to do what you suggest all the time from the start and was told I was
wrong (only need braces if the contents of the if/else clause was multi-line
(not the conditional). But, anyway -- changed it.

https://chromiumcodereview.appspot.com/9317002/diff/9001/chrome/installer/uti...
File chrome/installer/util/auto_launch_util.cc (right):

https://chromiumcodereview.appspot.com/9317002/diff/9001/chrome/installer/uti...
chrome/installer/util/auto_launch_util.cc:36: path =
command_line.GetSwitchValuePath(switches::kUserDataDir);
Good catch!

[grt (UTC plus 2), 2012-02-07 16:13:10]

https://chromiumcodereview.appspot.com/9317002/diff/19001/chrome/installer/ut...
File chrome/installer/util/auto_launch_util.cc (right):

https://chromiumcodereview.appspot.com/9317002/diff/19001/chrome/installer/ut...
chrome/installer/util/auto_launch_util.cc:39: BrowserDistribution* distribution
= BrowserDistribution::GetDistribution();
Apologies for not catching this earlier. Please use the following:
BrowserDistribution* distribution =
    BrowserDistribution::GetSpecificDistribution(
        BrowserDistribution::CHROME_BROWSER);
The reason is that GetDistribution() will return the Chrome Frame distribution
when called from within setup.exe under some circumstances.  That's never
desired here, so it's safest to explicitly request Chrome's distribution.

https://chromiumcodereview.appspot.com/9317002/diff/19001/chrome/installer/ut...
chrome/installer/util/auto_launch_util.cc:88: string16 cmd_line =
ASCIIToUTF16("\"");
This command line doesn't take into account a custom --user-data-dir.  Should it
be added if it's present on the current proc's command-line?

[Finnur, 2012-02-07 16:26:05]

https://chromiumcodereview.appspot.com/9317002/diff/19001/chrome/installer/ut...
File chrome/installer/util/auto_launch_util.cc (right):

https://chromiumcodereview.appspot.com/9317002/diff/19001/chrome/installer/ut...
chrome/installer/util/auto_launch_util.cc:39: BrowserDistribution* distribution
= BrowserDistribution::GetDistribution();
Good point. Fixed.

https://chromiumcodereview.appspot.com/9317002/diff/19001/chrome/installer/ut...
chrome/installer/util/auto_launch_util.cc:88: string16 cmd_line =
ASCIIToUTF16("\"");
On 2012/02/07 16:13:11, grt wrote:
> This command line doesn't take into account a custom --user-data-dir.  Should
it
> be added if it's present on the current proc's command-line?

Oh, interesting idea. You mean to add it, if present? Yeah, I think that's the
right thing to do. Good catch.

[grt (UTC plus 2), 2012-02-07 16:36:03]

Sorry to make more work for you...

https://chromiumcodereview.appspot.com/9317002/diff/19001/chrome/installer/ut...
File chrome/installer/util/auto_launch_util.cc (right):

https://chromiumcodereview.appspot.com/9317002/diff/19001/chrome/installer/ut...
chrome/installer/util/auto_launch_util.cc:88: string16 cmd_line =
ASCIIToUTF16("\"");
On 2012/02/07 16:26:05, Finnur wrote:
> On 2012/02/07 16:13:11, grt wrote:
> > This command line doesn't take into account a custom --user-data-dir. 
Should
> it
> > be added if it's present on the current proc's command-line?
> 
> Oh, interesting idea. You mean to add it, if present? Yeah, I think that's the
> right thing to do. Good catch.

I think this should be a part of the test plan for the feature: run Chrome with
a custom user data dir and make sure the autolaunch setting works as expected.

https://chromiumcodereview.appspot.com/9317002/diff/23003/chrome/installer/se...
File chrome/installer/setup/uninstall.cc (right):

https://chromiumcodereview.appspot.com/9317002/diff/23003/chrome/installer/se...
chrome/installer/setup/uninstall.cc:741: if
(auto_launch_util::WillLaunchAtLogin(
This won't work if custom user data dirs are in use, will it?  I think the right
thing to do at uninstall is to iterate over all autolaunch entries and remove
any that point to the Chrome executable that's being uninstalled.

https://chromiumcodereview.appspot.com/9317002/diff/23003/chrome/installer/ut...
File chrome/installer/util/auto_launch_util.cc (right):

https://chromiumcodereview.appspot.com/9317002/diff/23003/chrome/installer/ut...
chrome/installer/util/auto_launch_util.cc:103:
command_line.GetSwitchValuePath(switches::kUserDataDir).value();
how about this to avoid the string -> FilePath -> string conversions:
cmd_line += command_line.GetSwitchValueNative(switches::kUserDataDir);

[Finnur, 2012-02-07 18:20:30]

https://chromiumcodereview.appspot.com/9317002/diff/23003/chrome/installer/se...
File chrome/installer/setup/uninstall.cc (right):

https://chromiumcodereview.appspot.com/9317002/diff/23003/chrome/installer/se...
chrome/installer/setup/uninstall.cc:741: if
(auto_launch_util::WillLaunchAtLogin(
Hmm... It will not, but I'd rather disable this feature for --user-data-dir than
iterate over keys and remove random ones that point to a chrome executable...
(which would be a shame). 

However, I don't think this is a use case we should worry about. People who use
--user-data-dir know how to remove registry keys (and besides, a stray Run key
is harmless).

On 2012/02/07 16:36:04, grt wrote:
> This won't work if custom user data dirs are in use, will it?  I think the
right
> thing to do at uninstall is to iterate over all autolaunch entries and remove
> any that point to the Chrome executable that's being uninstalled.

https://chromiumcodereview.appspot.com/9317002/diff/23003/chrome/installer/ut...
File chrome/installer/util/auto_launch_util.cc (right):

https://chromiumcodereview.appspot.com/9317002/diff/23003/chrome/installer/ut...
chrome/installer/util/auto_launch_util.cc:103:
command_line.GetSwitchValuePath(switches::kUserDataDir).value();
Sure. I'll do that first thing tomorrow morning.

 On 2012/02/07 16:36:04, grt wrote:
> how about this to avoid the string -> FilePath -> string conversions:
> cmd_line += command_line.GetSwitchValueNative(switches::kUserDataDir);

[grt (UTC plus 2), 2012-02-07 18:39:19]

https://chromiumcodereview.appspot.com/9317002/diff/23003/chrome/installer/se...
File chrome/installer/setup/uninstall.cc (right):

https://chromiumcodereview.appspot.com/9317002/diff/23003/chrome/installer/se...
chrome/installer/setup/uninstall.cc:741: if
(auto_launch_util::WillLaunchAtLogin(
On 2012/02/07 18:20:30, Finnur wrote:
> Hmm... It will not, but I'd rather disable this feature for --user-data-dir
than
> iterate over keys and remove random ones that point to a chrome executable...
> (which would be a shame). 

Iterate and delete will be required if/when the feature supports anything other
than kInitialProfile, as the uninstaller doesn't know anything about what
profiles may have been created.

Also, deleting a profile must result in the removal of its corresponding
autolaunch key.  Can the initial profile be deleted?

> However, I don't think this is a use case we should worry about. People who
use
> --user-data-dir know how to remove registry keys (and besides, a stray Run key
> is harmless).

Leaving stray data laying around on users machines is bad.  How is a user
expected to know to look in the Run key to clean up behind Chrome?

[Finnur, 2012-02-09 16:29:22]

jhawkins: Can a brother get an LGTM?  :)

grt: I tried going down this path of cleaning up stray Run key entries to
support the --user-data-dir and decided to hold off since it was snowballing a
bit. I instead disabled the feature for --user-data-dir and made sure the Run
key is cleared when the profile is deleted. PTAL?

On 2012/02/07 18:39:19, grt wrote:
>
https://chromiumcodereview.appspot.com/9317002/diff/23003/chrome/installer/se...
> File chrome/installer/setup/uninstall.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9317002/diff/23003/chrome/installer/se...
> chrome/installer/setup/uninstall.cc:741: if
> (auto_launch_util::WillLaunchAtLogin(
> On 2012/02/07 18:20:30, Finnur wrote:
> > Hmm... It will not, but I'd rather disable this feature for --user-data-dir
> than
> > iterate over keys and remove random ones that point to a chrome
executable...
> > (which would be a shame). 
> 
> Iterate and delete will be required if/when the feature supports anything
other
> than kInitialProfile, as the uninstaller doesn't know anything about what
> profiles may have been created.
> 
> Also, deleting a profile must result in the removal of its corresponding
> autolaunch key.  Can the initial profile be deleted?
> 
> > However, I don't think this is a use case we should worry about. People who
> use
> > --user-data-dir know how to remove registry keys (and besides, a stray Run
key
> > is harmless).
> 
> Leaving stray data laying around on users machines is bad.  How is a user
> expected to know to look in the Run key to clean up behind Chrome?

[grt (UTC plus 2), 2012-02-09 16:43:50]

https://chromiumcodereview.appspot.com/9317002/diff/32001/chrome/browser/prof...
File chrome/browser/profiles/profile_info_cache.cc (right):

https://chromiumcodereview.appspot.com/9317002/diff/32001/chrome/browser/prof...
chrome/browser/profiles/profile_info_cache.cc:259:
base::Bind(&DeleteAutoLaunchValueForProfile, profile_path));
shouldn't this be more like the ProfileShortcutManager and take action when
OnProfileWillBeRemoved is sent out?

[Finnur, 2012-02-09 17:35:28]

We don't have a dedicated class to handle it and creating one feels
wasteful. Are you suggesting I remove this where we remove the shortcut (in
the ProfileShortcutManager)?

On Thu, Feb 9, 2012 at 16:43, <grt@chromium.org> wrote:

>
> https://chromiumcodereview.**appspot.com/9317002/diff/**
>
32001/chrome/browser/profiles/**profile_info_cache.cc<https://chromiumcodereview.appspot.com/9317002/diff/32001/chrome/browser/profiles/profile_info_cache.cc>
> File chrome/browser/profiles/**profile_info_cache.cc (right):
>
> https://chromiumcodereview.**appspot.com/9317002/diff/**
>
32001/chrome/browser/profiles/**profile_info_cache.cc#**newcode259<https://chromiumcodereview.appspot.com/9317002/diff/32001/chrome/browser/profiles/profile_info_cache.cc#newcode259>
> chrome/browser/profiles/**profile_info_cache.cc:259:
> base::Bind(&**DeleteAutoLaunchValueForProfil**e, profile_path));
> shouldn't this be more like the ProfileShortcutManager and take action
> when OnProfileWillBeRemoved is sent out?
>
>
https://chromiumcodereview.**appspot.com/9317002/<https://chromiumcodereview....
>

[grt (UTC plus 2), 2012-02-09 18:04:05]

I guess it would be a lot of code to add an observer for profiles that have
autolaunch enabled.  A browser/profile OWNER should weigh in on whether the
profile_info_cache change is okay.

[James Hawkins, 2012-02-09 18:32:53]

LGTM with nit.

https://chromiumcodereview.appspot.com/9317002/diff/32001/chrome/browser/ui/w...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://chromiumcodereview.appspot.com/9317002/diff/32001/chrome/browser/ui/w...
chrome/browser/ui/webui/options/browser_options_handler.cc:195: // We don't
support this for secondary profiles yet.
Optional, recommended nit: Don't use pronouns (We, this) in comments because
they (pronouns) are ambiguous, especially in such a dynamic context as code:

"Auto-launch is not supported for secondary profiles yet."

[Finnur, 2012-02-10 14:04:56]

grt: I've asked Miranda for a second opinion on the profile_info_cache.cc
changes. Does the rest look ok for an LGTM?

On 2012/02/09 18:32:53, James Hawkins wrote:
> LGTM with nit.
> 
>
https://chromiumcodereview.appspot.com/9317002/diff/32001/chrome/browser/ui/w...
> File chrome/browser/ui/webui/options/browser_options_handler.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9317002/diff/32001/chrome/browser/ui/w...
> chrome/browser/ui/webui/options/browser_options_handler.cc:195: // We don't
> support this for secondary profiles yet.
> Optional, recommended nit: Don't use pronouns (We, this) in comments because
> they (pronouns) are ambiguous, especially in such a dynamic context as code:
> 
> "Auto-launch is not supported for secondary profiles yet."

[Miranda Callahan, 2012-02-10 14:56:29]

LGTM, thanks.

On 2012/02/10 14:04:56, Finnur wrote:
> grt: I've asked Miranda for a second opinion on the profile_info_cache.cc
> changes. Does the rest look ok for an LGTM?
> 
> On 2012/02/09 18:32:53, James Hawkins wrote:
> > LGTM with nit.
> > 
> >
>
https://chromiumcodereview.appspot.com/9317002/diff/32001/chrome/browser/ui/w...
> > File chrome/browser/ui/webui/options/browser_options_handler.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/9317002/diff/32001/chrome/browser/ui/w...
> > chrome/browser/ui/webui/options/browser_options_handler.cc:195: // We don't
> > support this for secondary profiles yet.
> > Optional, recommended nit: Don't use pronouns (We, this) in comments because
> > they (pronouns) are ambiguous, especially in such a dynamic context as code:
> > 
> > "Auto-launch is not supported for secondary profiles yet."

[grt (UTC plus 2), 2012-02-10 16:30:05]

Lgtm

[Finnur, 2012-02-13 10:27:59]

Scott, can you give an OWNERS green light for the changes in browser_init.cc?

[sky, 2012-02-13 16:26:18]

LGTM