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

chrome/browser/devtools/devtools_window.h

Index: chrome/browser/devtools/devtools_window.h
diff --git a/chrome/browser/devtools/devtools_window.h b/chrome/browser/devtools/devtools_window.h
index 32c3d71b26bd23872dd77c971c13ab0efffab4ad..a2ab631e88f4ac7910587a05d309dd7ec1fed348 100644
--- a/chrome/browser/devtools/devtools_window.h
+++ b/chrome/browser/devtools/devtools_window.h
@@ -130,6 +130,66 @@ class DevToolsWindow : private content::NotificationObserver,
 
   void Show(const DevToolsToggleAction& action);
 
+  // BeforeUnload handling ////////////////////////////////////////////////////

[jeremy, 2013/11/12 10:52:46]

nit: beforeunload interception ?

[lushnikov, 2013/11/12 13:37:37]

Done.

+
+  // In order to preserve any edits the user may have made in devtools, the
+  // beforeunload event of the inspected page is hooked - devtools gets the
+  // first shot at handling beforeunload and presents a dialog to the user. If
+  // the user accepts the dialog then the script is given a chance to handle
+  // it. This way 2 dialogs may be displayed: one from the devtools asking the
+  // user to confirm that they're ok with their edits going away and another
+  // from the webpage as the result of its beforeunload handler.
+  // The following set of methods handle beforeunload event flow through
+  // DevTools window. When the |contents| with DevTools opened on them are

[jeremy, 2013/11/12 10:52:46]

nit: "the devtools", also capitalization of devtools should be the smae
throughout comments - again, minor nit.

[lushnikov, 2013/11/12 13:37:37]

Done.

+  // getting closed, the following sequence of calls takes place:
+  // 1. Instead of firing beforeunload event on |contents|,
+  //    method |DevToolsWindow::InterceptPageBeforeUnload| gets called.
+  // 2. DevTools fire beforeunload event for its own frontend and wait for ack,
+  //    which by the means of |HandleBeforeUnload| helper method always hits

[jeremy, 2013/11/12 10:52:46]

I'm not clear on the flow here - InterceptPageBeforeUnload() fires the
beforeunload event for the Devtools window itself, the synthesized beforeunload
event for DevTools window is then handled by HandleBeforeUnload() ?

I'm also not clear on everything after the comma.

[lushnikov, 2013/11/12 13:37:37]

Rewrote the comment with more details; should look much better now.

+  //    |DevToolsWindow::BeforeUnloadFired|. The ack has user decision
+  //    whether he wants to close DevTools or not.
+  // 3a. If user decides not to close DevTools, callback method
+  //     |WebContentsDelegate::BeforeUnloadFired| gets called on |contents|
+  //     delegate with the |proceed| argument set to false. This looks as if
+  //     |contents| got its beforeunload event fired and user rejected page
+  //     close.

[jeremy, 2013/11/12 10:52:46]

This is completely invisible from the web page's perspective, right?  the web
page itself never sees the beforeunload event in this scenario?

[lushnikov, 2013/11/12 13:37:37]

That's right.

+  // 3b. If user decides to close DevTools window, DevTools fire beforeunload
+  //     event on |contents|.

[jeremy, 2013/11/12 10:52:46]

add - "and everything proceeds as it normally would without the Devtools
interception".

[lushnikov, 2013/11/12 13:37:37]

Done.

+  // 4. Whenever delegate of the |contents| gets |BeforeUnloadFired| callback

[jeremy, 2013/11/12 10:52:46]

"If the user cancels the dialog put up by the WebContents, then |contents|'s
|BeforeUnloadFired| callback is called with the proceed argument set to false,
this causes |DevToolsWindow::OnPageCloseCancelled| to be called."

[lushnikov, 2013/11/12 13:37:37]

Slightly corrected detail; Done.

+  //    with |proceed| argument set to false, method
+  //    |DevToolsWindow::OnPageCloseCancelled| gets called.
+
+  // DevTools window in undocked state is not set as a delegate of
+  // its frontend. Instead, an instance of browser does, and thus

[jeremy, 2013/11/12 10:52:46]

*"does" -> "is set as the delegate"

[lushnikov, 2013/11/12 13:37:37]

Done.

+  // beforeunload event callback from DevTools frontend is not delivered to
+  // the instance of DevTools window, which is solely responsible for managing
+  // custom beforeunload event flow.
+  // This is a helper method to route callback from
+  // |Browser::BeforeUnloadFired| back to |DevToolsWindow::BeforeUnloadFired|.
+  // Returns true if routing was successful.

[jeremy, 2013/11/12 10:52:46]

Comment needs to document parameters and return value.

[lushnikov, 2013/11/12 13:37:37]

Done.

+  static bool HandleBeforeUnload(content::WebContents* contents,
+                                 bool proceed,
+                                 bool* proceed_to_fire_unload);
+
+  // This function returns true if there is a DevTools window inspecting
+  // given |contents| which would like to manage beforeunload event
+  // flow itself. In this case clients should not fire beforeunload

[jeremy, 2013/11/12 10:52:46]

"manage beforeunload event flow itself" -> "intercept beforeunload events as
described above".

[lushnikov, 2013/11/12 13:37:37]

Done.

+  // event on |contents| themselfes as DevTools window will take care of it.
+  static bool InterceptPageBeforeUnload(content::WebContents* contents);
+
+  // Returns true if DevTools browser window could be closed.

[jeremy, 2013/11/12 10:52:46]

I assume this function returns true if there are no pending user edits and the
devtools window can be closed immediately? Can you rewrite the comment to that
effect?

[lushnikov, 2013/11/12 13:37:37]

I renamed this method and put a more descriptive comment to avoid the confusion,
thanks.

+  static bool ShouldCloseDevToolsBrowser(Browser* browser);
+
+  // Returns true if DevTools window would like to hook beforeunload event
+  // of this |contents|.
+  static bool NeedsToInterceptBeforeUnload(content::WebContents* contents);
+
+  // Notify DevTools window that closing of |contents| was cancelled
+  // by user.
+  static void OnPageCloseCanceled(content::WebContents* contents);
+
+  void SetDockSideForTest(DevToolsDockSide dock_side);

[jeremy, 2013/11/12 10:52:46]

Thank you for writing up these comments, they make things much much clearer!!!

+
  private:
   friend class DevToolsControllerTest;
 
@@ -281,6 +341,7 @@ class DevToolsWindow : private content::NotificationObserver,
   int width_;
   int height_;
   DevToolsDockSide dock_side_before_minimized_;
+  bool inspected_page_is_closing_;
 
   scoped_ptr<DevToolsEmbedderMessageDispatcher> embedder_message_dispatcher_;
   base::WeakPtrFactory<DevToolsWindow> weak_factory_;