Update ownership semantics for DevtoolWindow and the toolbar WebContents.

This CL is a refactor with no intended behavior change.

Bug: 832879
Change-Id: I5d2d04d37f03f8ccbaef28695bc29a6b24b2e089
Reviewed-on: https://chromium-review.googlesource.com/1040592
Commit-Queue: Erik Chen <erikchen@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556210}

chrome/browser/devtools/devtools_window.cc

@@ -418,8 +418,7 @@ DevToolsWindow::~DevToolsWindow() {
   UpdateBrowserWindow();
   UpdateBrowserToolbar();
 
-  if (toolbox_web_contents_)
-    delete toolbox_web_contents_;
+  owned_toolbox_web_contents_.reset();
 
   DevToolsWindows* instances = g_devtools_window_instances.Pointer();
   DevToolsWindows::iterator it(
@@ -1144,9 +1143,7 @@ void DevToolsWindow::AddNewContents(WebContents* source,
                                     bool user_gesture,
                                     bool* was_blocked) {
   if (new_contents.get() == toolbox_web_contents_) {
-    // TODO(erikchen): Fix ownership semantics for WebContents.
-    // https://crbug.com/832879.
-    new_contents.release();
+    owned_toolbox_web_contents_ = std::move(new_contents);
 
     toolbox_web_contents_->SetDelegate(
         new DevToolsToolboxDelegate(toolbox_web_contents_,
@@ -1178,8 +1175,10 @@ void DevToolsWindow::WebContentsCreated(WebContents* source_contents,
   if (target_url.SchemeIs(content::kChromeDevToolsScheme) &&
       target_url.path().rfind("toolbox.html") != std::string::npos) {
     CHECK(can_dock_);
-    if (toolbox_web_contents_)
-      delete toolbox_web_contents_;
+
+    // Ownership will be passed in DevToolsWindow::AddNewContents.
+    if (owned_toolbox_web_contents_)
+      owned_toolbox_web_contents_.reset();
     toolbox_web_contents_ = new_contents;
 
     // Tag the DevTools toolbox WebContents with its TaskManager specific

chrome/browser/devtools/devtools_window.h

@@ -365,13 +365,22 @@ class DevToolsWindow : public DevToolsUIBindings::Delegate,
   FrontendType frontend_type_;
   Profile* profile_;
   content::WebContents* main_web_contents_;
+
+  // DevToolsWindow is informed of the creation of the |toolbox_web_contents_|
+  // in WebContentsCreated right before ownership is passed to to DevToolsWindow
+  // in AddNewContents(). The former call has information not available in the
+  // latter, so it's easiest to record a raw pointer first in
+  // |toolbox_web_contents_|, and then update ownership immediately afterwards.
+  // TODO(erikchen): If we updated AddNewContents() to also pass back the
+  // target url, then we wouldn't need to listen to WebContentsCreated at all.
   content::WebContents* toolbox_web_contents_;
+  std::unique_ptr<content::WebContents> owned_toolbox_web_contents_;
+
   DevToolsUIBindings* bindings_;
   Browser* browser_;
 
   // When DevToolsWindow is docked, it owns main_web_contents_. When it isn't
   // docked, the tab strip model owns the main_web_contents_.
-  // TODO(erikchen): This needs more careful thinking about.
   bool is_docked_;
   std::unique_ptr<content::WebContents> owned_main_web_contents_;