DevTools: Do not close devtools if there are dirty files in workspace

chrome/browser/ui/fast_unload_controller.cc

Index: chrome/browser/ui/fast_unload_controller.cc
diff --git a/chrome/browser/ui/fast_unload_controller.cc b/chrome/browser/ui/fast_unload_controller.cc
index 70b3a41cf97fc0a3b03489744adfc7c2fe3e3ec7..e594e55f6bff643d22efe9281e9b1dee72b60cab 100644
--- a/chrome/browser/ui/fast_unload_controller.cc
+++ b/chrome/browser/ui/fast_unload_controller.cc
@@ -71,8 +71,20 @@ bool FastUnloadController::CanCloseContents(content::WebContents* contents) {
 }
 
 // static
+bool FastUnloadController::CanFastShutdownWebContents(
+    content::WebContents* contents) {

[jeremy, 2013/11/07 14:18:34]

Can you please provide a comment on what fastshutdown means exactly and point to
it from the various delegates.

[lushnikov, 2013/11/07 17:18:10]

Do you think this one is needed? Looks like it could be easily figured out from
the code - there's only a single call of this method, which brings us right to
the fast shutdown code in content layer.

[jeremy, 2013/11/10 12:50:29]

There are way too many moving parts here and for someone unfamiliar with the
code this is very hard to follow.   Having a descriptive method name helps
reduce the cognitive load when reading through the code.

I'm sure we can find a generic term which has nothing to do with DevTools but
conveys what's going on, how about something like BeforeUnloadInterceptNeeded()
or somesunch?

+  return DevToolsWindow::GetInstanceForInspectedRenderViewHost(
+      contents->GetRenderViewHost()) == NULL;
+}
+
+// static
 bool FastUnloadController::RunUnloadEventsHelper(
     content::WebContents* contents) {
+  // If there's a devtools window attached to the web contents,
+  // then we would like to run its beforeunload handlers first.

[jeremy, 2013/11/07 14:18:34]

Please point to the comment that explains why.

[lushnikov, 2013/11/07 17:18:10]

Done.

+  if (DevToolsWindow::InterceptPageBeforeUnload(contents)) {

[jeremy, 2013/11/10 12:50:29]

2 things are unclear from this callsite:
* Does InterceptPageBeforeUnload() actually display the dialog?
* Is this a sync or an async operation?

Suggest changing the function name and adding a comment to make this clear.

+    return true;
+  }
   // If the WebContents is not connected yet, then there's no unload
   // handler we can fire even if the WebContents has an unload listener.
   // One case where we hit this is in a tab that has an infinite loop
@@ -90,6 +102,9 @@ bool FastUnloadController::RunUnloadEventsHelper(
 
 bool FastUnloadController::BeforeUnloadFired(content::WebContents* contents,
                                              bool proceed) {
+  if (!proceed)
+    DevToolsWindow::PageClosingCanceled(contents);
+
   if (!is_attempting_to_close_browser_) {
     if (!proceed) {
       contents->SetClosedByUserGesture(false);
@@ -126,6 +141,11 @@ bool FastUnloadController::ShouldCloseWindow() {
   if (HasCompletedUnloadProcessing())
     return true;
 
+  if (browser_->is_devtools() &&
+      DevToolsWindow::ShouldCloseDevToolsBrowser(browser_)) {
+    return true;
+  }
+
   // The behavior followed here varies based on the current phase of the
   // operation and whether a batched shutdown is in progress.
   //
@@ -153,7 +173,9 @@ bool FastUnloadController::ShouldCloseWindow() {
 
 bool FastUnloadController::CallBeforeUnloadHandlers(
     const base::Callback<void(bool)>& on_close_confirmed) {
-  if (!TabsNeedBeforeUnloadFired())
+  // DevTools browser will get its beforeunload events triggered on
+  // inspected tab closing

[jeremy, 2013/11/07 14:18:34]

will get -> gets

[lushnikov, 2013/11/07 17:18:10]

Done.

+  if (browser_->is_devtools() || !TabsNeedBeforeUnloadFired())
     return false;
 
   on_close_confirmed_ = on_close_confirmed;
@@ -182,7 +204,8 @@ bool FastUnloadController::TabsNeedBeforeUnloadFired() {
     if (!ContainsKey(tabs_needing_unload_, contents) &&
         !ContainsKey(tabs_needing_unload_ack_, contents) &&
         tab_needing_before_unload_ack_ != contents &&
-        contents->NeedToFireBeforeUnload())
+        (contents->NeedToFireBeforeUnload() ||
+        DevToolsWindow::NeedToFireBeforeUnload(contents)))

[jeremy, 2013/11/07 14:18:34]

This expression is already kind of crazy - can we split this out into a boolean
and then just check that boolean inside the if?

[lushnikov, 2013/11/07 17:18:10]

Done.

       tabs_needing_before_unload_.insert(contents);
   }
   return !tabs_needing_before_unload_.empty();
@@ -314,7 +337,8 @@ void FastUnloadController::ProcessPendingTabs() {
       CoreTabHelper* core_tab_helper = CoreTabHelper::FromWebContents(contents);
       core_tab_helper->OnCloseStarted();
 
-      contents->GetRenderViewHost()->FirePageBeforeUnload(false);
+      if (!DevToolsWindow::InterceptPageBeforeUnload(contents))

[jeremy, 2013/11/07 14:18:34]

I don't really think this method name conveys what is happening, but I don't
have any ideas for a better name, sorry :(

This needs a comment though, something like:

First let Devtools put up a warning dialog if needed, then give the renderview a
chance to put up a dialog.  See ... for details.

[lushnikov, 2013/11/07 17:18:10]

Done.

+        contents->GetRenderViewHost()->FirePageBeforeUnload(false);
     } else {
       ProcessPendingTabs();
     }
@@ -325,7 +349,6 @@ void FastUnloadController::ProcessPendingTabs() {
     on_close_confirmed_.Run(true);
     return;
   }
-
   // Process all the unload handlers. (The beforeunload handlers have finished.)
   if (!tabs_needing_unload_.empty()) {
     browser_->OnWindowClosing();
@@ -383,10 +406,10 @@ void FastUnloadController::CancelWindowClose() {
   DCHECK(is_attempting_to_close_browser_);
   tabs_needing_before_unload_.clear();
   if (tab_needing_before_unload_ack_ != NULL) {
-
     CoreTabHelper* core_tab_helper =
         CoreTabHelper::FromWebContents(tab_needing_before_unload_ack_);
     core_tab_helper->OnCloseCanceled();
+    DevToolsWindow::PageClosingCanceled(tab_needing_before_unload_ack_);

[jeremy, 2013/11/07 14:18:34]

*OnPageCloseCanceled() ?

[lushnikov, 2013/11/07 17:18:10]

Done.

     tab_needing_before_unload_ack_ = NULL;
   }
   for (WebContentsSet::iterator it = tabs_needing_unload_.begin();
@@ -395,6 +418,7 @@ void FastUnloadController::CancelWindowClose() {
 
     CoreTabHelper* core_tab_helper = CoreTabHelper::FromWebContents(contents);
     core_tab_helper->OnCloseCanceled();
+    DevToolsWindow::PageClosingCanceled(contents);
   }
   tabs_needing_unload_.clear();