From 70593e50a3bb6b26e57080b93b55a3baeedd5db2 Mon Sep 17 00:00:00 2001 From: "jackhou@chromium.org" Date: Thu, 15 May 2014 08:30:23 +0000 Subject: [Mac] Close browsers when quitting is prevented by QuitWithAppsController. Follow-up to https://codereview.chromium.org/220373003/ This closes all browser windows when quitting, even if Chrome is kept alive by QuitWithAppsController. TEST=Enable --apps-keep-chrome-alive in chrome://flags. Enable session restore in settings ("Continue where you left off"). Start Chrome and open an extra tab and browser window. Open an app from chrome://apps. Quit Chrome. i.e. via Dock->Quit, Menu->Quit, or Cmd+Q. Chrome and the app should stay running but browser windows should close. TEST=Repeat the first test and then: Close the app window, Chrome should quit. TEST=Repeat the first test and then: Click on the Chrome dock icon, the previous session should be restored. BUG=333429 Review URL: https://codereview.chromium.org/253383004 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@270638 0039d316-1c4b-4281-b951-d872f2087c98 --- apps/app_shim/app_shim_handler_mac.cc | 37 +++++++++++++++----- apps/app_shim/app_shim_handler_mac.h | 5 +++ chrome/browser/app_controller_mac.mm | 39 ++++++++++++--------- ..._with_apps_controller_mac_interactive_uitest.cc | 40 ++++++++++++---------- .../ui/cocoa/confirm_quit_panel_controller.mm | 6 +++- 5 files changed, 84 insertions(+), 43 deletions(-) diff --git a/apps/app_shim/app_shim_handler_mac.cc b/apps/app_shim/app_shim_handler_mac.cc index cfbf4ff..7e67e73 100644 --- a/apps/app_shim/app_shim_handler_mac.cc +++ b/apps/app_shim/app_shim_handler_mac.cc @@ -61,7 +61,7 @@ class AppShimHandlerRegistry : public content::NotificationObserver { } void MaybeTerminate() { - if (!browser_opened_ever_) { + if (!browser_session_running_) { // Post this to give AppWindows a chance to remove themselves from the // registry. base::MessageLoop::current()->PostTask( @@ -69,16 +69,26 @@ class AppShimHandlerRegistry : public content::NotificationObserver { } } + bool ShouldRestoreSession() { + return !browser_session_running_; + } + private: friend struct DefaultSingletonTraits; typedef std::map HandlerMap; AppShimHandlerRegistry() : default_handler_(NULL), - browser_opened_ever_(false) { + browser_session_running_(false) { registrar_.Add( this, chrome::NOTIFICATION_BROWSER_OPENED, content::NotificationService::AllBrowserContextsAndSources()); + registrar_.Add( + this, chrome::NOTIFICATION_CLOSE_ALL_BROWSERS_REQUEST, + content::NotificationService::AllBrowserContextsAndSources()); + registrar_.Add( + this, chrome::NOTIFICATION_BROWSER_CLOSE_CANCELLED, + content::NotificationService::AllBrowserContextsAndSources()); } virtual ~AppShimHandlerRegistry() {} @@ -88,17 +98,23 @@ class AppShimHandlerRegistry : public content::NotificationObserver { int type, const content::NotificationSource& source, const content::NotificationDetails& details) OVERRIDE { - DCHECK_EQ(chrome::NOTIFICATION_BROWSER_OPENED, type); - registrar_.Remove( - this, chrome::NOTIFICATION_BROWSER_OPENED, - content::NotificationService::AllBrowserContextsAndSources()); - browser_opened_ever_ = true; + switch (type) { + case chrome::NOTIFICATION_BROWSER_OPENED: + case chrome::NOTIFICATION_BROWSER_CLOSE_CANCELLED: + browser_session_running_ = true; + break; + case chrome::NOTIFICATION_CLOSE_ALL_BROWSERS_REQUEST: + browser_session_running_ = false; + break; + default: + NOTREACHED(); + } } HandlerMap handlers_; AppShimHandler* default_handler_; content::NotificationRegistrar registrar_; - bool browser_opened_ever_; + bool browser_session_running_; DISALLOW_COPY_AND_ASSIGN(AppShimHandlerRegistry); }; @@ -132,4 +148,9 @@ void AppShimHandler::MaybeTerminate() { AppShimHandlerRegistry::GetInstance()->MaybeTerminate(); } +// static +bool AppShimHandler::ShouldRestoreSession() { + return AppShimHandlerRegistry::GetInstance()->ShouldRestoreSession(); +} + } // namespace apps diff --git a/apps/app_shim/app_shim_handler_mac.h b/apps/app_shim/app_shim_handler_mac.h index 0ecde07..9e53e0e 100644 --- a/apps/app_shim/app_shim_handler_mac.h +++ b/apps/app_shim/app_shim_handler_mac.h @@ -56,6 +56,11 @@ class AppShimHandler { // shell windows, and the app list is not visible. static void MaybeTerminate(); + // Whether browser sessions should be restored right now. This is true if + // the browser has been quit but kept alive because Chrome Apps are still + // running. + static bool ShouldRestoreSession(); + // Invoked by the shim host when the shim process is launched. The handler // must call OnAppLaunchComplete to inform the shim of the result. // |launch_type| indicates the type of launch. diff --git a/chrome/browser/app_controller_mac.mm b/chrome/browser/app_controller_mac.mm index b863936..22ec9a9 100644 --- a/chrome/browser/app_controller_mac.mm +++ b/chrome/browser/app_controller_mac.mm @@ -372,12 +372,6 @@ class AppControllerProfileObserver : public ProfileInfoCacheObserver { ![self shouldQuitWithInProgressDownloads]) return NO; - // Check for active apps, and prompt the user if they really want to quit - // (and also quit the apps). - if (!browser_shutdown::IsTryingToQuit() && - quitWithAppsController_.get() && !quitWithAppsController_->ShouldQuit()) - return NO; - // 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 @@ -388,6 +382,19 @@ class AppControllerProfileObserver : public ProfileInfoCacheObserver { [self applicationShouldTerminate:app] != NSTerminateNow) return NO; + // Check for active apps. If quitting is prevented, only close browsers and + // sessions. + if (!browser_shutdown::IsTryingToQuit() && + quitWithAppsController_ && !quitWithAppsController_->ShouldQuit()) { + content::NotificationService::current()->Notify( + chrome::NOTIFICATION_CLOSE_ALL_BROWSERS_REQUEST, + content::NotificationService::AllSources(), + content::NotificationService::NoDetails()); + // This will close all browser sessions. + chrome::CloseAllBrowsers(); + return NO; + } + size_t num_browsers = chrome::GetTotalBrowserCount(); // Initiate a shutdown (via chrome::CloseAllBrowsersAndQuit()) if we aren't @@ -1208,16 +1215,16 @@ class AppControllerProfileObserver : public ProfileInfoCacheObserver { // Normally, it'd just open a new empty page. { static BOOL doneOnce = NO; - if (!doneOnce) { - doneOnce = YES; - if (base::mac::WasLaunchedAsHiddenLoginItem()) { - SessionService* sessionService = - SessionServiceFactory::GetForProfileForSessionRestore( - [self lastProfile]); - if (sessionService && - sessionService->RestoreIfNecessary(std::vector())) - return NO; - } + BOOL attemptRestore = apps::AppShimHandler::ShouldRestoreSession() || + (!doneOnce && base::mac::WasLaunchedAsHiddenLoginItem()); + doneOnce = YES; + if (attemptRestore) { + SessionService* sessionService = + SessionServiceFactory::GetForProfileForSessionRestore( + [self lastProfile]); + if (sessionService && + sessionService->RestoreIfNecessary(std::vector())) + return NO; } } diff --git a/chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac_interactive_uitest.cc b/chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac_interactive_uitest.cc index 51a8acb..18eb721 100644 --- a/chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac_interactive_uitest.cc +++ b/chrome/browser/ui/cocoa/apps/quit_with_apps_controller_mac_interactive_uitest.cc @@ -7,8 +7,10 @@ #include "apps/app_window_registry.h" #include "apps/ui/native_app_window.h" #include "base/command_line.h" +#include "base/run_loop.h" #include "chrome/browser/apps/app_browsertest_util.h" #include "chrome/browser/browser_process.h" +#include "chrome/browser/chrome_browser_application_mac.h" #include "chrome/browser/extensions/extension_service.h" #include "chrome/browser/extensions/extension_test_message_listener.h" #include "chrome/browser/lifetime/application_lifetime.h" @@ -39,14 +41,6 @@ class QuitWithAppsControllerInteractiveTest command_line->AppendSwitch(switches::kAppsKeepChromeAlive); } - virtual void SetUpOnMainThread() OVERRIDE { - ExtensionBrowserTest::SetUpOnMainThread(); - - ExtensionTestMessageListener listener("Launched", false); - app_ = InstallAndLaunchPlatformApp("minimal_id"); - ASSERT_TRUE(listener.WaitUntilSatisfied()); - } - const extensions::Extension* app_; private: @@ -62,6 +56,18 @@ IN_PROC_BROWSER_TEST_F(QuitWithAppsControllerInteractiveTest, QuitBehavior) { const Notification* notification; message_center::MessageCenter* message_center = message_center::MessageCenter::Get(); + + // With no app windows open, ShouldQuit returns true. + EXPECT_TRUE(controller->ShouldQuit()); + notification = g_browser_process->notification_ui_manager()->FindById( + QuitWithAppsController::kQuitWithAppsNotificationID); + EXPECT_EQ(NULL, notification); + + // Open an app window. + ExtensionTestMessageListener listener("Launched", false); + app_ = InstallAndLaunchPlatformApp("minimal_id"); + ASSERT_TRUE(listener.WaitUntilSatisfied()); + // One browser and one app window at this point. EXPECT_FALSE(chrome::BrowserIterator().done()); EXPECT_TRUE(apps::AppWindowRegistry::IsAppWindowRegisteredInAnyProfile(0)); @@ -97,12 +103,11 @@ IN_PROC_BROWSER_TEST_F(QuitWithAppsControllerInteractiveTest, QuitBehavior) { EXPECT_FALSE(chrome::BrowserIterator().done()); EXPECT_TRUE(apps::AppWindowRegistry::IsAppWindowRegisteredInAnyProfile(0)); - // Normally, quitting would close all browsers, but since we're just - // simulating a quit, close it here. + // Quitting should not quit but close all browsers content::WindowedNotificationObserver observer( chrome::NOTIFICATION_BROWSER_CLOSED, content::NotificationService::AllSources()); - chrome::BrowserIterator()->window()->Close(); + chrome_browser_application_mac::Terminate(); observer.Wait(); EXPECT_TRUE(chrome::BrowserIterator().done()); @@ -114,14 +119,13 @@ IN_PROC_BROWSER_TEST_F(QuitWithAppsControllerInteractiveTest, QuitBehavior) { QuitWithAppsController::kQuitWithAppsNotificationID); ASSERT_TRUE(notification); - // Clicking "Quit All Apps." button closes all app windows. + // Clicking "Quit All Apps." button closes all app windows. With no browsers + // open, this should also quit Chrome. + content::WindowedNotificationObserver quit_observer( + chrome::NOTIFICATION_APP_TERMINATING, + content::NotificationService::AllSources()); notification->delegate()->ButtonClick(0); message_center->RemoveAllNotifications(false); EXPECT_FALSE(apps::AppWindowRegistry::IsAppWindowRegisteredInAnyProfile(0)); - - // With no app windows open, ShouldQuit returns true. - EXPECT_TRUE(controller->ShouldQuit()); - notification = g_browser_process->notification_ui_manager()->FindById( - QuitWithAppsController::kQuitWithAppsNotificationID); - EXPECT_EQ(NULL, notification); + quit_observer.Wait(); } diff --git a/chrome/browser/ui/cocoa/confirm_quit_panel_controller.mm b/chrome/browser/ui/cocoa/confirm_quit_panel_controller.mm index 499dc93..ad2c835 100644 --- a/chrome/browser/ui/cocoa/confirm_quit_panel_controller.mm +++ b/chrome/browser/ui/cocoa/confirm_quit_panel_controller.mm @@ -15,6 +15,7 @@ #include "chrome/browser/browser_process.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/profiles/profile_manager.h" +#import "chrome/browser/ui/cocoa/browser_window_controller.h" #include "chrome/browser/ui/cocoa/confirm_quit.h" #include "chrome/common/pref_names.h" #include "grit/generated_resources.h" @@ -154,7 +155,10 @@ void RegisterLocalState(PrefRegistrySimple* registry) { - (void)setCurrentProgress:(NSAnimationProgress)progress { for (NSWindow* window in [application_ windows]) { - [window setAlphaValue:1.0 - progress]; + if ([[window windowController] + isKindOfClass:[BrowserWindowController class]]) { + [window setAlphaValue:1.0 - progress]; + } } } -- cgit v1.1