DevTools: “Dock to right” broken after turning a tab into a window of its own.

chrome/browser/ui/views/frame/browser_view.cc

 // 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.
 
 #include "chrome/browser/ui/views/frame/browser_view.h"
 
 #include <algorithm>
 
 #include "base/auto_reset.h"
 #include "base/command_line.h"
@@ skipping 143 matching lines @@
 using views::ColumnSet;
 using views::GridLayout;
 
 namespace {
 // The height of the status bubble.
 const int kStatusBubbleHeight = 20;
 // The name of a key to store on the window handle so that other code can
 // locate this object using just the handle.
 const char* const kBrowserViewKey = "__BROWSER_VIEW__";
 
-// Minimal height of devtools pane or content pane when devtools are docked
-// to the browser window.
-const int kMinDevToolsHeight = 50;
-const int kMinDevToolsWidth = 150;
-const int kMinContentsSize = 50;
-
 // The number of milliseconds between loading animation frames.
 const int kLoadingAnimationFrameTimeMs = 30;
 // The amount of space we expect the window border to take up.
 const int kWindowBorderWidth = 5;
 
 // How round the 'new tab' style bookmarks bar is.
 const int kNewtabBarRoundness = 5;
 
 }  // namespace
 
@@ skipping 165 matching lines @@
       active_bookmark_bar_(NULL),
       tabstrip_(NULL),
       toolbar_(NULL),
       window_switcher_button_(NULL),
       infobar_container_(NULL),
       contents_container_(NULL),
       devtools_container_(NULL),
       contents_(NULL),
       contents_split_(NULL),
       devtools_dock_side_(DEVTOOLS_DOCK_SIDE_BOTTOM),
+      devtools_window_(NULL),
       initialized_(false),
       ignore_layout_(true),
 #if defined(OS_WIN) && !defined(USE_AURA)
       hung_window_detector_(&hung_plugin_action_),
       ticker_(0),
 #endif
       force_location_bar_focus_(false),
       ALLOW_THIS_IN_INITIALIZER_LIST(color_change_listener_(this)),
       ALLOW_THIS_IN_INITIALIZER_LIST(activate_modal_dialog_factory_(this)) {
   browser_->tab_strip_model()->AddObserver(this);
@@ skipping 315 matching lines @@
   if (MaybeShowBookmarkBar(chrome::GetActiveTabContents(browser_.get())))
     Layout();
 }
 
 void BrowserView::UpdateDevTools() {
   UpdateDevToolsForContents(GetActiveTabContents());
   Layout();
 }
 
 
-void BrowserView::SetDevToolsDockSide(DevToolsDockSide side) {
-  if (devtools_dock_side_ == side)
-    return;
-
-  if (devtools_container_->visible()) {
-    HideDevToolsContainer();
-    ShowDevToolsContainer(side);
-  } else {
-    devtools_dock_side_ = side;
-  }
-}
-
 void BrowserView::UpdateLoadingAnimations(bool should_animate) {
   if (should_animate) {
     if (!loading_animation_timer_.IsRunning()) {
       // Loads are happening, and the timer isn't running, so start it.
       last_animation_time_ = base::TimeTicks::Now();
       loading_animation_timer_.Start(FROM_HERE,
           TimeDelta::FromMilliseconds(kLoadingAnimationFrameTimeMs), this,
           &BrowserView::LoadingAnimationCallback);
     }
   } else {
@@ skipping 1329 matching lines @@
 }
 
 bool BrowserView::MaybeShowInfoBar(TabContents* contents) {
   // TODO(beng): Remove this function once the interface between
   //             InfoBarContainer, DownloadShelfView and WebContents and this
   //             view is sorted out.
   return true;
 }
 
 void BrowserView::UpdateDevToolsForContents(TabContents* tab_contents) {
-  DevToolsWindow* devtools_window = tab_contents ?
+  DevToolsWindow* new_devtools_window = tab_contents ?
       DevToolsWindow::GetDockedInstanceForInspectedTab(
           tab_contents->web_contents()) : NULL;
-  TabContents* devtools_tab_contents =
-      devtools_window ? devtools_window->tab_contents() : NULL;
-  WebContents* devtools_contents = devtools_tab_contents ?
-      devtools_tab_contents->web_contents() : NULL;
-
-  if (devtools_contents == devtools_container_->web_contents()) {
-    if (devtools_contents &&
-        devtools_window->dock_side() != devtools_dock_side_)
-      SetDevToolsDockSide(devtools_window->dock_side());
-    return;
+  // Fast return in case of the same window having same orientation.
+  if (devtools_window_ == new_devtools_window) {
+    if (!new_devtools_window ||
+        (new_devtools_window->dock_side() == devtools_dock_side_)) {
+      return;
+    }
   }
 
-  bool should_show = devtools_contents && !devtools_container_->visible();
-  bool should_hide = !devtools_contents && devtools_container_->visible();
+  // Replace tab contents.
+  if (devtools_window_ != new_devtools_window) {
+    devtools_container_->SetWebContents(new_devtools_window ?
+    new_devtools_window->tab_contents()->web_contents() : NULL);

[Peter Kasting, 2012/10/24 21:18:09]

Nit: Indent 4

[pfeldman, 2012/10/25 15:17:40]

Done.

+  }
 
-  devtools_container_->SetWebContents(devtools_contents);
-  RestackLocationBarContainer();
+  // Store last used position.
+  if (devtools_window_) {
+    if (devtools_dock_side_ == DEVTOOLS_DOCK_SIDE_RIGHT) {
+      devtools_window_->SetWidth(
+          contents_split_->width() - contents_split_->divider_offset());
+    } else {
+      devtools_window_->SetHeight(
+          contents_split_->height() - contents_split_->divider_offset());
+    }
+  }
 
-  if (should_show)
-    ShowDevToolsContainer(devtools_window->dock_side());
-  else if (should_hide)
+  // Show / hide container if necessary. Changing dock orientation is
+  // hide + show.
+  bool should_hide = devtools_window_ && (!new_devtools_window ||
+      devtools_dock_side_ != new_devtools_window->dock_side());
+  bool should_show = new_devtools_window && (!devtools_window_ || should_hide);
+
+  if (should_hide)
     HideDevToolsContainer();
+
+  devtools_window_ = new_devtools_window;
+
+  if (should_show) {
+    devtools_dock_side_ = new_devtools_window->dock_side();
+    ShowDevToolsContainer();
+  } else if (new_devtools_window) {
+    UpdateDevToolsSplitPosition();
+  }
 }
 
-void BrowserView::ShowDevToolsContainer(DevToolsDockSide dock_side) {
-  devtools_dock_side_ = dock_side;
+void BrowserView::ShowDevToolsContainer() {
   if (!devtools_focus_tracker_.get()) {
     // Install devtools focus tracker when dev tools window is shown for the
     // first time.
     devtools_focus_tracker_.reset(
         new views::ExternalFocusTracker(devtools_container_,
                                         GetFocusManager()));
   }
-
+  devtools_container_->SetVisible(true);
+  devtools_dock_side_ = devtools_window_->dock_side();
   bool dock_to_right = devtools_dock_side_ == DEVTOOLS_DOCK_SIDE_RIGHT;
-
-  int contents_size = dock_to_right ? contents_split_->width() :
-      contents_split_->height();
-  int min_size = dock_to_right ? kMinDevToolsWidth : kMinDevToolsHeight;
-
-  // Restore split offset.
-  int split_offset = browser_->profile()->GetPrefs()->
-      GetInteger(dock_to_right ? prefs::kDevToolsVSplitLocation :
-                                 prefs::kDevToolsHSplitLocation);
-
-  if (split_offset == -1)
-    split_offset = contents_size * 1 / 3;
-
-  // Make sure user can see both panes.
-  split_offset = std::max(min_size, split_offset);
-  split_offset = std::min(contents_size - kMinContentsSize, split_offset);
-  if (split_offset < 0)
-    split_offset = contents_size * 1 / 3;
-  contents_split_->set_divider_offset(contents_size - split_offset);
-
-  devtools_container_->SetVisible(true);
   contents_split_->set_orientation(
       dock_to_right ? views::SingleSplitView::HORIZONTAL_SPLIT
                     : views::SingleSplitView::VERTICAL_SPLIT);
+  UpdateDevToolsSplitPosition();
   contents_split_->InvalidateLayout();
   Layout();
 }
 
 void BrowserView::HideDevToolsContainer() {
-  // Store split offset when hiding devtools window only.
-  bool dock_to_right = devtools_dock_side_ == DEVTOOLS_DOCK_SIDE_RIGHT;
-  int contents_size = dock_to_right ? contents_split_->width() :
-      contents_split_->height();
-
-  browser_->profile()->GetPrefs()->SetInteger(
-      dock_to_right ? prefs::kDevToolsVSplitLocation :
-                      prefs::kDevToolsHSplitLocation,
-      contents_size - contents_split_->divider_offset());
-
   // Restore focus to the last focused view when hiding devtools window.
   devtools_focus_tracker_->FocusLastFocusedExternalView();
-
   devtools_container_->SetVisible(false);
   contents_split_->InvalidateLayout();
   Layout();
 }
 
+void BrowserView::UpdateDevToolsSplitPosition() {
+  if (devtools_window_->dock_side() == DEVTOOLS_DOCK_SIDE_RIGHT) {

[Peter Kasting, 2012/10/24 21:18:09]

Nit: Simpler:

  contents_split_->set_divider_offset(
      (devtools_window_->dock_side() == DEVTOOLS_DOCK_SIDE_RIGHT) ?
          (contents_split_->width() -
              devtools_window_->GetWidth(contents_split_->width())) :
          (contents_split_->height() -
              devtools_window_->GetHeight(contents_split_->height())));

You can make this even simpler if instead of having GetWidth()/GetHeight()
return the dev tools size, they return the content size.  Then you can simplify
to something like:

  contents_split_->set_divider_offset(
      (devtools_window_->dock_side() == DEVTOOLS_DOCK_SIDE_RIGHT) ?
          devtools_window_->GetContentsWidth(contents_split_->width()) :
          devtools_window_->GetContentsHeight(contents_split_->height()));

[pfeldman, 2012/10/25 15:17:40]

I did not want to return the content size, because it is not really a size:
depending on the devtools position, it holds value for either one dimention or
the other. Taken together, they do not really form devtools size.

+    int split_offset = contents_split_->width() -
+        devtools_window_->GetWidth(contents_split_->width());
+    contents_split_->set_divider_offset(split_offset);
+  } else {
+    int split_offset = contents_split_->height() -
+        devtools_window_->GetHeight(contents_split_->height());
+    contents_split_->set_divider_offset(split_offset);
+  }
+}
+
 void BrowserView::UpdateUIForContents(TabContents* contents) {
   bool needs_layout = MaybeShowBookmarkBar(contents);
   needs_layout |= MaybeShowInfoBar(contents);
   if (needs_layout)
     Layout();
 }
 
 bool BrowserView::UpdateChildViewAndLayout(views::View* new_view,
                                            views::View** old_view) {
   DCHECK(old_view);
@@ skipping 484 matching lines @@
 
   Browser* modal_browser =
       browser::FindBrowserWithWebContents(active_dialog->web_contents());
   if (modal_browser && (browser_ != modal_browser)) {
     modal_browser->window()->FlashFrame(true);
     modal_browser->window()->Activate();
   }
 
   AppModalDialogQueue::GetInstance()->ActivateModalDialog();
 }