[DevTools] Remove dock side knowledge from browser.

After this change, DevToolsWindow waits for frontend to decide
on dock side, and only then shows it either docked or undocked.

This also cleans up DevToolsWindow public API, so clients don't use
test function, and test clients are explicit with their docking expectations.

Note: follow up patch on frontend side is require to switch
from SetDockSide to SetIsDocked call.

BUG=318751

Review URL: https://codereview.chromium.org/108213012

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@245308 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/devtools/devtools_embedder_message_dispatcher.cc

@@ -167,6 +167,9 @@ DevToolsEmbedderMessageDispatcher::DevToolsEmbedderMessageDispatcher(
   RegisterHandler("requestSetDockSide",
       BindToListParser(base::Bind(&Delegate::SetDockSide,
                                   base::Unretained(delegate))));
+  RegisterHandler("setIsDocked",
+      BindToListParser(base::Bind(&Delegate::SetIsDocked,
+                                  base::Unretained(delegate))));
   RegisterHandler("openInNewTab",
       BindToListParser(base::Bind(&Delegate::OpenInNewTab,
                                   base::Unretained(delegate))));

chrome/browser/devtools/devtools_embedder_message_dispatcher.h

@@ -31,7 +31,9 @@ class DevToolsEmbedderMessageDispatcher {
     virtual void SetContentsInsets(
         int top, int left, int bottom, int right) = 0;
     virtual void MoveWindow(int x, int y) = 0;
+    // TODO(dgozman): remove once clients have switched to |SetIsDocked|.
     virtual void SetDockSide(const std::string& side) = 0;
+    virtual void SetIsDocked(bool is_docked) = 0;
     virtual void OpenInNewTab(const std::string& url) = 0;
     virtual void SaveToFile(const std::string& url,
                             const std::string& content,

chrome/browser/devtools/devtools_toggle_action.cc

@@ -62,3 +62,8 @@ DevToolsToggleAction DevToolsToggleAction::Reveal(
   return DevToolsToggleAction(
       new RevealParams(url, line_number, column_number));
 }
+
+// static
+DevToolsToggleAction DevToolsToggleAction::NoOp() {
+  return DevToolsToggleAction(kNoOp);
+}

chrome/browser/devtools/devtools_toggle_action.h

@@ -16,7 +16,8 @@ struct DevToolsToggleAction {
     kShowConsole,
     kInspect,
     kToggle,
-    kReveal
+    kReveal,
+    kNoOp
   };
 
   struct RevealParams {
@@ -41,6 +42,7 @@ struct DevToolsToggleAction {
   static DevToolsToggleAction Reveal(const base::string16& url,
                                      size_t line_number,
                                      size_t column_number);
+  static DevToolsToggleAction NoOp();
 
   Type type() const { return type_; }
   const RevealParams* params() const { return params_.get(); }

chrome/browser/devtools/devtools_window.h

@@ -49,13 +49,6 @@ namespace user_prefs {
 class PrefRegistrySyncable;
 }
 
-enum DevToolsDockSide {
-  DEVTOOLS_DOCK_SIDE_UNDOCKED = 0,
-  DEVTOOLS_DOCK_SIDE_BOTTOM,
-  DEVTOOLS_DOCK_SIDE_RIGHT,
-  DEVTOOLS_DOCK_SIDE_MINIMIZED
-};
-
 class DevToolsWindow : private content::NotificationObserver,
                        private content::WebContentsDelegate,
                        private content::DevToolsFrontendHostDelegate,
@@ -69,31 +62,52 @@ class DevToolsWindow : private content::NotificationObserver,
 
   static std::string GetDevToolsWindowPlacementPrefKey();
   static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
+
   // Return the DevToolsWindow for the given RenderViewHost if one exists,
   // otherwise NULL.
   static DevToolsWindow* GetInstanceForInspectedRenderViewHost(
       content::RenderViewHost* inspected_rvh);
+
+  // Return the DevToolsWindow for the given WebContents if one exists and is
+  // docked, otherwise NULL. This method will return only fully initialized
+  // window ready to be presented in UI.
+  // For immediately-ready-to-use but maybe not yet fully initialized DevTools
+  // use |GetInstanceForInspectedRenderViewHost| instead.
   static DevToolsWindow* GetDockedInstanceForInspectedTab(
       content::WebContents* inspected_tab);
+
   static bool IsDevToolsWindow(content::RenderViewHost* window_rvh);
-  static DevToolsWindow* OpenDevToolsWindowForWorker(
-      Profile* profile,
-      content::DevToolsAgentHost* worker_agent);
-  static DevToolsWindow* CreateDevToolsWindowForWorker(Profile* profile);
+
+  // Open or reveal DevTools window, and perform the specified action.
+  static DevToolsWindow* OpenDevToolsWindow(
+      content::RenderViewHost* inspected_rvh,
+      const DevToolsToggleAction& action);
+
+  // Open or reveal DevTools window, with no special action.
   static DevToolsWindow* OpenDevToolsWindow(
       content::RenderViewHost* inspected_rvh);
+
+  static DevToolsWindow* OpenDevToolsWindowForTest(
+      content::RenderViewHost* inspected_rvh, bool is_docked);
+  static DevToolsWindow* OpenDevToolsWindowForTest(
+      Browser* browser, bool is_docked);
+
+  // Perform specified action for current WebContents inside a |browser|.
+  // This may close currently open DevTools window.
   static DevToolsWindow* ToggleDevToolsWindow(
       Browser* browser,
       const DevToolsToggleAction& action);
-  static void OpenExternalFrontend(Profile* profile,
-                                   const std::string& frontend_uri,
-                                   content::DevToolsAgentHost* agent_host);
 
-  // Exposed for testing, normal clients should not use this method.
-  static DevToolsWindow* ToggleDevToolsWindow(
-      content::RenderViewHost* inspected_rvh,
-      bool force_open,
-      const DevToolsToggleAction& action);
+  // External frontend is always undocked.
+  static void OpenExternalFrontend(
+      Profile* profile,
+      const std::string& frontend_uri,
+      content::DevToolsAgentHost* agent_host);
+
+  // Worker frontend is always undocked.
+  static DevToolsWindow* OpenDevToolsWindowForWorker(
+      Profile* profile,
+      content::DevToolsAgentHost* worker_agent);
 
   static void InspectElement(
       content::RenderViewHost* inspected_rvh, int x, int y);
@@ -105,10 +119,8 @@ class DevToolsWindow : private content::NotificationObserver,
 
   content::WebContents* web_contents() { return web_contents_; }
   Browser* browser() { return browser_; }  // For tests.
-  DevToolsDockSide dock_side() const { return dock_side_; }
 
   content::RenderViewHost* GetRenderViewHost();
-  content::DevToolsClientHost* GetDevToolsClientHostForTest();
 
   // Inspected WebContents is placed over DevTools WebContents in docked mode.
   // The following methods return the insets of inspected WebContents
@@ -119,8 +131,6 @@ class DevToolsWindow : private content::NotificationObserver,
   // the overlaying inspected WebContents size.
   gfx::Size GetMinimumSize() const;
 
-  void Show(const DevToolsToggleAction& action);
-
   // BeforeUnload interception ////////////////////////////////////////////////
 
   // In order to preserve any edits the user may have made in devtools, the
@@ -195,37 +205,53 @@ class DevToolsWindow : private content::NotificationObserver,
   // by user.
   static void OnPageCloseCanceled(content::WebContents* contents);
 
-  void SetDockSideForTest(DevToolsDockSide dock_side);
-
  private:
   friend class DevToolsControllerTest;
+  friend class DevToolsSanityTest;
   friend class BrowserWindowControllerTest;
 
+  // DevTools initialization typically follows this way:
+  // - Toggle/Open: client call;
+  // - Create;
+  // - ScheduleShow: setup window to be functional, but not yet show;
+  // - DocumentOnLoadCompletedInMainFrame: frontend loaded;
+  // - SetIsDocked: frontend decided on docking state;
+  // - OnLoadCompleted: ready to present frontend;
+  // - Show: actually placing frontend WebContents to a Browser or docked place;
+  // - DoAction: perform action passed in Toggle/Open.
+  enum LoadState {
+    kNotLoaded,
+    kOnLoadFired, // Implies SetIsDocked was not yet called.
+    kIsDockedSet, // Implies DocumentOnLoadCompleted was not yet called.
+    kLoadCompleted
+  };
+
   DevToolsWindow(Profile* profile,
                  const GURL& frontend_url,
                  content::RenderViewHost* inspected_rvh,
-                 DevToolsDockSide dock_side);
+                 bool can_dock);
 
   static DevToolsWindow* Create(Profile* profile,
                                 const GURL& frontend_url,
                                 content::RenderViewHost* inspected_rvh,
-                                DevToolsDockSide dock_side,
                                 bool shared_worker_frontend,
                                 bool external_frontend,
                                 bool can_dock);
   static GURL GetDevToolsURL(Profile* profile,
                              const GURL& base_url,
-                             DevToolsDockSide dock_side,
                              bool shared_worker_frontend,
                              bool external_frontend,
                              bool can_dock);
   static DevToolsWindow* FindDevToolsWindow(content::DevToolsAgentHost*);
   static DevToolsWindow* AsDevToolsWindow(content::RenderViewHost*);
-  static DevToolsDockSide GetDockSideFromPrefs(Profile* profile);
-  static std::string SideToString(DevToolsDockSide dock_side);
-  static DevToolsDockSide SideFromString(const std::string& dock_side);
+  static DevToolsWindow* CreateDevToolsWindowForWorker(Profile* profile);
+  static bool IsDockedFromString(const std::string& dock_side);
   static bool FindInspectedBrowserAndTabIndex(
       content::WebContents* inspected_web_contents, Browser**, int* tab);
+  static DevToolsWindow* ToggleDevToolsWindow(
+      content::RenderViewHost* inspected_rvh,
+      bool force_open,
+      const DevToolsToggleAction& action);
 
   // content::NotificationObserver:
   virtual void Observe(int type,
@@ -275,6 +301,7 @@ class DevToolsWindow : private content::NotificationObserver,
       int left, int top, int right, int bottom) OVERRIDE;
   virtual void MoveWindow(int x, int y) OVERRIDE;
   virtual void SetDockSide(const std::string& side) OVERRIDE;
+  virtual void SetIsDocked(bool is_docked) OVERRIDE;
   virtual void OpenInNewTab(const std::string& url) OVERRIDE;
   virtual void SaveToFile(const std::string& url,
                           const std::string& content,
@@ -315,10 +342,11 @@ class DevToolsWindow : private content::NotificationObserver,
 
   void CreateDevToolsBrowser();
   BrowserWindow* GetInspectedBrowserWindow();
-  bool IsInspectedBrowserPopup();
-  void UpdateFrontendDockSide();
-  void ScheduleAction(const DevToolsToggleAction& action);
-  void DoAction();
+  void ScheduleShow(const DevToolsToggleAction& action);
+  void Show(const DevToolsToggleAction& action);
+  void DoAction(const DevToolsToggleAction& action);
+  void LoadCompleted();
+  void SetIsDockedAndShowImmediatelyForTest(bool is_docked);
   void UpdateTheme();
   void AddDevToolsExtensionsToClient();
   void CallClientFunction(const std::string& function_name,
@@ -326,8 +354,6 @@ class DevToolsWindow : private content::NotificationObserver,
                           const base::Value* arg2,
                           const base::Value* arg3);
   void UpdateBrowserToolbar();
-  bool IsDocked();
-  void Restore();
   content::WebContents* GetInspectedWebContents();
   void DocumentOnLoadCompletedInMainFrame();
 
@@ -340,10 +366,11 @@ class DevToolsWindow : private content::NotificationObserver,
   Profile* profile_;
   content::WebContents* web_contents_;
   Browser* browser_;
-  // TODO(dgozman): move dock side knowledge entirely to frontend.
-  DevToolsDockSide dock_side_;
-  bool is_loaded_;
+  bool is_docked_;
+  bool can_dock_;
+  LoadState load_state_;
   DevToolsToggleAction action_on_load_;
+  bool ignore_set_is_docked_for_test_;
   content::NotificationRegistrar registrar_;
   scoped_ptr<content::DevToolsClientHost> frontend_host_;
   scoped_ptr<DevToolsFileHelper> file_helper_;
@@ -354,7 +381,6 @@ class DevToolsWindow : private content::NotificationObserver,
       IndexingJobsMap;
   IndexingJobsMap indexing_jobs_;
   gfx::Insets contents_insets_;
-  DevToolsDockSide dock_side_before_minimized_;
   // True if we're in the process of handling a beforeunload event originating
   // from the inspected webcontents, see InterceptPageBeforeUnload for details.
   bool intercepted_page_beforeunload_;

chrome/browser/extensions/api/app_window/app_window_api.cc

@@ -63,9 +63,8 @@ class DevToolsRestorer : public content::NotificationObserver {
                    content::RenderViewHost* created_view)
       : delayed_create_function_(delayed_create_function) {
     DevToolsWindow* devtools_window =
-        DevToolsWindow::ToggleDevToolsWindow(
+        DevToolsWindow::OpenDevToolsWindow(
             created_view,
-            true /* force_open */,
             DevToolsToggleAction::ShowConsole());
 
     registrar_.Add(

chrome/browser/policy/policy_browsertest.cc

@@ -1368,7 +1368,8 @@ IN_PROC_BROWSER_TEST_F(PolicyTest, DeveloperToolsDisabled) {
   content::WebContents* contents =
       browser()->tab_strip_model()->GetActiveWebContents();
   DevToolsWindow *devtools_window =
-      DevToolsWindow::GetDockedInstanceForInspectedTab(contents);
+      DevToolsWindow::GetInstanceForInspectedRenderViewHost(
+          contents->GetRenderViewHost());
   EXPECT_TRUE(devtools_window);
 
   // Disable devtools via policy.
@@ -1382,10 +1383,12 @@ IN_PROC_BROWSER_TEST_F(PolicyTest, DeveloperToolsDisabled) {
   // wait for devtools close
   close_observer.Wait();
   // The existing devtools window should have closed.
-  EXPECT_FALSE(DevToolsWindow::GetDockedInstanceForInspectedTab(contents));
+  EXPECT_FALSE(DevToolsWindow::GetInstanceForInspectedRenderViewHost(
+      contents->GetRenderViewHost()));
   // And it's not possible to open it again.
   EXPECT_FALSE(chrome::ExecuteCommand(browser(), IDC_DEV_TOOLS));
-  EXPECT_FALSE(DevToolsWindow::GetDockedInstanceForInspectedTab(contents));
+  EXPECT_FALSE(DevToolsWindow::GetInstanceForInspectedRenderViewHost(
+      contents->GetRenderViewHost()));
 }
 
 // TODO(samarth): remove along with rest of NTP4 code.

chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc

@@ -353,8 +353,7 @@ IN_PROC_BROWSER_TEST_F(ChromeRenderProcessHostTest,
   DCHECK(devtools);
 
   // DevTools start in a separate process.
-  DevToolsWindow::ToggleDevToolsWindow(
-      devtools, true, DevToolsToggleAction::Inspect());
+  DevToolsWindow::OpenDevToolsWindow(devtools, DevToolsToggleAction::Inspect());
   host_count++;
   EXPECT_EQ(tab_count, browser()->tab_strip_model()->count());
   EXPECT_EQ(host_count, RenderProcessHostCount());
@@ -401,8 +400,7 @@ IN_PROC_BROWSER_TEST_F(ChromeRenderProcessHostTest,
   DCHECK(devtools);
 
   // DevTools start in a separate process.
-  DevToolsWindow::ToggleDevToolsWindow(
-      devtools, true, DevToolsToggleAction::Inspect());
+  DevToolsWindow::OpenDevToolsWindow(devtools, DevToolsToggleAction::Inspect());
   host_count++;
   EXPECT_EQ(tab_count, browser()->tab_strip_model()->count());
   EXPECT_EQ(host_count, RenderProcessHostCount());

chrome/browser/task_manager/task_manager_browsertest.cc

@@ -513,12 +513,8 @@ IN_PROC_BROWSER_TEST_F(TaskManagerBrowserTest, DISABLED_WebWorkerJSHeapMemory) {
 }
 
 IN_PROC_BROWSER_TEST_F(TaskManagerBrowserTest, NoticeInTabDevToolsWindow) {
-  DevToolsWindow* dev_tools = DevToolsWindow::ToggleDevToolsWindow(
-      model()->GetResourceWebContents(1)->GetRenderViewHost(),
-      true,
-      DevToolsToggleAction::Inspect());
-  // Dock side bottom should be the default.
-  ASSERT_EQ(DEVTOOLS_DOCK_SIDE_BOTTOM, dev_tools->dock_side());
+  DevToolsWindow::OpenDevToolsWindowForTest(
+      model()->GetResourceWebContents(1)->GetRenderViewHost(), true);
   TaskManagerBrowserTestUtil::WaitForWebResourceChange(2);
 }
 
@@ -527,12 +523,9 @@ IN_PROC_BROWSER_TEST_F(TaskManagerBrowserTest, NoticeInTabDevToolsWindow) {
 IN_PROC_BROWSER_TEST_F(TaskManagerNoShowBrowserTest,
                        NoticeInTabDevToolsWindow) {
   // First create the devtools window.
-  DevToolsWindow* dev_tools = DevToolsWindow::ToggleDevToolsWindow(
+  DevToolsWindow::OpenDevToolsWindowForTest(
       browser()->tab_strip_model()->GetActiveWebContents()->GetRenderViewHost(),
-      true,
-      DevToolsToggleAction::Inspect());
-  // Dock side bottom should be the default.
-  ASSERT_EQ(DEVTOOLS_DOCK_SIDE_BOTTOM, dev_tools->dock_side());
+      true);
   // Make sure that the devtools window is loaded before starting the task
   // manager.
   content::RunAllPendingInMessageLoop();

chrome/browser/ui/cocoa/browser_window_controller_browsertest.mm

@@ -378,8 +378,8 @@ IN_PROC_BROWSER_TEST_F(BrowserWindowControllerTest,
                        StatusBubblePositioning) {
   NSPoint origin = [controller() statusBubbleBaseFrame].origin;
 
-  DevToolsWindow* devtools_window = DevToolsWindow::ToggleDevToolsWindow(
-      browser(), DevToolsToggleAction::Show());
+  DevToolsWindow* devtools_window = DevToolsWindow::OpenDevToolsWindowForTest(
+      browser(), true);
   SetDevToolsWindowContentsInsets(devtools_window, 10, 10, 10, 10);
 
   NSPoint originWithDevTools = [controller() statusBubbleBaseFrame].origin;

chrome/browser/ui/cocoa/dev_tools_controller_browsertest.mm

@@ -22,8 +22,7 @@ class DevToolsControllerTest : public InProcessBrowserTest {
   }
 
   virtual void SetUpOnMainThread() OVERRIDE {
-    DevToolsWindow::ToggleDevToolsWindow(browser(),
-                                         DevToolsToggleAction::Show());
+    DevToolsWindow::OpenDevToolsWindowForTest(browser(), true);
   }
 
  private:

chrome/browser/ui/cocoa/view_id_util_browsertest.mm

@@ -10,6 +10,7 @@
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "chrome/browser/bookmarks/bookmark_test_helpers.h"
 #include "chrome/browser/bookmarks/bookmark_utils.h"
+#include "chrome/browser/devtools/devtools_window.h"
 #include "chrome/browser/download/download_shelf.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
@@ -47,9 +48,7 @@ class ViewIDTest : public InProcessBrowserTest {
     chrome::ShowFindBar(browser());
 
     // Make sure docked devtools is created to test VIEW_ID_DEV_TOOLS_DOCKED
-    browser()->profile()->GetPrefs()->SetString(prefs::kDevToolsDockSide,
-                                                "dock_bottom");
-    chrome::ToggleDevToolsWindow(browser(), DevToolsToggleAction::Inspect());
+    DevToolsWindow::OpenDevToolsWindowForTest(browser(), true);
 
     // Make sure download shelf is created to test VIEW_ID_DOWNLOAD_SHELF
     browser()->window()->GetDownloadShelf()->Show();

chrome/browser/ui/panels/panel.cc

@@ -412,16 +412,14 @@ void Panel::ExecuteCommandWithDisposition(int id,
     // DevTools
     case IDC_DEV_TOOLS:
       content::RecordAction(UserMetricsAction("DevTools_ToggleWindow"));
-      DevToolsWindow::ToggleDevToolsWindow(
+      DevToolsWindow::OpenDevToolsWindow(
           GetWebContents()->GetRenderViewHost(),
-          true,
           DevToolsToggleAction::Show());
       break;
     case IDC_DEV_TOOLS_CONSOLE:
       content::RecordAction(UserMetricsAction("DevTools_ToggleConsole"));
-      DevToolsWindow::ToggleDevToolsWindow(
+      DevToolsWindow::OpenDevToolsWindow(
           GetWebContents()->GetRenderViewHost(),
-          true,
           DevToolsToggleAction::ShowConsole());
       break;
 

chrome/browser/ui/views/extensions/extension_popup.cc

@@ -254,8 +254,7 @@ void ExtensionPopup::ShowBubble() {
   host()->host_contents()->GetView()->Focus();
 
   if (inspect_with_devtools_) {
-    DevToolsWindow::ToggleDevToolsWindow(host()->render_view_host(),
-        true,
+    DevToolsWindow::OpenDevToolsWindow(host()->render_view_host(),
         DevToolsToggleAction::ShowConsole());
   }
 }

chrome/browser/ui/webui/extensions/extension_error_handler.cc

@@ -197,13 +197,6 @@ void ExtensionErrorHandler::HandleOpenDevTools(const base::ListValue* args) {
   if (!rvh)
     return;
 
-  // Check if we already have an inspector for the given RenderViewHost. If not,
-  // create one.
-  DevToolsWindow* window =
-      DevToolsWindow::GetInstanceForInspectedRenderViewHost(rvh);
-  if (!window)
-    window = DevToolsWindow::OpenDevToolsWindow(rvh);
-
   // If we include a url, we should inspect it specifically (and not just the
   // render view).
   base::string16 url;
@@ -216,8 +209,10 @@ void ExtensionErrorHandler::HandleOpenDevTools(const base::ListValue* args) {
 
     // Line/column numbers are reported in display-friendly 1-based numbers,
     // but are inspected in zero-based numbers.
-    window->Show(
+    DevToolsWindow::OpenDevToolsWindow(rvh,
         DevToolsToggleAction::Reveal(url, line_number - 1, column_number - 1));
+  } else {
+    DevToolsWindow::OpenDevToolsWindow(rvh);
   }
 
   // Once we open the inspector, we focus on the appropriate tab...

chrome/common/pref_names.cc

@@ -1793,9 +1793,6 @@ const char kDevToolsDisabled[] = "devtools.disabled";
 const char kDevToolsDiscoverUsbDevicesEnabled[] =
     "devtools.discover_usb_devices";
 
-// A string specifying the dock location (either 'bottom' or 'right').
-const char kDevToolsDockSide[] = "devtools.dock_side";
-
 // Maps of files edited locally using DevTools.
 const char kDevToolsEditedFiles[] = "devtools.edited_files";
 

chrome/common/pref_names.h

@@ -591,7 +591,6 @@ extern const char kNtpWebStoreEnabled[];
 extern const char kDevToolsAdbKey[];
 extern const char kDevToolsDisabled[];
 extern const char kDevToolsDiscoverUsbDevicesEnabled[];
-extern const char kDevToolsDockSide[];
 extern const char kDevToolsEditedFiles[];
 extern const char kDevToolsFileSystemPaths[];
 extern const char kDevToolsOpenDocked[];