Nicely handle OnBeforeUnloads with guest and lock mode.

chrome/browser/ui/browser_list.cc

Index: chrome/browser/ui/browser_list.cc
diff --git a/chrome/browser/ui/browser_list.cc b/chrome/browser/ui/browser_list.cc
index 000c4c6210db311e0600ad9cac30d7a454e833c8..fd43814ec2b62d0160b11def87f5ef00bc790544 100644
--- a/chrome/browser/ui/browser_list.cc
+++ b/chrome/browser/ui/browser_list.cc
@@ -12,6 +12,8 @@
 #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"
@@ -32,6 +34,10 @@ base::LazyInstance<ObserverList<chrome::BrowserListObserver> >::Leaky
 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:
 
@@ -116,6 +122,7 @@ 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()) {
@@ -130,6 +137,67 @@ void BrowserList::CloseAllBrowsersWithProfile(Profile* profile) {
 }
 
 // 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());