Make the relationship between the find bar position and the top container height more obvious

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

 // Copyright 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_layout.h"
 
 #include "base/observer_list.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/find_bar/find_bar.h"
@@ skipping 95 matching lines @@
 // BrowserViewLayout, public:
 
 BrowserViewLayout::BrowserViewLayout()
     : browser_(NULL),
       browser_view_(NULL),
       bookmark_bar_(NULL),
       infobar_container_(NULL),
       contents_split_(NULL),
       contents_container_(NULL),
       download_shelf_(NULL),
-      find_bar_y_(0),
       ALLOW_THIS_IN_INITIALIZER_LIST(
           dialog_host_(new WebContentsModalDialogHostViews(this))),
       web_contents_modal_dialog_top_y_(-1) {
 }
 
 BrowserViewLayout::~BrowserViewLayout() {
 }
 
 void BrowserViewLayout::Init(Browser* browser,
                              BrowserView* browser_view,
@@ skipping 40 matching lines @@
   int widths[] = {
         tabstrip_size.width() + tab_strip_insets.left + tab_strip_insets.right,
         toolbar_size.width(),
         bookmark_bar_size.width(),
         contents_size.width() };
   int min_width = *std::max_element(&widths[0], &widths[arraysize(widths)]);
   return gfx::Size(min_width, min_height);
 }
 
 gfx::Rect BrowserViewLayout::GetFindBarBoundingBox() const {
-  // This function returns the area the Find Bar can be laid out
-  // within. This basically implies the "user-perceived content
-  // area" of the browser window excluding the vertical
-  // scrollbar. This is not quite so straightforward as positioning
-  // based on the TabContentsContainer since the BookmarkBarView may
-  // be visible but not persistent (in the New Tab case) and we
-  // position the Find Bar over the top of it in that case since the
-  // BookmarkBarView is not _visually_ connected to the Toolbar.
+  // This function returns the area the Find Bar can be laid out within. This
+  // basically implies the "user-perceived content area" of the browser
+  // window excluding the vertical scrollbar. The "user-perceived content area"
+  // excludes the detached bookmark bar (in the New Tab case) and any infobars
+  // since they are not _visually_ connected to the Toolbar.
 
   // First determine the bounding box of the content area in Widget
   // coordinates.
   gfx::Rect bounding_box = contents_container_->ConvertRectToWidget(
       contents_container_->GetLocalBounds());
 
-  // Adjust the position and size of the bounding box by the find bar offset
-  // calculated during the last Layout.
-  int height_delta = find_bar_y_ - bounding_box.y();
-  bounding_box.set_y(find_bar_y_);
+  // The find bar is postioned right below the top container.
+  int find_bar_y = browser_view_->y() +

[James Cook, 2013/04/26 16:14:51]

I think this would be clearer if you either did this computation with
ConvertPointToWidget to emphasize that all math is done in Widget coordinates,
or changed the above ConvertRectToWidget to do all the math in BrowserView
coordinates.

I would also document the coordinate system of the returned gfx::Rect of this
function.

[pkotwicz, 2013/04/27 00:02:43]

- Converted explicitly to widget coordinates.
- Added comment to browser_view_layout.h to document the coordinate system. A
better comment is already present in browser_view.h

+      browser_view_->top_container()->bounds().bottom() - 1;
+
+  // Grow the height of |bounding_box| by the height of any elements between
+  // the top container and |contents_container_| such as the detached bookmark
+  // bar and any infobars.
+  int height_delta = find_bar_y - bounding_box.y();
+  bounding_box.set_y(find_bar_y);
   bounding_box.set_height(std::max(0, bounding_box.height() + height_delta));
 
   // Finally decrease the width of the bounding box by the width of
   // the vertical scroll bar.
   int scrollbar_width = gfx::scrollbar_size();
   bounding_box.set_width(std::max(0, bounding_box.width() - scrollbar_width));
   if (base::i18n::IsRTL())
     bounding_box.set_x(bounding_box.x() + scrollbar_width);
 
   return bounding_box;
@@ skipping 200 matching lines @@
   int height = toolbar_visible ? toolbar->GetPreferredSize().height() : 0;
   toolbar->SetVisible(toolbar_visible);
   toolbar->SetBounds(vertical_layout_rect_.x(), y, browser_view_width, height);
 
   return y + height;
 }
 
 int BrowserViewLayout::LayoutBookmarkAndInfoBars(int top) {
   web_contents_modal_dialog_top_y_ =
       top + browser_view_->y() - kConstrainedWindowOverlap;
-  find_bar_y_ = top + browser_view_->y() - 1;
   if (bookmark_bar_) {
     // If we're showing the Bookmark bar in detached style, then we
     // need to show any Info bar _above_ the Bookmark bar, since the
     // Bookmark bar is styled to look like it's part of the page.
     if (bookmark_bar_->IsDetached())
       return LayoutBookmarkBar(LayoutInfoBar(top));
     // Otherwise, Bookmark bar first, Info bar second.
     top = std::max(browser_view_->toolbar_->bounds().bottom(),
                    LayoutBookmarkBar(top));
   }
-  find_bar_y_ = top + browser_view_->y() - 1;
   return LayoutInfoBar(top);
 }
 
 int BrowserViewLayout::LayoutBookmarkBar(int top) {
   int y = top;
   if (!browser_view_->IsBookmarkBarVisible()) {
     bookmark_bar_->SetVisible(false);
     // TODO(jamescook): Don't change the bookmark bar height when it is
     // invisible, so we can use its height for layout even in that state.
     bookmark_bar_->SetBounds(0, y, browser_view_->width(), 0);
@@ skipping 132 matching lines @@
   return bottom;
 }
 
 bool BrowserViewLayout::InfobarVisible() const {
   // Cast to a views::View to access GetPreferredSize().
   views::View* infobar_container = infobar_container_;
   // NOTE: Can't check if the size IsEmpty() since it's always 0-width.
   return browser_->SupportsWindowFeature(Browser::FEATURE_INFOBAR) &&
       (infobar_container->GetPreferredSize().height() != 0);
 }