Remove details from BROWSER_CLOSING and BROWSER_CLOSED notifications.

The details was a boolean indicating whether this is the last browser window closing. As part of this change the logic for saving pinned tabs has been changed to not use the boolean in the details, and has been updated to handle the case where a user exit or closing of the last window is cancelled by an onbeforeunload event.

Also as part of this change, both the pinned tab and multi profile features have been updated to handle cases of the browser process not being closed due to background mode or (in the future) packaged apps, and then more browser windows being opened.

BUG=None
TEST=Test shutdown 


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148281

Review URL: https://chromiumcodereview.appspot.com/10800031

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@148741 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/automation/automation_provider_observers.cc

@@ -878,7 +878,9 @@ void BrowserClosedNotificationObserver::Observe(
     return;
   }
 
-  content::Details<bool> close_app(details);
+  int browser_count = static_cast<int>(BrowserList::size());
+  // We get the notification before the browser is removed from the BrowserList.
+  bool app_closing = browser_count == 1;
 
   if (use_json_interface_) {
     AutomationJSONReply(automation_,
@@ -889,7 +891,7 @@ void BrowserClosedNotificationObserver::Observe(
                                                            true);
     } else {
       AutomationMsg_CloseBrowser::WriteReplyParams(reply_message_.get(), true,
-                                                   *(close_app.ptr()));
+                                                   app_closing);
     }
     automation_->Send(reply_message_.release());
   }

chrome/browser/profiles/profile_manager.cc

@@ -561,8 +561,13 @@ void ProfileManager::Observe(
       DCHECK(profile);
       if (!profile->IsOffTheRecord() && ++browser_counts_[profile] == 1) {
         active_profiles_.push_back(profile);
-        save_active_profiles = !closing_all_browsers_;
+        save_active_profiles = true;
       }
+      // If browsers are opening, we can't be closing all the browsers. This
+      // can happen if the application was exited, but background mode or
+      // packaged apps prevented the process from shutting down, and then
+      // a new browser window was opened.
+      closing_all_browsers_ = false;
       break;
     }
     case chrome::NOTIFICATION_BROWSER_CLOSED: {

chrome/browser/ui/browser.cc

@@ -588,8 +588,6 @@ void Browser::OnWindowClosing() {
   if (!ShouldCloseWindow())
     return;
 
-  bool exiting = false;
-
   // Application should shutdown on last window close if the user is explicitly
   // trying to quit, or if there is nothing keeping the browser alive (such as
   // AppController on the Mac, or BackgroundContentsService for background
@@ -597,10 +595,8 @@ void Browser::OnWindowClosing() {
   bool should_quit_if_last_browser =
       browser_shutdown::IsTryingToQuit() || !browser::WillKeepAlive();
 
-  if (should_quit_if_last_browser && BrowserList::size() == 1) {
+  if (should_quit_if_last_browser && BrowserList::size() == 1)
     browser_shutdown::OnShutdownStarting(browser_shutdown::WINDOW_CLOSE);
-    exiting = true;
-  }
 
   // Don't use GetForProfileIfExisting here, we want to force creation of the
   // session service so that user can restore what was open.
@@ -624,7 +620,7 @@ void Browser::OnWindowClosing() {
   content::NotificationService::current()->Notify(
       chrome::NOTIFICATION_BROWSER_CLOSING,
       content::Source<Browser>(this),
-      content::Details<bool>(&exiting));
+      content::NotificationService::NoDetails());
 
   chrome::CloseAllTabs(this);
 }

chrome/browser/ui/browser_list_impl.cc

@@ -70,28 +70,10 @@ void BrowserListImpl::AddBrowser(Browser* browser) {
 void BrowserListImpl::RemoveBrowser(Browser* browser) {
   RemoveBrowserFrom(browser, &last_active_browsers_);
 
-  // Many UI tests rely on closing the last browser window quitting the
-  // application.
-  // Mac: Closing all windows does not indicate quitting the application. Lie
-  // for now and ignore behavior outside of unit tests.
-  // ChromeOS: Force closing last window to close app with flag.
-  // TODO(andybons | pkotwicz): Fix the UI tests to Do The Right Thing.
-#if defined(OS_CHROMEOS)
-  bool closing_app;
-  if (CommandLine::ForCurrentProcess()->HasSwitch(
-      switches::kDisableZeroBrowsersOpenForTests))
-    closing_app = (browsers_.size() == 1);
-  else
-    closing_app = (browsers_.size() == 1 &&
-        browser_shutdown::IsTryingToQuit());
-#else
-  bool closing_app = (browsers_.size() == 1);
-#endif  // OS_CHROMEOS
-
   content::NotificationService::current()->Notify(
       chrome::NOTIFICATION_BROWSER_CLOSED,
       content::Source<Browser>(browser),
-      content::Details<bool>(&closing_app));
+      content::NotificationService::NoDetails());
 
   RemoveBrowserFrom(browser, &browsers_);
 

chrome/browser/ui/tabs/pinned_tab_service.cc

@@ -11,7 +11,9 @@
 #include "chrome/common/chrome_notification_types.h"
 #include "content/public/browser/notification_service.h"
 
-static bool IsLastNormalBrowser(Browser* browser) {
+namespace {
+
+bool IsOnlyNormalBrowser(Browser* browser) {
   for (BrowserList::const_iterator i = BrowserList::begin();
        i != BrowserList::end(); ++i) {
     if (*i != browser && (*i)->is_type_tabbed() &&
@@ -22,9 +24,11 @@ static bool IsLastNormalBrowser(Browser* browser) {
   return true;
 }
 
+}  // namespace
+
 PinnedTabService::PinnedTabService(Profile* profile)
     : profile_(profile),
-      got_exiting_(false),
+      save_pinned_tabs_(true),
       has_normal_browser_(false) {
   registrar_.Add(this, chrome::NOTIFICATION_BROWSER_OPENED,
                  content::NotificationService::AllBrowserContextsAndSources());
@@ -32,14 +36,30 @@ PinnedTabService::PinnedTabService(Profile* profile)
                  content::NotificationService::AllSources());
   registrar_.Add(this, chrome::NOTIFICATION_CLOSE_ALL_BROWSERS_REQUEST,
                  content::NotificationService::AllSources());
+  registrar_.Add(this, chrome::NOTIFICATION_TAB_ADDED,
+                 content::NotificationService::AllSources());
 }
 
 void PinnedTabService::Observe(int type,
                                const content::NotificationSource& source,
                                const content::NotificationDetails& details) {
-  if (got_exiting_)
-    return;
-
+  // Saving of tabs happens when saving is enabled, and when either the user
+  // exits the application or closes the last browser window.
+  // Saving is disabled when the user exits the application to prevent the
+  // pin state of all the open browsers being overwritten by the state of the
+  // last browser window to close.
+  // Saving is re-enabled when a browser window or tab is opened again.
+  // Note, cancelling a shutdown (via onbeforeunload) will not re-enable pinned
+  // tab saving immediately, to prevent the following situation:
+  //   * two windows are open, one with pinned tabs
+  //   * user exits
+  //   * pinned tabs are saved
+  //   * window with pinned tabs is closed
+  //   * other window blocks close with onbeforeunload
+  //   * user saves work, etc. then closes the window
+  //   * pinned tabs are saved, without the window with the pinned tabs,
+  //     over-writing the correct state.
+  // Saving is re-enabled if a new tab or window is opened.
   switch (type) {
     case chrome::NOTIFICATION_BROWSER_OPENED: {
       Browser* browser = content::Source<Browser>(source).ptr();
@@ -47,15 +67,20 @@ void PinnedTabService::Observe(int type,
           browser->profile() == profile_) {
         has_normal_browser_ = true;
       }
+      save_pinned_tabs_ = true;
+      break;
+    }
+
+    case chrome::NOTIFICATION_TAB_ADDED: {
+      save_pinned_tabs_ = true;
       break;
     }
 
     case chrome::NOTIFICATION_BROWSER_CLOSING: {
       Browser* browser = content::Source<Browser>(source).ptr();
-      if (has_normal_browser_ && browser->profile() == profile_) {
-        if (*(content::Details<bool>(details)).ptr()) {
-          GotExit();
-        } else if (IsLastNormalBrowser(browser)) {
+      if (has_normal_browser_ && save_pinned_tabs_ &&
+          browser->profile() == profile_) {
+        if (IsOnlyNormalBrowser(browser)) {
           has_normal_browser_ = false;
           PinnedTabCodec::WritePinnedTabs(profile_);
         }
@@ -64,8 +89,10 @@ void PinnedTabService::Observe(int type,
     }
 
     case chrome::NOTIFICATION_CLOSE_ALL_BROWSERS_REQUEST: {
-      if (has_normal_browser_)
-        GotExit();
+      if (has_normal_browser_ && save_pinned_tabs_) {
+        PinnedTabCodec::WritePinnedTabs(profile_);
+        save_pinned_tabs_ = false;
+      }
       break;
     }
 
@@ -73,9 +100,3 @@ void PinnedTabService::Observe(int type,
       NOTREACHED();
   }
 }
-
-void PinnedTabService::GotExit() {
-  DCHECK(!got_exiting_);
-  got_exiting_ = true;
-  PinnedTabCodec::WritePinnedTabs(profile_);
-}

chrome/browser/ui/tabs/pinned_tab_service.h

@@ -21,9 +21,6 @@ class PinnedTabService : public content::NotificationObserver,
   explicit PinnedTabService(Profile* profile);
 
  private:
-  // Invoked when we're about to exit.
-  void GotExit();
-
   // content::NotificationObserver.
   virtual void Observe(int type,
                        const content::NotificationSource& source,
@@ -31,9 +28,9 @@ class PinnedTabService : public content::NotificationObserver,
 
   Profile* profile_;
 
-  // If true we've seen an exit event (or the last browser is closing which
-  // triggers an exit) and can ignore all other events.
-  bool got_exiting_;
+  // True if we should save the pinned tabs when a browser window closes or the
+  // user exits the application.
+  bool save_pinned_tabs_;
 
   // True if there is at least one normal browser for our profile.
   bool has_normal_browser_;

chrome/common/chrome_notification_types.h

@@ -25,20 +25,13 @@ enum NotificationType {
   NOTIFICATION_BROWSER_WINDOW_READY,
 
   // This message is sent when a browser is closing. The source is a
-  // Source<Browser> containing the affected Browser. Details is a boolean
-  // that if true indicates that the application will be closed as a result of
-  // this browser window closure (i.e. this was the last opened browser
-  // window on win/linux). This is sent prior to BROWSER_CLOSED, and may be
-  // sent more than once for a particular browser.
+  // Source<Browser> containing the affected Browser. No details are expected.
+  // This is sent prior to BROWSER_CLOSED, and may be sent more than once for a
+  // particular browser.
   NOTIFICATION_BROWSER_CLOSING,
 
   // This message is sent after a window has been closed.  The source is a
-  // Source<Browser> containing the affected Browser.  Details is a boolean
-  // that if true indicates that the last browser window has closed - this
-  // does not indicate that the application is exiting (observers should
-  // listen for APP_TERMINATING if they want to detect when the application
-  // will shut down). Note that the boolean pointed to by details is only
-  // valid for the duration of this call.
+  // Source<Browser> containing the affected Browser.  No details are exptected.
   NOTIFICATION_BROWSER_CLOSED,
 
   // This message is sent when closing a browser has been cancelled, either by