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 175 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 320 matching lines @@
   if (MaybeShowBookmarkBar(GetActiveTabContents()))
     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 1343 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);
+  }
 
-  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());

[Peter Kasting, 2012/10/26 19:35:41]

The reason I said DevToolsWindow doesn't need width_ and height_ is because you
have those values here and can pass them back in to GetWidth() and GetHeight()
later.  And you don't need to gate their calculation on whether
|devtools_window_| is non-NULL, you can do it all the time.

This all becomes clearer if you inline ShowDevToolsContainer() and
HideDevToolsContainer() into this function.

[pfeldman, 2012/10/29 12:24:25]

The problem is that there is 1 browser_view instance with single, _current_
width and height values. At the same time, there are N tabs and N
devtools_windows, each having its own width and height. And then you select tab
"i", you need to restore corresponding width, belonging to that very devtools
instance.

[Peter Kasting, 2012/10/29 20:30:50]

OK, I finally figured out what was confusing me.  It was a combination of
interpreting contents_split_->width() as contents_split_->divider_offset() and
interpreting GetWidth()'s container_width argument as what we'd set the width_
to.

I still think a slight simplification is possible.  It seems like
GetWidth()/GetHeight() could still be static methods that calculate the
appropriate value and then return it.  There's no need to write width_/height_
from these methods since the block here in the caller will set them anyway
before we change tabs away.  This makes it clearer that these variables are only
relevant for caching the window width and height during the time when it's not
visible, as opposed to also being used for something while the window is
showing, or causing the window's width and height to actually change as a direct
result of calling GetWidth()/GetHeight().

Once this is done, a few comments could be fleshed out.  width_ and height_
themselves should be commented to the effect of the above paragraph.  The block
here should say something more like "Tell the current devtools window, if any,
to save off its current size before we change tabs, so we can restore the size
on switching back later."

This also raises another question.  We don't write the on-disk pref until you
switch away from a tab.  Does this code get reached during browser shutdown as
well?  If not, we'll basically "forget" the last set of changes the user makes
to the split location.

+    } 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();
   // In NTP search mode, schedule a repaint of toolbar and tabstrip.
   if (browser()->search_model()->mode().is_ntp()) {
     toolbar_->SchedulePaint();
     tabstrip_->SchedulePaint();
   }
 }
 
 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();
   // In NTP search mode, schedule a repaint of toolbar and tabstrip.
   if (browser()->search_model()->mode().is_ntp()) {
     toolbar_->SchedulePaint();
     tabstrip_->SchedulePaint();
   }
 }
 
+void BrowserView::UpdateDevToolsSplitPosition() {
+  if (devtools_window_->dock_side() == DEVTOOLS_DOCK_SIDE_RIGHT) {
+    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 485 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();
 }