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

chrome/browser/devtools/devtools_window.h

 // 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.
 
 #ifndef CHROME_BROWSER_DEVTOOLS_DEVTOOLS_WINDOW_H_
 #define CHROME_BROWSER_DEVTOOLS_DEVTOOLS_WINDOW_H_
 
 #include <string>
 #include <vector>
 
@@ skipping 112 matching lines @@
   int GetHeight(int container_height);
 
   // Stores preferred devtools window width for this instance.
   void SetWidth(int width);
 
   // Stores preferred devtools window height for this instance.
   void SetHeight(int height);
 
   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;
 
   DevToolsWindow(Profile* profile,
                  const GURL& frontend_url,
                  content::RenderViewHost* inspected_rvh,
                  DevToolsDockSide dock_side);
 
   static DevToolsWindow* Create(Profile* profile,
                                 const GURL& frontend_url,
@@ skipping 131 matching lines @@
   scoped_ptr<DevToolsFileHelper> file_helper_;
   scoped_refptr<DevToolsFileSystemIndexer> file_system_indexer_;
   typedef std::map<
       int,
       scoped_refptr<DevToolsFileSystemIndexer::FileSystemIndexingJob> >
       IndexingJobsMap;
   IndexingJobsMap indexing_jobs_;
   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_;
   DISALLOW_COPY_AND_ASSIGN(DevToolsWindow);
 };
 
 #endif  // CHROME_BROWSER_DEVTOOLS_DEVTOOLS_WINDOW_H_