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..7f6b3826d6f455c456696a8b7dd5edb017c0cfeb 100644
--- a/chrome/browser/devtools/devtools_window.h
+++ b/chrome/browser/devtools/devtools_window.h
@@ -130,6 +130,38 @@ class DevToolsWindow : private content::NotificationObserver,
 
   void Show(const DevToolsToggleAction& action);
 
+  // DevTools window in undocked state doesn't receive beforeunload callback
+  // from its frontend webcontents as a browser is set as its delegate.
+  // This is a helper method to route callback to DevTools window. Returns
+  // true if routing was successful.
+  static bool HandleBeforeUnload(content::WebContents* contents,

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

What's missing from the comments in these functions is information about the
flow of a beforeunload event, how is it passed through these functions? 

I think that most of the comment for InterceptPageBeforeUnload() should be moved
up to a comment that prefaces these functions and explains things in more
detail.

Variables also need comments.

[lushnikov, 2013/11/11 15:22:54]

Done. 
Arguments are exactly the same as they are in |BeforeUnloadFired| callback; I
updated the comment so that (hopefully) it will be more clear.

+                                 bool proceed,
+                                 bool* proceed_to_fire_unload);
+
+  // In order to preserve any edits the user may have made in devtools, the
+  // beforeunload event of 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 it's beforeunload handler. This function
+  // returns true if devtools window took responsobility for firing
+  // beforeunload events for inspected page.

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

I find this and the above function name a bit generic in that it's not clear
what part they play in the flow.  I'd vote for more descriptive names (sorry, no
suggestions at the moment).

[lushnikov, 2013/11/11 15:22:54]

Do the additional comments bring some light on motivation behind this naming?

+  static bool InterceptPageBeforeUnload(content::WebContents* contents);
+
+  // Returns true if DevTools browser window could be closed.
+  static bool ShouldCloseDevToolsBrowser(Browser* browser);
+
+  // Returns true if DevTools window would like to hook beforeunload event
+  // of this |contents|.
+  static bool NeedToFireBeforeUnload(content::WebContents* contents);

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

Again, method name doesn't convey what's going on here.

NeedsToInterceptBeforeUnload()? HasBeforeUnloadHanlder()?

[lushnikov, 2013/11/11 15:22:54]

I pick "NeedsToInterceptBeforeUnload" :)

+
+  // Notify DevTools window that closing of |contents| was cancelled
+  // by used.

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

used -> user

[lushnikov, 2013/11/11 15:22:54]

Done.

+  static void OnPageCloseCanceled(content::WebContents* contents);
+
+  void SetDockSideForTest(DevToolsDockSide dock_side);
+
  private:
   friend class DevToolsControllerTest;
 
@@ -281,6 +313,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_;