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

chrome/browser/devtools/devtools_window.cc

Index: chrome/browser/devtools/devtools_window.cc
diff --git a/chrome/browser/devtools/devtools_window.cc b/chrome/browser/devtools/devtools_window.cc
index f8e1b6a769f5171fb8211b2c4d185f1b12cc4868..fd2b30b454c87fa56f859d358cecac9165df679c 100644
--- a/chrome/browser/devtools/devtools_window.cc
+++ b/chrome/browser/devtools/devtools_window.cc
@@ -298,7 +298,7 @@ void DevToolsWindow::RegisterProfilePrefs(
 }
 
 // static
-DevToolsWindow* DevToolsWindow::GetDockedInstanceForInspectedTab(
+DevToolsWindow* DevToolsWindow::GetInstanceForInspectedTab(
     content::WebContents* inspected_web_contents) {
   if (!inspected_web_contents ||
       !DevToolsAgentHost::HasFor(inspected_web_contents->GetRenderViewHost()))
@@ -306,6 +306,13 @@ DevToolsWindow* DevToolsWindow::GetDockedInstanceForInspectedTab(
   scoped_refptr<DevToolsAgentHost> agent(DevToolsAgentHost::GetOrCreateFor(
       inspected_web_contents->GetRenderViewHost()));
   DevToolsWindow* window = FindDevToolsWindow(agent.get());
+  return window ? window : NULL;

[vsevik, 2013/09/11 11:51:13]

return window

[lushnikov, 2013/09/11 14:03:09]

Done.

+}
+
+// static
+DevToolsWindow* DevToolsWindow::GetDockedInstanceForInspectedTab(
+    content::WebContents* inspected_web_contents) {
+  DevToolsWindow* window = GetInstanceForInspectedTab(inspected_web_contents);
   return (window && window->IsDocked()) ? window : NULL;
 }
 
@@ -539,6 +546,29 @@ void DevToolsWindow::Show(DevToolsToggleAction action) {
   ScheduleAction(action);
 }
 
+// static
+void DevToolsWindow::HandleBeforeUnload(content::WebContents* frontend_contents,
+    bool proceed, bool* proceed_to_fire_unload) {
+  DevToolsWindow* window = AsDevToolsWindow(
+      frontend_contents->GetRenderViewHost());
+  if (window)
+    window->BeforeUnloadFired(frontend_contents, proceed,
+        proceed_to_fire_unload);
+}
+
+bool DevToolsWindow::InspectedPageWillClose() {
+  if (inspected_page_is_closing_)
+    return false;
+  inspected_page_is_closing_ = true;
+  closing_target_ = DEVTOOLS_CLOSING_TARGET_INSPECTED_PAGE;
+  web_contents_->GetRenderViewHost()->FirePageBeforeUnload(false);
+  return true;
+}
+
+void DevToolsWindow::InspectedPageCancelClose() {
+  inspected_page_is_closing_ = false;
+}
+
 DevToolsWindow::DevToolsWindow(Profile* profile,
                                const GURL& url,
                                content::RenderViewHost* inspected_rvh,
@@ -551,7 +581,9 @@ DevToolsWindow::DevToolsWindow(Profile* profile,
       weak_factory_(this),
       width_(-1),
       height_(-1),
-      dock_side_before_minimized_(dock_side) {
+      dock_side_before_minimized_(dock_side),
+      closing_target_(DEVTOOLS_CLOSING_TARGET_NONE),
+      inspected_page_is_closing_(false) {
   web_contents_ =
       content::WebContents::Create(content::WebContents::CreateParams(profile));
   frontend_contents_observer_.reset(
@@ -779,6 +811,37 @@ void DevToolsWindow::AddNewContents(content::WebContents* source,
 }
 
 void DevToolsWindow::CloseContents(content::WebContents* source) {
+  // Update dev tools to reflect removed dev tools window.
+  BrowserWindow* inspected_window = GetInspectedBrowserWindow();
+  if (inspected_window)
+    inspected_window->UpdateDevTools();
+  // In case of docked web_contents_, we own it so delete here.
+  delete web_contents_;
+
+  delete this;
+}
+
+void DevToolsWindow::BeforeUnloadFired(content::WebContents* tab,
+                                       bool proceed,
+                                       bool* proceed_to_fire_unload) {
+  CHECK(closing_target_ != DEVTOOLS_CLOSING_TARGET_NONE);

[vsevik, 2013/09/11 11:51:13]

According to
http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-
you should not use CHECK here and below. Please use DCHECK.

[lushnikov, 2013/09/11 14:03:09]

Done.

+  if (closing_target_ == DEVTOOLS_CLOSING_TARGET_DEVTOOLS) {
+    if (proceed) {
+      content::DevToolsManager::GetInstance()->ClientHostClosing(
+          frontend_host_.get());
+    }
+    *proceed_to_fire_unload = proceed;
+  } else {
+    content::WebContents* inspected_web_contents = GetInspectedWebContents();
+    if (proceed) {
+      inspected_web_contents->GetRenderViewHost()->FirePageBeforeUnload(false);
+    } else {
+      bool tmp;
+      inspected_web_contents->GetDelegate()->BeforeUnloadFired(
+          inspected_web_contents, false, &tmp);
+      CHECK(!tmp);
+    }
+  }
 }
 
 bool DevToolsWindow::PreHandleKeyboardEvent(
@@ -846,11 +909,19 @@ void DevToolsWindow::ActivateWindow() {
     browser_->window()->Activate();
 }
 
+void DevToolsWindow::ActivateContents(content::WebContents* contents) {

[vsevik, 2013/09/11 11:51:13]

I am not sure about this method, but why is it so different from the one above?

+  if (IsDocked()) {
+    content::WebContents* inspected_tab = this->GetInspectedWebContents();
+    inspected_tab->GetDelegate()->ActivateContents(inspected_tab);
+  } else {
+    browser_->window()->Activate();
+  }
+}
+
 void DevToolsWindow::CloseWindow() {
   DCHECK(IsDocked());
-  content::DevToolsManager::GetInstance()->ClientHostClosing(
-      frontend_host_.get());
-  Hide();
+  closing_target_ = DEVTOOLS_CLOSING_TARGET_DEVTOOLS;

[vsevik, 2013/09/11 11:51:13]

Looks like this field is redundant.

[lushnikov, 2013/09/11 14:03:09]

Done.

+  web_contents_->GetRenderViewHost()->FirePageBeforeUnload(false);
 }
 
 void DevToolsWindow::MoveWindow(int x, int y) {
@@ -1192,23 +1263,7 @@ void DevToolsWindow::UpdateFrontendDockSide() {
 }
 
 void DevToolsWindow::Hide() {
-  if (IsDocked()) {
-    // Update dev tools to reflect removed dev tools window.
-    BrowserWindow* inspected_window = GetInspectedBrowserWindow();
-    if (inspected_window)
-      inspected_window->UpdateDevTools();
-    // In case of docked web_contents_, we own it so delete here.
-    delete web_contents_;
-
-    delete this;
-  } else {
-    // First, initiate self-destruct to free all the registrars.
-    // Then close all tabs. Browser will take care of deleting web_contents_
-    // for us.
-    Browser* browser = browser_;
-    delete this;
-    browser->tab_strip_model()->CloseAllTabs();
-  }
+  web_contents_->GetRenderViewHost()->ClosePage();

[vsevik, 2013/09/11 11:51:13]

IS this correct in case of undocked devtools?
why don't you delete this?
why don't we inline this method now that there is only one usage?
Please check beforeunload does not happen here.

[lushnikov, 2013/09/11 14:03:09]

The deletion of DevToolsWindow will happen in ::Observe method

 }
 
 void DevToolsWindow::ScheduleAction(DevToolsToggleAction action) {