Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9972012:
Resolve the conflict that auto-launch has with the background mode feature (part 1). (Closed)
|
Created:
8 years, 8 months ago by Finnur Modified:
8 years, 8 months ago CC:
chromium-reviews, grt+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionResolve the conflict that auto-launch has with the background mode feature.
The Auto-launch experiment writes to the Run Registry key on Windows asking for Chrome to be started at login.
The BackgroundModeManager writes to another Run Registry key asking for Chrome to be started without a window.
We need to consolidate this into one Run key that specifies which feature you want as a command line flag to chrome.exe and make sure if a window is requested, then a window is shown (BackgroundModeManager doesn't care if the window is shown or not).
BUG=123139, 53600
TEST=QA has testing instructions on how to test the experimental auto-launch feature (requires branded build). Not sure about testing instructions for background mode.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=133166
Patch Set 1 #
Total comments: 3
Patch Set 2 : #
Total comments: 15
Patch Set 3 : #
Total comments: 11
Patch Set 4 : #Patch Set 5 : gclient sync, ignore #
Total comments: 5
Messages
Total messages: 29 (0 generated)
This changes my code to:
1) Support background mode feature sharing the Run key with my auto-start
feature.
2) Make sure the auto-start feature "wins" if both background mode and
auto-start are specified.
I started looking into changing the BackgroundManager to use my code, and I
believe the change is relatively simple. If we delete the code in
background_mode_manager_win.cc (roughly lines 23-68) that deals with the
registry we just have to change one function like so (once I've checked in my
code):
void BackgroundModeManager::EnableLaunchOnStartup(bool should_launch) {
// This functionality is only defined for default profile, currently.
if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kUserDataDir))
return;
BrowserThread::PostTask(
BrowserThread::FILE, FROM_HERE,
base::Bind(&auto_launch_util::SetWillLaunchAtLogin,
FilePath(),
profile->GetPath().BaseName().value(),
auto_launch_util::FLAG_PRESERVE, //
Auto start.
should_launch ? auto_launch_util::FLAG_ENABLE : //
Background mode.
auto_launch_util::FLAG_DISABLE));
}
... and then we're done (once we test, which I'm not sure how to do). :)
However, the only snag (which is another thing that prevented me from actually
doing this in this CL)... This function doesn't have access to the |profile|
object, although it is clear from some comments in this file that the intention
to add it is there. Can I rely on someone from your side to take that step and
hook up this function?
hi finnur! i have a drive-by question about the overall approach. https://chromiumcodereview.appspot.com/9972012/diff/1/chrome/browser/profiles... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://chromiumcodereview.appspot.com/9972012/diff/1/chrome/browser/profiles... chrome/browser/profiles/profile_shortcut_manager_win.cc:215: auto_launch_util::SetWillLaunchAtLogin( this doesn't seem to scale very well: all callsites that adjust one "feature" of the Run keys now need to know about all features for all other callsites. can the interface to the Run key be simplified so that the enable/disable autolaunch sites only need to care about flipping that one bit, and let the underlying implementation take care of preserving other settings or deleting the Run value altogether where appropriate?
https://chromiumcodereview.appspot.com/9972012/diff/1/chrome/browser/profiles... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://chromiumcodereview.appspot.com/9972012/diff/1/chrome/browser/profiles... chrome/browser/profiles/profile_shortcut_manager_win.cc:215: auto_launch_util::SetWillLaunchAtLogin( Can you elaborate a bit? Are you talking about an enum: DISABLE_ALL, DISABLE_AUTO_START, DISABLE_BACKGROUND_MODE, ENABLE_AUTO_START, ENABLE_BACKGROUND_MODE, ... that we pass in to SetWillLaunchAtLogin instead of the bools? On 2012/04/16 15:30:14, grt wrote: > this doesn't seem to scale very well: all callsites that adjust one "feature" of > the Run keys now need to know about all features for all other callsites. can > the interface to the Run key be simplified so that the enable/disable autolaunch > sites only need to care about flipping that one bit, and let the underlying > implementation take care of preserving other settings or deleting the Run value > altogether where appropriate?
https://chromiumcodereview.appspot.com/9972012/diff/1/chrome/browser/profiles... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://chromiumcodereview.appspot.com/9972012/diff/1/chrome/browser/profiles... chrome/browser/profiles/profile_shortcut_manager_win.cc:215: auto_launch_util::SetWillLaunchAtLogin( My first thought was just different functions: one (or one set) related to autolaunch and another related to background mode. The underlying implementation of them would know that they use different command-line args in a shared Run key, but the callers don't need to know that. On 2012/04/16 15:43:48, Finnur wrote: > Can you elaborate a bit? Are you talking about an enum: > > DISABLE_ALL, > DISABLE_AUTO_START, > DISABLE_BACKGROUND_MODE, > ENABLE_AUTO_START, > ENABLE_BACKGROUND_MODE, > > ... that we pass in to SetWillLaunchAtLogin instead of the bools? > > > > On 2012/04/16 15:30:14, grt wrote: > > this doesn't seem to scale very well: all callsites that adjust one "feature" > of > > the Run keys now need to know about all features for all other callsites. can > > the interface to the Run key be simplified so that the enable/disable > autolaunch > > sites only need to care about flipping that one bit, and let the underlying > > implementation take care of preserving other settings or deleting the Run > value > > altogether where appropriate? >
I like how this simplifies the call sites. Please take another look. https://chromiumcodereview.appspot.com/9972012/diff/8001/chrome/browser/ui/br... File chrome/browser/ui/browser_init.cc (right): https://chromiumcodereview.appspot.com/9972012/diff/8001/chrome/browser/ui/br... chrome/browser/ui/browser_init.cc:261: profile_->GetPath().BaseName().value())); // Background mode. Comment removed locally.
https://chromiumcodereview.appspot.com/9972012/diff/8001/chrome/browser/profi... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://chromiumcodereview.appspot.com/9972012/diff/8001/chrome/browser/profi... chrome/browser/profiles/profile_shortcut_manager_win.cc:211: if (auto_launch_util::WillLaunchAtLoginWithSwitch( can you do away with the "WithSwitch" part of this, too? https://chromiumcodereview.appspot.com/9972012/diff/8001/chrome/browser/ui/br... File chrome/browser/ui/browser_init.cc (right): https://chromiumcodereview.appspot.com/9972012/diff/8001/chrome/browser/ui/br... chrome/browser/ui/browser_init.cc:383: Browser* browser = BrowserList::FindAnyBrowser(profile, true); Looks like this could potentially return NULL. Will that never happen in this case for some reason? https://chromiumcodereview.appspot.com/9972012/diff/8001/chrome/installer/uti... File chrome/installer/util/auto_launch_util.h (right): https://chromiumcodereview.appspot.com/9972012/diff/8001/chrome/installer/uti... chrome/installer/util/auto_launch_util.h:43: // Disables autostarting Chrome at computer startup. |profile_directory| is the nit: insert newline before comment https://chromiumcodereview.appspot.com/9972012/diff/8001/chrome/installer/uti... chrome/installer/util/auto_launch_util.h:61: // Disables autostarting Chrome in background mode at computer startup. nit: insert newline before comment
See my note about background mode not being specific to a given profile - given that, I think you can add background mode support to this patch. http://codereview.chromium.org/9972012/diff/8001/chrome/browser/ui/browser_in... File chrome/browser/ui/browser_init.cc (right): http://codereview.chromium.org/9972012/diff/8001/chrome/browser/ui/browser_in... chrome/browser/ui/browser_init.cc:383: Browser* browser = BrowserList::FindAnyBrowser(profile, true); Agreed. Do we know this is never called with no window open (i.e. while chrome is in bg mode)? http://codereview.chromium.org/9972012/diff/8001/chrome/installer/setup/unins... File chrome/installer/setup/uninstall.cc (right): http://codereview.chromium.org/9972012/diff/8001/chrome/installer/setup/unins... chrome/installer/setup/uninstall.cc:768: ASCIIToUTF16(chrome::kInitialProfile), Why do we need to call this first? Can't we just call DisableAllAutostartFeatures() regardless of whether we're currently set to autostart (seems like that routine should deal with not having the registry key set)? http://codereview.chromium.org/9972012/diff/8001/chrome/installer/util/auto_l... File chrome/installer/util/auto_launch_util.h (right): http://codereview.chromium.org/9972012/diff/8001/chrome/installer/util/auto_l... chrome/installer/util/auto_launch_util.h:24: // not blank, must be present for the function to return true. Not sure what the behavior is if command_line_switch == "". Is "" the same as wildcard? http://codereview.chromium.org/9972012/diff/8001/chrome/installer/util/auto_l... chrome/installer/util/auto_launch_util.h:35: // |application_path| is needed when one when the caller is not the process remove "when one" http://codereview.chromium.org/9972012/diff/8001/chrome/installer/util/auto_l... chrome/installer/util/auto_launch_util.h:59: void EnableBackgroundModeAtLogin(const FilePath& application_path, I don't think we need the profile_directory for this - background mode is defined as loading *all* profiles at startup that have a background app installed, via ProfileManager::AutoLoadProfiles(). http://codereview.chromium.org/9972012/diff/8001/chrome/installer/util/auto_l... chrome/installer/util/auto_launch_util.h:64: void DisableBackgroundModeAtLogin(const string16& profile_directory); Likewise, this shouldn't need to take a path (see my previous comment on EnableBackgroundModeAtLogin()).
http://codereview.chromium.org/9972012/diff/8001/chrome/installer/util/auto_l... File chrome/installer/util/auto_launch_util.cc (right): http://codereview.chromium.org/9972012/diff/8001/chrome/installer/util/auto_l... chrome/installer/util/auto_launch_util.cc:179: void DisableBackgroundModeAtLogin(const string16& profile_directory) { I'm concerned this won't have the effect you're looking for. Right now in chrome_browser_main.cc we autoload any profiles that have background apps, irrelevant of any command line features. So you may need to add a wrapper around that call if you're trying to prevent the loading of background apps since this doesn't seem to change the global background_mode state.
I think the plan is that that routine will only be called when we have no background apps installed (basically, this is our attempt to remove chrome from the auto-run registry key, except we don't actually want to do that in the case that the auto-launch experiment is active for this user).
PTAL. http://codereview.chromium.org/9972012/diff/8001/chrome/browser/ui/browser_in... File chrome/browser/ui/browser_init.cc (right): http://codereview.chromium.org/9972012/diff/8001/chrome/browser/ui/browser_in... chrome/browser/ui/browser_init.cc:383: Browser* browser = BrowserList::FindAnyBrowser(profile, true); Yes, this codepath doesn't execute when in background mode. That's actually what triggered the investigation of this issue. I don't think |browser| can be NULL here because we have a valid profile from that browser that we look up. Having said that, I'm OK with adding a NULL check if you feel that's the right thing to do. On 2012/04/16 22:57:33, Andrew T Wilson wrote: > Agreed. Do we know this is never called with no window open (i.e. while chrome > is in bg mode)? http://codereview.chromium.org/9972012/diff/8001/chrome/installer/util/auto_l... File chrome/installer/util/auto_launch_util.cc (right): http://codereview.chromium.org/9972012/diff/8001/chrome/installer/util/auto_l... chrome/installer/util/auto_launch_util.cc:179: void DisableBackgroundModeAtLogin(const string16& profile_directory) { If you look at SetWillLaunchAtLogin you'll see that for background mode we only use the profile_directory to find the right Run key to modify. We have to find *a* key to attach the background mode to and since background mode is profile-agnostic it makes sense to use Default. I've therefore removed the profile_directory and now specify 'Default' in its place. On 2012/04/17 01:02:11, rpetterson wrote: > I'm concerned this won't have the effect you're looking for. Right now in > chrome_browser_main.cc we autoload any profiles that have background apps, > irrelevant of any command line features. So you may need to add a wrapper around > that call if you're trying to prevent the loading of background apps since this > doesn't seem to change the global background_mode state. http://codereview.chromium.org/9972012/diff/8001/chrome/installer/util/auto_l... File chrome/installer/util/auto_launch_util.h (right): http://codereview.chromium.org/9972012/diff/8001/chrome/installer/util/auto_l... chrome/installer/util/auto_launch_util.h:24: // not blank, must be present for the function to return true. Correct. That function is private now but I'll update the comment. On 2012/04/16 22:57:33, Andrew T Wilson wrote: > Not sure what the behavior is if command_line_switch == "". Is "" the same as > wildcard?
hi finnur. i like how it's coming together. it seems that the naming isn't consistent. could you make a pass to harmonize things (see below) and adjust the comments appropriately? thanks. http://codereview.chromium.org/9972012/diff/15002/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): http://codereview.chromium.org/9972012/diff/15002/chrome/browser/profiles/pro... chrome/browser/profiles/profile_shortcut_manager_win.cc:214: auto_launch_util::DisableAutoStartAtLogin(profile_path.BaseName().value()); LaunchWithWindowAtLogin vs. AutoStartAtLogin? http://codereview.chromium.org/9972012/diff/15002/chrome/installer/util/auto_... File chrome/installer/util/auto_launch_util.h (right): http://codereview.chromium.org/9972012/diff/15002/chrome/installer/util/auto_... chrome/installer/util/auto_launch_util.h:17: // Returns whether the Chrome executable specified in |application_path| is set Suggest: "...the Chrome executable in the directory |application_path|..." http://codereview.chromium.org/9972012/diff/15002/chrome/installer/util/auto_... chrome/installer/util/auto_launch_util.h:18: // to auto-launch at computer startup with a given |command_line_switch|. remove reference to |command_line_switch| from comment http://codereview.chromium.org/9972012/diff/15002/chrome/installer/util/auto_... chrome/installer/util/auto_launch_util.h:19: // NOTE: |application_path| is optional and should be blank in most cases (as nit: move the optional argument(s) after all required arguments. http://codereview.chromium.org/9972012/diff/15002/chrome/installer/util/auto_... chrome/installer/util/auto_launch_util.h:25: // Same as above, except checks whether Chrome is set to launch in Background nit: newline before comment http://codereview.chromium.org/9972012/diff/15002/chrome/installer/util/auto_... chrome/installer/util/auto_launch_util.h:31: const string16& profile_directory); nit: indentation http://codereview.chromium.org/9972012/diff/15002/chrome/installer/util/auto_... chrome/installer/util/auto_launch_util.h:33: // Disables all auto-start features. |profile_directory| is the name of the nit: newline before comment http://codereview.chromium.org/9972012/diff/15002/chrome/installer/util/auto_... chrome/installer/util/auto_launch_util.h:36: void DisableAllAutostartFeatures(const string16& profile_directory); Please harmonize on one term (one of: Autostart, AutoStart, AutoLaunch, auto_launch).
Re: DisableBackgroundModeAtLogin, Got it. SGTM.
Mostly looks good although I have one concern about backwards compatibility. http://codereview.chromium.org/9972012/diff/15002/chrome/browser/background/b... File chrome/browser/background/background_mode_manager_win.cc (right): http://codereview.chromium.org/9972012/diff/15002/chrome/browser/background/b... chrome/browser/background/background_mode_manager_win.cc:31: base::Bind(auto_launch_util::EnableBackgroundModeAtLogin) : To be clear, we're not changing anything about the key we're currently writing (i.e. it's in the same place, etc), correct? I want to make sure that if I have a user who is already in background mode (has a registry key created by the current background mode code), that a call to auto_launch_util::DisableBackgroundModeAtLogin() will remove that old registry key. Otherwise, if the user currently has a background app installed, then uninstalls the app after upgrading to a version containing your patch, chrome will be left in background mode due to the old registry key. http://codereview.chromium.org/9972012/diff/15002/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): http://codereview.chromium.org/9972012/diff/15002/chrome/browser/profiles/pro... chrome/browser/profiles/profile_shortcut_manager_win.cc:214: auto_launch_util::DisableAutoStartAtLogin(profile_path.BaseName().value()); On 2012/04/17 15:08:58, grt wrote: > LaunchWithWindowAtLogin vs. AutoStartAtLogin? Agreed that it would be good to have consistent terminology: BackgroundMode = launch on OS login with no open window AutoStart (or something else) = launch on OS login *with* an open window I'm OK with changing these terms if needed (although BackgroundMode has enough history that I'd be somewhat loath to change it), as long as we're consistent.
Good point on the backward compatibility issue. I've added a migration path that
looks for the "chromium" value containing --no-startup-window and migrates it
into the new auto-start structure.
I've also standardized on the following terminology:
[Enable|Disable][Background|Foreground]StartAtLogin and to turn everything off
you just call DisableAllAutoStartFeatures.
I also combined the getter functions into one:
bool IsAutoStartRequested(const string16& profile_directory,
const FilePath& application_path,
bool window_requested);
In terms of command line flags, if |window_requested| is true, the caller is
interested in --auto-start-chrome, otherwise the caller is interested in
--no-startup-window.
This way, BackgroundMode is more of an auto-start 'flavor', as opposed to being
something else than an auto-start feature.
PS. I tested this by installing a background app (the sample one from atwilson)
and saw that it adds the --no-startup-window to the right key. I didn't test the
app thoroughly, but I did notice two issues (that I'm not sure are related to my
feature):
1) If I close the only Chrome window and log off and log back on again, Chrome
will complain that it didn't shut down correctly, unless I manually shut down
Chrome from the tray menu.
2) Same when uninstalling, the installer will complain there are Chrome windows
open (unless I use the tray).
I don't see how my change would cause this, though. Note: I have a relatively
old tree that I'm working against. I'll update after I upload and build again.
http://codereview.chromium.org/9972012/diff/15002/chrome/browser/background/b...
File chrome/browser/background/background_mode_manager_win.cc (right):
http://codereview.chromium.org/9972012/diff/15002/chrome/browser/background/b...
chrome/browser/background/background_mode_manager_win.cc:31:
base::Bind(auto_launch_util::EnableBackgroundModeAtLogin) :
Not sure I fully understand the question, but the key is in the same place
(under Run), it will just be named
GoogleChromeAutoLaunch_1D82556C434872640A5CBCA7F8955E16 (or whatever the hash
number is for the string Default).
My latest patch will remove any deprecated 'chromium' key when
enabling/disabling an autostart feature and will turn on background mode if
FLAG_ENABLE OR if key 'chromium' exists and FLAG_PRESERVE is set for background
mode.
Hope that answers your question.
On 2012/04/18 01:07:36, Andrew T Wilson wrote:
> To be clear, we're not changing anything about the key we're currently writing
> (i.e. it's in the same place, etc), correct?
>
> I want to make sure that if I have a user who is already in background mode
(has
> a registry key created by the current background mode code), that a call to
> auto_launch_util::DisableBackgroundModeAtLogin() will remove that old registry
> key.
>
> Otherwise, if the user currently has a background app installed, then
uninstalls
> the app after upgrading to a version containing your patch, chrome will be
left
> in background mode due to the old registry key.
Also, Scott and James, can I get OWNERS approval for chrome/browser/ui and chrome/browser/ui/webui/options2 (respectively)?
LGTM
On 2012/04/18 13:51:04, Finnur wrote: > 1) If I close the only Chrome window and log off and log back on again, Chrome > will complain that it didn't shut down correctly, unless I manually shut down > Chrome from the tray menu. This should not happen. This typically means that chrome crashed during shutdown. Any chance you could log out while a debugger is attached to see if it can catch the crash? > 2) Same when uninstalling, the installer will complain there are Chrome windows > open (unless I use the tray). I'm not familiar with the installer code. I'm wondering if it's just checking for the chrome process running, in which case the behavior you describe makes sense.
I'm travelling today, added csilv.
James: Thanks. Andrew: I updated my tree to the latest code and now the crash does not occur, but I still see the "Chrome is still running" on uninstall. This is from DoUninstallTasks in chrome_browser_main.cc. We use some event on Windows to signify that Chrome is running and I think the background mode Chrome might be creating the event when it should not, because this Chrome process is now both doing background tasks (no-window) and showing a window. So we have a long lived process in the background, that used to at one point show a window... Does this ring any bells?
Well, here's what I see: bool already_running = browser_util::IsBrowserAlreadyRunning(); This will get set to true if there's already a chrome browser process running because only one process can create that named singleton Event object. I don't think we want to get rid of that event object just because chrome shifts into background mode, because we actually need to shutdown that process before we uninstall. If we think we need to handle this case more gracefully, we probably need to add code to send an event to the remote process to tell it to exit, and then poll for it to actually shut down. Or change the string in the "you have a chrome window running" dialog to cover background mode. In any case, this behavior isn't new with your CL, so feel free to log a bug for it.
Will do. Thanks. As for the CL, do you think it looks OK?
On 2012/04/18 19:16:28, Finnur wrote: > Will do. Thanks. > > As for the CL, do you think it looks OK? browser_options_handler2.cc owner LGTM
One nit/suggestion that you can ignore, and one question - I think I'm ready to approve but just want to make sure I understand how the backwards-compatibility path works. http://codereview.chromium.org/9972012/diff/23001/chrome/installer/util/auto_... File chrome/installer/util/auto_launch_util.cc (right): http://codereview.chromium.org/9972012/diff/23001/chrome/installer/util/auto_... chrome/installer/util/auto_launch_util.cc:116: bool HasDeprecatedBackgroundModeSwitch() { I think this should probably be called RemoveDeprecated... since it has the side effect of removing the switch. http://codereview.chromium.org/9972012/diff/23001/chrome/installer/util/auto_... chrome/installer/util/auto_launch_util.cc:148: // we need to make an extra call to SetWillLaunchAtLogin. What case does this cover? So, if someone passes in a profile_directory != kInitialProfile, then we write a key, then overwrite it below? Why is this necessary?
http://codereview.chromium.org/9972012/diff/23001/chrome/installer/util/auto_... File chrome/installer/util/auto_launch_util.cc (right): http://codereview.chromium.org/9972012/diff/23001/chrome/installer/util/auto_... chrome/installer/util/auto_launch_util.cc:116: bool HasDeprecatedBackgroundModeSwitch() { Good idea. How about: CheckAndRemoveDeprecatedBackgroundModeSwitch? On 2012/04/18 23:07:51, Andrew T Wilson wrote: > I think this should probably be called RemoveDeprecated... since it has the side > effect of removing the switch. http://codereview.chromium.org/9972012/diff/23001/chrome/installer/util/auto_... chrome/installer/util/auto_launch_util.cc:148: // we need to make an extra call to SetWillLaunchAtLogin. Yes, this is the "profile_directory != kInitialProfile" case. Note that we are calling SetWillLaunchAtLogin with kInitialProfile because we always store the background mode setting with the Default profile. Once that completes we continue below, but note that we are not overwriting the key below since we're dealing with a different Run key (for another profile, since profile_directory != kInitialProfile). On 2012/04/18 23:07:51, Andrew T Wilson wrote: > What case does this cover? So, if someone passes in a profile_directory != > kInitialProfile, then we write a key, then overwrite it below? Why is this > necessary?
LGTM if we add that NOTREACHED() based on our conversation. http://codereview.chromium.org/9972012/diff/23001/chrome/installer/util/auto_... File chrome/installer/util/auto_launch_util.cc (right): http://codereview.chromium.org/9972012/diff/23001/chrome/installer/util/auto_... chrome/installer/util/auto_launch_util.cc:148: // we need to make an extra call to SetWillLaunchAtLogin. Let's put a NOTREACHED here since I don't think this is doing the right thing (we don't want to have two instances of chrome being launched and we need to figure out what the right UX would be if we ever want to support multi-profiles with auto-launch).
rlp: Can you give OWNERS LG for profiles?
Also, I filed: http://code.google.com/p/chromium/issues/detail?id=124376 ... for the uninstall problem.
LGTM. Sorry about that. Meant to before.
rlp: No problem. No worries. All is well. :) |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
