diff options
| -rw-r--r-- | chrome/browser/app_controller_mac.h | 7 | ||||
| -rw-r--r-- | chrome/browser/app_controller_mac.mm | 77 | ||||
| -rw-r--r-- | chrome/browser/browser.cc | 14 | ||||
| -rw-r--r-- | chrome/browser/browser_list.cc | 4 | ||||
| -rw-r--r-- | chrome/browser/browser_list_mac.mm | 15 | ||||
| -rw-r--r-- | chrome/browser/browser_shutdown.cc | 15 | ||||
| -rw-r--r-- | chrome/browser/browser_shutdown.h | 16 | ||||
| -rw-r--r-- | chrome/browser/chrome_browser_application_mac.h | 9 | ||||
| -rw-r--r-- | chrome/browser/chrome_browser_application_mac.mm | 123 | ||||
| -rw-r--r-- | chrome/browser/js_modal_dialog_mac.mm | 6 | ||||
| -rw-r--r-- | chrome/chrome_browser.gypi | 2 |
11 files changed, 217 insertions, 71 deletions
diff --git a/chrome/browser/app_controller_mac.h b/chrome/browser/app_controller_mac.h index 11f705f..86cf2d9 100644 --- a/chrome/browser/app_controller_mac.h +++ b/chrome/browser/app_controller_mac.h @@ -56,6 +56,13 @@ class Profile; - (void)didEndMainMessageLoop; - (Profile*)defaultProfile; +// Try to close all browser windows, and if that succeeds then quit. +- (BOOL)tryToTerminateApplication:(NSApplication*)app; + +// Stop trying to terminate the application. That is, prevent the final browser +// window closure from causing the application to quit. +- (void)stopTryingToTerminateApplication:(NSApplication*)app; + // Show the preferences window, or bring it to the front if it's already // visible. - (IBAction)showPreferences:(id)sender; diff --git a/chrome/browser/app_controller_mac.mm b/chrome/browser/app_controller_mac.mm index 8985761..5da68a3 100644 --- a/chrome/browser/app_controller_mac.mm +++ b/chrome/browser/app_controller_mac.mm @@ -1,4 +1,4 @@ -// Copyright (c) 2009 The Chromium Authors. All rights reserved. +// Copyright (c) 2010 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. @@ -138,6 +138,8 @@ void RecordLastRunAppBundlePath() { @interface AppController(Private) - (void)initMenuState; +- (void)handleQuitEvent:(NSAppleEventDescriptor*)event + withReply:(NSAppleEventDescriptor*)reply; - (void)openUrls:(const std::vector<GURL>&)urls; - (void)getUrl:(NSAppleEventDescriptor*)event withReply:(NSAppleEventDescriptor*)reply; @@ -217,30 +219,66 @@ void RecordLastRunAppBundlePath() { } } +// (NSApplicationDelegate protocol) This is the Apple-approved place to override +// the default handlers. +- (void)applicationWillFinishLaunching:(NSNotification*)notification { + NSAppleEventManager* em = [NSAppleEventManager sharedAppleEventManager]; + [em setEventHandler:self + andSelector:@selector(handleQuitEvent:withReply:) + forEventClass:kCoreEventClass + andEventID:kAEQuitApplication]; +} + +// (NSApplicationDelegate protocol) Our mechanism for application termination +// does not go through |-applicationShouldTerminate:|, so it should not be +// called. In a release build, cancelling termination will prevent a crash (but +// if things go really wrong, the user may have to force-terminate the +// application). - (NSApplicationTerminateReply)applicationShouldTerminate: - (NSApplication *)sender { - // Check for in-progress downloads, and prompt the user if they really want to - // quit (and thus cancel the downloads). - if (![self shouldQuitWithInProgressDownloads]) - return NSTerminateCancel; + (NSApplication*)sender { + NOTREACHED(); + return NSTerminateCancel; +} + +- (BOOL)tryToTerminateApplication:(NSApplication*)app { + // Set the state to "trying to quit", so that closing all browser windows will + // lead to termination. + browser_shutdown::SetTryingToQuit(true); + + // TODO(viettrungluu): Remove Apple Event handlers here? (It's safe to leave + // them in, but I'm not sure about UX; we'd also want to disable other things + // though.) http://crbug.com/40861 + + if (!BrowserList::size()) + return YES; + + // Try to close all the windows. + BrowserList::CloseAllBrowsers(true); + + return NO; +} + +- (void)stopTryingToTerminateApplication:(NSApplication*)app { + if (browser_shutdown::IsTryingToQuit()) { + // Reset the "trying to quit" state, so that closing all browser windows + // will no longer lead to termination. + browser_shutdown::SetTryingToQuit(false); - return NSTerminateNow; + // TODO(viettrungluu): Were we to remove Apple Event handlers above, we + // would have to reinstall them here. http://crbug.com/40861 + } } // Called when the app is shutting down. Clean-up as appropriate. -- (void)applicationWillTerminate:(NSNotification *)aNotification { +- (void)applicationWillTerminate:(NSNotification*)aNotification { NSAppleEventManager* em = [NSAppleEventManager sharedAppleEventManager]; [em removeEventHandlerForEventClass:kInternetEventClass andEventID:kAEGetURL]; [em removeEventHandlerForEventClass:'WWW!' andEventID:'OURL']; - // Close all the windows. - BrowserList::CloseAllBrowsers(true); - - // On Windows, this is done in Browser::OnWindowClosing, but that's not - // appropriate on Mac since we don't shut down when we reach zero windows. - browser_shutdown::OnShutdownStarting(browser_shutdown::BROWSER_EXIT); + // There better be no browser windows left at this point. + CHECK_EQ(BrowserList::size(), 0u); // Release the reference to the browser process. Once all the browsers get // dealloc'd, it will stop the RunLoop and fall back into main(). @@ -395,7 +433,7 @@ void RecordLastRunAppBundlePath() { CFPropertyListRef plist = CFPreferencesCopyAppValue(checkInterval, app); if (!plist) { const float fiveHoursInSeconds = 5.0 * 60.0 * 60.0; - NSNumber *value = [NSNumber numberWithFloat:fiveHoursInSeconds]; + NSNumber* value = [NSNumber numberWithFloat:fiveHoursInSeconds]; CFPreferencesSetAppValue(checkInterval, value, app); CFPreferencesAppSynchronize(app); } @@ -422,7 +460,7 @@ void RecordLastRunAppBundlePath() { // call this from awakeFromNib. NSMenu* view_menu = [[[NSApp mainMenu] itemWithTag:IDC_VIEW_MENU] submenu]; NSMenuItem* encoding_menu_item = [view_menu itemWithTag:IDC_ENCODING_MENU]; - NSMenu *encoding_menu = [encoding_menu_item submenu]; + NSMenu* encoding_menu = [encoding_menu_item submenu]; EncodingMenuControllerDelegate::BuildEncodingMenu([self defaultProfile], encoding_menu); @@ -802,6 +840,13 @@ void RecordLastRunAppBundlePath() { return NULL; } +// (Private) Never call |-applicationShouldTerminate:|; just make everything go +// through |-terminate:|. +- (void)handleQuitEvent:(NSAppleEventDescriptor*)event + withReply:(NSAppleEventDescriptor*)reply { + [NSApp terminate:nil]; +} + // Various methods to open URLs that we get in a native fashion. We use // BrowserInit here because on the other platforms, URLs to open come through // the ProcessSingleton, and it calls BrowserInit. It's best to bottleneck the diff --git a/chrome/browser/browser.cc b/chrome/browser/browser.cc index 1c510dc..dffd1cd 100644 --- a/chrome/browser/browser.cc +++ b/chrome/browser/browser.cc @@ -636,14 +636,18 @@ void Browser::OnWindowClosing() { bool exiting = false; -#if defined(OS_WIN) || defined(OS_LINUX) - // We don't want to do this on Mac since closing all windows isn't a sign - // that the app is shutting down. - if (BrowserList::size() == 1) { +#if defined(OS_MACOSX) + // On Mac, closing the last window isn't usually a sign that the app is + // shutting down. + bool should_quit_if_last_browser = browser_shutdown::IsTryingToQuit(); +#else + bool should_quit_if_last_browser = true; +#endif + + if (should_quit_if_last_browser && BrowserList::size() == 1) { browser_shutdown::OnShutdownStarting(browser_shutdown::WINDOW_CLOSE); exiting = true; } -#endif // Don't use HasSessionService here, we want to force creation of the // session service so that user can restore what was open. diff --git a/chrome/browser/browser_list.cc b/chrome/browser/browser_list.cc index ec7865d1..94b0c6b 100644 --- a/chrome/browser/browser_list.cc +++ b/chrome/browser/browser_list.cc @@ -253,8 +253,8 @@ void BrowserList::CloseAllBrowsersAndExit() { CloseAllBrowsers(true); #else // On the Mac, the application continues to run once all windows are closed. - // Terminate will result in a CloseAllBrowsers(true) call, and additionally, - // will cause the application to exit cleanly. + // Terminate will result in a CloseAllBrowsers(true) call, and once (and if) + // that is done, will cause the application to exit cleanly. chrome_browser_application_mac::Terminate(); #endif } diff --git a/chrome/browser/browser_list_mac.mm b/chrome/browser/browser_list_mac.mm new file mode 100644 index 0000000..f65a3e5 --- /dev/null +++ b/chrome/browser/browser_list_mac.mm @@ -0,0 +1,15 @@ +// Copyright (c) 2010 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "chrome/browser/browser_list.h" + +#include "chrome/browser/browser_shutdown.h" +#import "chrome/browser/chrome_browser_application_mac.h" + +// static +void BrowserList::AllBrowsersClosed() { + // Only terminate after all browsers are closed if trying quit. + if (browser_shutdown::IsTryingToQuit()) + chrome_browser_application_mac::Terminate(); +} diff --git a/chrome/browser/browser_shutdown.cc b/chrome/browser/browser_shutdown.cc index 5d24596..c052790 100644 --- a/chrome/browser/browser_shutdown.cc +++ b/chrome/browser/browser_shutdown.cc @@ -41,6 +41,11 @@ using base::TimeDelta; namespace browser_shutdown { +#if defined(OS_MACOSX) +// Whether the browser is trying to quit (e.g., Quit chosen from menu). +bool g_trying_to_quit = false; +#endif // OS_MACOSX + Time shutdown_started_; ShutdownType shutdown_type_ = NOT_VALID; int shutdown_num_processes_; @@ -226,4 +231,14 @@ void ReadLastShutdownInfo() { &ReadLastShutdownFile, type, num_procs, num_procs_slow)); } +#if defined(OS_MACOSX) +void SetTryingToQuit(bool quitting) { + g_trying_to_quit = quitting; +} + +bool IsTryingToQuit() { + return g_trying_to_quit; +} +#endif + } // namespace browser_shutdown diff --git a/chrome/browser/browser_shutdown.h b/chrome/browser/browser_shutdown.h index 49d800f..3ab2c2e 100644 --- a/chrome/browser/browser_shutdown.h +++ b/chrome/browser/browser_shutdown.h @@ -44,6 +44,22 @@ void Shutdown(); // Called at startup to create a histogram from our previous shutdown time. void ReadLastShutdownInfo(); +#if defined(OS_MACOSX) +// On Mac, closing the last window does not automatically quit the application. +// To actually quit, set a flag which makes final window closure trigger a quit. +// If the quit is aborted, then the flag should be reset (but see notes below on +// the proper way to do this, i.e., usually not using |SetTryingToQuit()|). + +// This is a low-level mutator; in general, don't call it, except from +// appropriate places in the app controller. To quit, use usual means, e.g., +// using |chrome_browser_application_mac::Terminate()|. To stop quitting, use +// |chrome_browser_application_mac::CancelTerminate()|. +void SetTryingToQuit(bool quitting); + +// General accessor. +bool IsTryingToQuit(); +#endif // OS_MACOSX + } // namespace browser_shutdown #endif // CHROME_BROWSER_BROWSER_SHUTDOWN_H__ diff --git a/chrome/browser/chrome_browser_application_mac.h b/chrome/browser/chrome_browser_application_mac.h index 9a1ab9e..67b4230 100644 --- a/chrome/browser/chrome_browser_application_mac.h +++ b/chrome/browser/chrome_browser_application_mac.h @@ -1,4 +1,4 @@ -// Copyright (c) 2009 The Chromium Authors. All rights reserved. +// Copyright (c) 2010 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. @@ -10,6 +10,10 @@ #import "base/chrome_application_mac.h" @interface BrowserCrApplication : CrApplication +// Our implementation of |-terminate:| only attempts to terminate the +// application, i.e., begins a process which may lead to termination. This +// method cancels that process. +- (void)cancelTerminate:(id)sender; @end namespace chrome_browser_application_mac { @@ -34,6 +38,9 @@ namespace chrome_browser_application_mac { // Calls -[NSApp terminate:]. void Terminate(); +// Cancels a termination started by |Terminate()|. +void CancelTerminate(); + } // namespace chrome_browser_application_mac #endif // CHROME_BROWSER_CHROME_BROWSER_APPLICATION_MAC_H_ diff --git a/chrome/browser/chrome_browser_application_mac.mm b/chrome/browser/chrome_browser_application_mac.mm index f373e11..07aac53 100644 --- a/chrome/browser/chrome_browser_application_mac.mm +++ b/chrome/browser/chrome_browser_application_mac.mm @@ -1,4 +1,4 @@ -// Copyright (c) 2009 The Chromium Authors. All rights reserved. +// Copyright (c) 2010 The Chromium Authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. @@ -9,6 +9,7 @@ #import "base/scoped_nsobject.h" #import "base/sys_string_conversions.h" #import "chrome/app/breakpad_mac.h" +#import "chrome/browser/app_controller_mac.h" #import "chrome/browser/cocoa/objc_method_swizzle.h" // The implementation of NSExceptions break various assumptions in the @@ -120,6 +121,10 @@ void Terminate() { [NSApp terminate:nil]; } +void CancelTerminate() { + [NSApp cancelTerminate:nil]; +} + } // namespace chrome_browser_application_mac namespace { @@ -161,56 +166,80 @@ BOOL SwizzleNSExceptionInit() { return [super init]; } -// -terminate: is the entry point for orderly "quit" operations in Cocoa. -// This includes the application menu's quit menu item and keyboard -// equivalent, the application's dock icon menu's quit menu item, "quit" (not -// "force quit") in the Activity Monitor, and quits triggered by user logout -// and system restart and shutdown. +//////////////////////////////////////////////////////////////////////////////// +// HISTORICAL COMMENT (by viettrungluu, from +// http://codereview.chromium.org/1520006 with mild editing): +// +// A quick summary of the state of things (before the changes to shutdown): +// +// Currently, we are totally hosed (put in a bad state in which Cmd-W does the +// wrong thing, and which will probably eventually lead to a crash) if we begin +// quitting but termination is aborted for some reason. +// +// I currently know of two ways in which termination can be aborted: +// (1) Common case: a window has an onbeforeunload handler which pops up a +// "leave web page" dialog, and the user answers "no, don't leave". +// (2) Uncommon case: popups are enabled (in Content Settings, i.e., the popup +// blocker is disabled), and some nasty web page pops up a new window on +// closure. +// +// I don't know of other ways in which termination can be aborted, but they may +// exist (or may be added in the future, for that matter). // -// The default NSApplication -terminate: implementation will end the process -// by calling exit(), and thus never leave the main run loop. This is -// unsuitable for Chrome's purposes. Chrome depends on leaving the main -// run loop to perform a proper orderly shutdown. This design is ingrained -// in the application and the assumptions that its code makes, and is -// entirely reasonable and works well on other platforms, but it's not -// compatible with the standard Cocoa quit sequence. Quits originated from -// within the application can be redirected to not use -terminate:, but -// quits from elsewhere cannot be. +// My CL [see above] does the following: +// a. Should prevent being put in a bad state (which breaks Cmd-W and leads to +// crash) under all circumstances. +// b. Should completely handle (1) properly. +// c. Doesn't (yet) handle (2) properly and puts it in a weird state (but not +// that bad). +// d. Any other ways of aborting termination would put it in that weird state. // -// To allow the Cocoa-based Chrome to support the standard Cocoa -terminate: -// interface, and allow all quits to cause Chrome to shut down properly -// regardless of their origin, -terminate: is overriden. The custom -// -terminate: does not end the application with exit(). Instead, it simply -// returns after posting the normal NSApplicationWillTerminateNotification -// notification. The application is responsible for exiting on its own in -// whatever way it deems appropriate. In Chrome's case, the main run loop will -// end and the applicaton will exit by returning from main(). +// c. can be fixed by having the global flag reset on browser creation or +// similar (and doing so might also fix some possible d.'s as well). I haven't +// done this yet since I haven't thought about it carefully and since it's a +// corner case. // -// This implementation of -terminate: is scaled back and is not as -// fully-featured as the implementation in NSApplication, nor is it a direct -// drop-in replacement -terminate: in most applications. It is -// purpose-specific to Chrome. +// The weird state: a state in which closing the last window quits the browser. +// This might be a bit annoying, but it's not dangerous in any way. +//////////////////////////////////////////////////////////////////////////////// + +// |-terminate:| is the entry point for orderly "quit" operations in Cocoa. This +// includes the application menu's quit menu item and keyboard equivalent, the +// application's dock icon menu's quit menu item, "quit" (not "force quit") in +// the Activity Monitor, and quits triggered by user logout and system restart +// and shutdown. +// +// The default |-terminate:| implementation ends the process by calling exit(), +// and thus never leaves the main run loop. This is unsuitable for Chrome since +// Chrome depends on leaving the main run loop to perform an orderly shutdown. +// We support the normal |-terminate:| interface by overriding the default +// implementation. Our implementation, which is very specific to the needs of +// Chrome, works by asking the application delegate to terminate using its +// |-tryToTerminateApplication:| method. +// +// |-tryToTerminateApplication:| differs from the standard +// |-applicationShouldTerminate:| in that no special event loop is run in the +// case that immediate termination is not possible (e.g., if dialog boxes +// allowing the user to cancel have to be shown). Instead, this method sets a +// flag and tries to close all browsers. This flag causes the closure of the +// final browser window to begin actual tear-down of the application. +// Termination is cancelled by resetting this flag. The standard +// |-applicationShouldTerminate:| is not supported, and code paths leading to it +// must be redirected. - (void)terminate:(id)sender { - NSApplicationTerminateReply shouldTerminate = NSTerminateNow; - SEL selector = @selector(applicationShouldTerminate:); - if ([[self delegate] respondsToSelector:selector]) - shouldTerminate = [[self delegate] applicationShouldTerminate:self]; - - // If shouldTerminate is NSTerminateLater, the application is expected to - // call -replyToApplicationShouldTerminate: when it knows whether or not it - // should terminate. If the argument is YES, - // -replyToApplicationShouldTerminate: will call -terminate:. This will - // result in another call to the delegate's -applicationShouldTerminate:, - // which would be expected to return NSTerminateNow at that point. - if (shouldTerminate != NSTerminateNow) - return; - - [[NSNotificationCenter defaultCenter] - postNotificationName:NSApplicationWillTerminateNotification - object:self]; - - // Return, don't exit. The application is responsible for exiting on its - // own. + AppController* appController = static_cast<AppController*>([NSApp delegate]); + if ([appController tryToTerminateApplication:self]) { + [[NSNotificationCenter defaultCenter] + postNotificationName:NSApplicationWillTerminateNotification + object:self]; + } + + // Return, don't exit. The application is responsible for exiting on its own. +} + +- (void)cancelTerminate:(id)sender { + AppController* appController = static_cast<AppController*>([NSApp delegate]); + [appController stopTryingToTerminateApplication:self]; } - (BOOL)sendAction:(SEL)anAction to:(id)aTarget from:(id)sender { diff --git a/chrome/browser/js_modal_dialog_mac.mm b/chrome/browser/js_modal_dialog_mac.mm index 3bdc9aa..d15e6ad 100644 --- a/chrome/browser/js_modal_dialog_mac.mm +++ b/chrome/browser/js_modal_dialog_mac.mm @@ -10,6 +10,7 @@ #include "app/message_box_flags.h" #import "base/cocoa_protocols_mac.h" #include "base/sys_string_conversions.h" +#import "chrome/browser/chrome_browser_application_mac.h" #include "grit/app_strings.h" #include "grit/generated_resources.h" @@ -68,6 +69,11 @@ break; } case NSAlertSecondButtonReturn: { // Cancel + // If the user wants to stay on this page, stop quitting (if a quit is in + // progress). + if (bridge->is_before_unload_dialog()) + chrome_browser_application_mac::CancelTerminate(); + bridge->OnCancel(); break; } diff --git a/chrome/chrome_browser.gypi b/chrome/chrome_browser.gypi index b323f802..715fc39 100644 --- a/chrome/chrome_browser.gypi +++ b/chrome/chrome_browser.gypi @@ -250,6 +250,7 @@ 'browser/browser_init.cc', 'browser/browser_init.h', 'browser/browser_list_gtk.cc', + 'browser/browser_list_mac.mm', 'browser/browser_list_stub.cc', 'browser/browser_list_win.cc', 'browser/browser_list.cc', @@ -2544,6 +2545,7 @@ 'browser/automation/automation_provider_list_generic.cc', 'browser/bookmarks/bookmark_context_menu.cc', 'browser/bookmarks/bookmark_drop_info.cc', + 'browser/browser_list_stub.cc', 'browser/dock_info.cc', 'browser/importer/nss_decryptor_system_nss.cc', 'browser/importer/nss_decryptor_system_nss.h', |