Nicely handle OnBeforeUnloads with guest and lock mode.

chrome/browser/ui/browser_list.cc

 // Copyright (c) 2012 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/ui/browser_list.h"
 
 #include <algorithm>
 
 #include "base/logging.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/browser_shutdown.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/lifetime/application_lifetime.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/profiles/profile_info_cache.h"
+#include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/browser_iterator.h"
 #include "chrome/browser/ui/browser_list_observer.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/host_desktop.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/user_metrics.h"
 
 using base::UserMetricsAction;
 using content::WebContents;
 
 // static
 base::LazyInstance<ObserverList<chrome::BrowserListObserver> >::Leaky
     BrowserList::observers_ = LAZY_INSTANCE_INITIALIZER;
 
 // static
 BrowserList* BrowserList::native_instance_ = NULL;
 BrowserList* BrowserList::ash_instance_ = NULL;
 
+// static
+// The current browser handling its OnBeforeUnload event before closing.
+Browser* current_closing_browser_;

[Peter Kasting, 2014/08/28 18:46:12]

You don't care about the actual pointer value here, so this could at least be
changed to a bool.

[Mike Lerman, 2014/08/29 18:02:21]

Done.

+
 ////////////////////////////////////////////////////////////////////////////////
 // BrowserList, public:
 
 Browser* BrowserList::GetLastActive() const {
   if (!last_active_browsers_.empty())
     return *(last_active_browsers_.rbegin());
   return NULL;
 }
 
 // static
@@ skipping 64 matching lines @@
 // static
 void BrowserList::AddObserver(chrome::BrowserListObserver* observer) {
   observers_.Get().AddObserver(observer);
 }
 
 // static
 void BrowserList::RemoveObserver(chrome::BrowserListObserver* observer) {
   observers_.Get().RemoveObserver(observer);
 }
 
+// static
 void BrowserList::CloseAllBrowsersWithProfile(Profile* profile) {
   BrowserVector browsers_to_close;
   for (chrome::BrowserIterator it; !it.done(); it.Next()) {
     if (it->profile()->GetOriginalProfile() == profile->GetOriginalProfile())
       browsers_to_close.push_back(*it);
   }
 
   for (BrowserVector::const_iterator it = browsers_to_close.begin();
        it != browsers_to_close.end(); ++it) {
     (*it)->window()->Close();
   }
 }
 
 // static
+void BrowserList::CloseAllBrowsersWithProfile(Profile* profile,
+    const base::Callback<void(size_t)>& on_close_success) {
+  current_closing_browser_ = NULL;
+  BrowserVector browsers_to_close;
+  for (chrome::BrowserIterator it; !it.done(); it.Next()) {
+    if (it->profile()->GetOriginalProfile() == profile->GetOriginalProfile())
+      browsers_to_close.push_back(*it);
+  }
+
+  size_t profile_index = g_browser_process->profile_manager()->
+      GetProfileInfoCache().GetIndexOfProfileWithPath(profile->GetPath());
+  TryToCloseBrowserList(browsers_to_close, on_close_success, profile_index);
+}
+
+// static
+void BrowserList::TryToCloseBrowserList(const BrowserVector& browsers_to_close,
+    const base::Callback<void(size_t)>& on_close_success,
+    const size_t profile_index) {
+  for (BrowserVector::const_iterator it = browsers_to_close.begin();
+       it != browsers_to_close.end(); ++it) {
+    if ((*it)->CallBeforeUnloadHandlers(
+            base::Bind(&BrowserList::PostBeforeUnloadHandlers,
+                       browsers_to_close,
+                       on_close_success,
+                       profile_index))) {
+      current_closing_browser_ = *it;
+      return;
+    }
+  }
+
+  on_close_success.Run(profile_index);

[Peter Kasting, 2014/08/28 18:46:12]

It surprises me that we run this before we've actually closed the browsers.  Is
this order important?  If so, make the ordering clear in the header comments. 
Otherwise perhaps we should do this at the end of the function.

[Mike Lerman, 2014/08/29 18:02:21]

Not important. Moved.

+
+  for (BrowserVector::const_iterator it = browsers_to_close.begin();
+       it != browsers_to_close.end(); ++it) {

[Peter Kasting, 2014/08/28 18:46:12]

Nit: {} not necessary

[Mike Lerman, 2014/08/29 18:02:21]

Done.

+    (*it)->window()->Close();
+  }
+}
+
+// static
+void BrowserList::PostBeforeUnloadHandlers(
+    const BrowserVector& browsers_to_close,
+    const base::Callback<void(size_t)>& on_close_success,
+    const size_t profile_index,
+    bool proceed) {
+  // Process only one call if the latter of multiple browsers clicks abort.

[Peter Kasting, 2014/08/28 18:46:12]

It's still unclear to me why this code has this effect, probably because
CallBeforeUnloadHandlers() itself is poorly documented and so I can't actually
understand the control flow among all these different methods and callbacks.

I suggest improving the documentation of that function (and its whole class) as
well as adding some comments in TryToCloseBrowserList() about why we're setting
a variable and the control flow that will lead to us making use of it.

[Mike Lerman, 2014/08/29 18:02:21]

I've improved the documentation in browser_list.h that describes the control
flow among the methods here. I think this adds clarity to the control flow.

The UnloadController.h seems to have some good documentation on both the private
and public methods. 

+  if (!current_closing_browser_)
+    return;
+
+  current_closing_browser_ = NULL;
+
+  if (proceed) {
+    TryToCloseBrowserList(browsers_to_close, on_close_success, profile_index);
+  } else {
+    for (BrowserVector::const_iterator it = browsers_to_close.begin();
+        it != browsers_to_close.end(); ++it) {
+      (*it)->ResetBeforeUnloadHandlers();
+    }
+  }
+}
+
+// static
 void BrowserList::SetLastActive(Browser* browser) {
   content::RecordAction(UserMetricsAction("ActiveBrowserChanged"));
   BrowserList* browser_list = GetInstance(browser->host_desktop_type());
 
   RemoveBrowserFrom(browser, &browser_list->last_active_browsers_);
   browser_list->last_active_browsers_.push_back(browser);
 
   FOR_EACH_OBSERVER(chrome::BrowserListObserver, observers_.Get(),
                     OnBrowserSetLastActive(browser));
 }
@@ skipping 30 matching lines @@
 }
 
 // static
 void BrowserList::RemoveBrowserFrom(Browser* browser,
                                     BrowserVector* browser_list) {
   BrowserVector::iterator remove_browser =
       std::find(browser_list->begin(), browser_list->end(), browser);
   if (remove_browser != browser_list->end())
     browser_list->erase(remove_browser);
 }