Cleanup in preparation for Fullscreen mode.

* Remove some unnecessary cruft from BrowserView.

* Change Find bar to position itself based on the translated bounds of the bookmark bar or toolbar, whichever is found.  This is a bit simpler than the offset-based version before and works much better with Fullscreen mode.

* Make Bookmark bar not have a strange "1 px" minimum size, which was only needed because we were always subtracting that much in the BrowserView layout functions.

* Change BrowserView toolbar layout functions to always lay out toolbars (instead of only when visible; they are 0-height when invisible), and explicitly set their visibility.  This shouldn't have any visible effect, but it goes hand-in-hand with the Find bar changes.

* Add a function on the Bookmark bar to determine if we're displaying detached, and change various callers to use it.  Right now this does fairly little, but in Fullscreen mode it becomes necessary because we always display the Bookmark bar this way.  There is one user-visible effect: when toggling the Bookmark bar on/off while the Find bar is visible, the Find bar will now snap to the Bookmark bar only when it finishes "attaching" to the toolbar, not as soon as it starts.  Finnur and I both think this actually looks slightly better.
Review URL: http://codereview.chromium.org/21359

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

5 files changed, 94 insertions, 144 deletions

diff --git a/chrome/browser/views/bookmark_bar_view.cc b/chrome/browser/views/bookmark_bar_view.cc
index 7deb438..1848bad 100644
--- a/chrome/browser/views/bookmark_bar_view.cc
+++ b/chrome/browser/views/bookmark_bar_view.cc
@@ -766,9 +766,8 @@ gfx::Size BookmarkBarView::GetPreferredSize() {
                         (kNewtabBarHeight - kBarHeight) *
                         (1 - size_animation_->GetCurrentValue())));
   } else {
-    prefsize.set_height(
-        std::max(static_cast<int>(static_cast<double>(kBarHeight) *
-                 size_animation_->GetCurrentValue()), 1));
+    prefsize.set_height(static_cast<int>(static_cast<double>(kBarHeight) *
+                        size_animation_->GetCurrentValue()));
   }
 
   // Width doesn't matter, we're always given a width based on the browser
@@ -878,7 +877,7 @@ void BookmarkBarView::ViewHierarchyChanged(bool is_add,
 }
 
 void BookmarkBarView::Paint(ChromeCanvas* canvas) {
-  if (OnNewTabPage() && (!IsAlwaysShown() || size_animation_->IsAnimating())) {
+  if (IsDetachedStyle()) {
     // Draw the background to match the new tab page.
     canvas->FillRectInt(kNewtabBackgroundColor, 0, 0, width(), height());
 
@@ -1106,6 +1105,10 @@ int BookmarkBarView::OnPerformDrop(const DropTargetEvent& event) {
   return PerformDropImpl(data, parent_node, index);
 }
 
+bool BookmarkBarView::IsDetachedStyle() {
+  return OnNewTabPage() && (size_animation_->GetCurrentValue() != 1);
+}
+
 bool BookmarkBarView::IsAlwaysShown() {
   return profile_->GetPrefs()->GetBoolean(prefs::kShowBookmarkBar);
 }
@@ -1376,7 +1379,7 @@ void BookmarkBarView::RunMenu(views::View* view,
   int x = view->GetX(APPLY_MIRRORING_TRANSFORMATION);
   int bar_height = height() - kMenuOffset;
 
-  if (OnNewTabPage() && !IsAlwaysShown())
+  if (IsDetachedStyle())
     bar_height -= kNewtabVerticalPadding;
 
   int start_index = 0;
diff --git a/chrome/browser/views/bookmark_bar_view.h b/chrome/browser/views/bookmark_bar_view.h
index 3cd7a57..d8ab41e 100644
--- a/chrome/browser/views/bookmark_bar_view.h
+++ b/chrome/browser/views/bookmark_bar_view.h
@@ -129,10 +129,11 @@ class BookmarkBarView : public views::View,
   // Returns the model.
   BookmarkModel* GetModel() { return model_; }
 
-  // Returns true if the bookmarks bar preference is set to 'always show', we
-  // use this as a shorthand way of knowing what style of bar to draw (if the
-  // pref is set to false but we're painting, then we must be on the new tab
-  // page).
+  // Returns true if the bookmark bar is drawn detached from the toolbar.  This
+  // can only be true when OnNewTabPage() is true (see below).
+  bool IsDetachedStyle();
+
+  // Returns true if the bookmarks bar preference is set to 'always show'.
   bool IsAlwaysShown();
 
   // True if we're on a page where the bookmarks bar is always visible.
diff --git a/chrome/browser/views/find_bar_win.cc b/chrome/browser/views/find_bar_win.cc
index 2d1d1c2..671cc92 100644
--- a/chrome/browser/views/find_bar_win.cc
+++ b/chrome/browser/views/find_bar_win.cc
@@ -481,39 +481,18 @@ void FindBarWin::GetDialogBounds(gfx::Rect* bounds) {
   // Start with browser's client rect, then change it below.
   *bounds = gfx::Rect(browser_client_rect);
 
-  // Find the dimensions of the toolbar and the BookmarkBar.
-  gfx::Rect toolbar_bounds, bookmark_bar_bounds;
-  if (toolbar) {
-    toolbar_bounds = toolbar->GetLocalBounds(false);
-    // Need to convert toolbar bounds into Widget coords because the toolbar
-    // is the child of another view that isn't the top level view. This is
-    // required to ensure correct positioning relative to the top,left of the
-    // window.
-    gfx::Point topleft;
-    views::View::ConvertPointToWidget(toolbar, &topleft);
-    toolbar_bounds.Offset(topleft.x(), topleft.y());
-  }
-
-  // If the bookmarks bar is available, we need to update our
-  // position and paint accordingly
-  if (bookmark_bar) {
-    if (bookmark_bar->IsAlwaysShown()) {
-      // If it's always on, don't try to blend with the toolbar.
+  // Find the bounds of the toolbar we want to dock to.
+  gfx::Rect toolbar_bounds;
+  views::View* bounds_view = NULL;
+  view_->SetToolbarBlend(true);
+  if (bookmark_bar && !bookmark_bar->IsDetachedStyle()) {
+    bounds_view = bookmark_bar;
+    toolbar_bounds = bookmark_bar->GetLocalBounds(false);
+    if (bookmark_bar->IsAlwaysShown())
       view_->SetToolbarBlend(false);
-    } else {
-      // Else it's on, but hidden (in which case we should try
-      // to blend with the toolbar.
-      view_->SetToolbarBlend(true);
-    }
-
-    // If we're not in the New Tab page style, align ourselves with
-    // the bookmarks bar (this works even if the bar is hidden).
-    if (!bookmark_bar->OnNewTabPage() ||
-        bookmark_bar->IsAlwaysShown()) {
-      bookmark_bar_bounds = bookmark_bar->bounds();
-    }
-  } else {
-    view_->SetToolbarBlend(true);
+  } else if (toolbar) {
+    bounds_view = toolbar;
+    toolbar_bounds = toolbar->GetLocalBounds(false);
   }
 
   // Figure out at which y coordinate to draw the FindInPage window. If we have
@@ -523,20 +502,18 @@ void FindBarWin::GetDialogBounds(gfx::Rect* bounds) {
   // window or a Chrome application so we want to draw at the top of the page
   // content (right beneath the title bar).
   int y_pos_offset = 0;
-  if (!toolbar_bounds.IsEmpty()) {
+  if (bounds_view) {
     // We have a toolbar (chrome), so overlap it by one pixel.
+    gfx::Point origin;
+    views::View::ConvertPointToWidget(bounds_view, &origin);
+    toolbar_bounds.Offset(origin);
     y_pos_offset = toolbar_bounds.bottom() - 1;
-    // If there is a bookmark bar attached to the toolbar we should appear
-    // attached to it instead of the toolbar.
-    if (!bookmark_bar_bounds.IsEmpty()) {
-      y_pos_offset += bookmark_bar_bounds.height() - 1 - 
-          bookmark_bar->GetToolbarOverlap();
-    }
   } else {
-    // There is no toolbar, so this is probably a constrained window or a Chrome
-    // Application. This means we draw the Find window at the top of the page
-    // content window. We subtract 1 to overlap the light-blue line that is part
-    // of the title bar (so that we don't look detached by 1 pixel).
+    // The toolbar isn't visible, so we're in fullscreen mode or this is an app
+    // window. This means we draw the Find window at the top of the page content
+    // window. We subtract 1 to overlap the client edge that is part of the
+    // title bar (so that we don't look detached by 1 pixel).
+    // TODO(pkasting): This -1 is wrong in maximized mode.
     WINDOWINFO wi;
     wi.cbSize = sizeof(WINDOWINFO);
     GetWindowInfo(parent_hwnd_, &wi);
diff --git a/chrome/browser/views/frame/browser_view.cc b/chrome/browser/views/frame/browser_view.cc
index b262967..144f0a1 100644
--- a/chrome/browser/views/frame/browser_view.cc
+++ b/chrome/browser/views/frame/browser_view.cc
@@ -177,7 +177,6 @@ BrowserView::BrowserView(Browser* browser)
       frame_(NULL),
       browser_(browser),
       active_bookmark_bar_(NULL),
-      active_info_bar_(NULL),
       active_download_shelf_(NULL),
       toolbar_(NULL),
       contents_container_(NULL),
@@ -191,8 +190,6 @@ BrowserView::BrowserView(Browser* browser)
 #endif
       forwarding_to_tab_strip_(false) {
   InitClass();
-  show_bookmark_bar_pref_.Init(prefs::kShowBookmarkBar,
-                               browser_->profile()->GetPrefs(), this);
   browser_->tabstrip_model()->AddObserver(this);
 }
 
@@ -366,19 +363,15 @@ void BrowserView::PrepareToRunSystemMenu(HMENU menu) {
 }
 
 bool BrowserView::SupportsWindowFeature(WindowFeature feature) const {
-  return !!(FeaturesForBrowserType(browser_->type()) & feature);
-}
-
-// static
-unsigned int BrowserView::FeaturesForBrowserType(Browser::Type type) {
+  const Browser::Type type = browser_->type();
   unsigned int features = FEATURE_INFOBAR | FEATURE_DOWNLOADSHELF;
   if (type == Browser::TYPE_NORMAL)
     features |= FEATURE_TABSTRIP | FEATURE_TOOLBAR | FEATURE_BOOKMARKBAR;
+  else
+    features |= FEATURE_TITLEBAR;
   if (type != Browser::TYPE_APP)
     features |= FEATURE_LOCATIONBAR;
-  if (type != Browser::TYPE_NORMAL)
-    features |= FEATURE_TITLEBAR;
-  return features;
+  return !!(features & feature);
 }
 
 // static
@@ -579,14 +572,9 @@ void BrowserView::DestroyBrowser() {
 }
 
 bool BrowserView::IsBookmarkBarVisible() const {
-  if (!bookmark_bar_view_.get())
-    return false;
-
-  if (bookmark_bar_view_->OnNewTabPage() || bookmark_bar_view_->IsAnimating())
-    return true;
-
-  // 1 is the minimum in GetPreferredSize for the bookmark bar.
-  return bookmark_bar_view_->GetPreferredSize().height() > 1;
+  return SupportsWindowFeature(FEATURE_BOOKMARKBAR) &&
+      bookmark_bar_view_.get() &&
+      (bookmark_bar_view_->GetPreferredSize().height() != 0);
 }
 
 gfx::Rect BrowserView::GetRootWindowResizerRect() const {
@@ -1165,52 +1153,40 @@ views::DropTargetEvent* BrowserView::MapEventToTabStrip(
 }
 
 int BrowserView::LayoutTabStrip() {
-  if (IsTabStripVisible()) {
-    gfx::Rect tabstrip_bounds = frame_->GetBoundsForTabStrip(tabstrip_);
-    gfx::Point tabstrip_origin = tabstrip_bounds.origin();
-    ConvertPointToView(GetParent(), this, &tabstrip_origin);
-    tabstrip_bounds.set_origin(tabstrip_origin);
-    tabstrip_->SetBounds(tabstrip_bounds.x(), tabstrip_bounds.y(),
-                         tabstrip_bounds.width(), tabstrip_bounds.height());
-    return tabstrip_bounds.bottom();
-  }
-  return 0;
+  gfx::Rect tabstrip_bounds = frame_->GetBoundsForTabStrip(tabstrip_);
+  gfx::Point tabstrip_origin = tabstrip_bounds.origin();
+  ConvertPointToView(GetParent(), this, &tabstrip_origin);
+  tabstrip_bounds.set_origin(tabstrip_origin);
+  bool visible = IsTabStripVisible();
+  int y = visible ? tabstrip_bounds.y() : 0;
+  int height = visible ? tabstrip_bounds.height() : 0;
+  tabstrip_->SetVisible(visible);
+  tabstrip_->SetBounds(tabstrip_bounds.x(), y, tabstrip_bounds.width(), height);
+  return y + height;
 }
 
 int BrowserView::LayoutToolbar(int top) {
-  if (IsToolbarVisible()) {
-    gfx::Size ps = toolbar_->GetPreferredSize();
-    int toolbar_y =
-        top - (IsTabStripVisible() ? kToolbarTabStripVerticalOverlap : 0);
-    int browser_view_width = width();
+  int browser_view_width = width();
 #ifdef CHROME_PERSONALIZATION
-    if (IsPersonalizationEnabled())
-      Personalization::AdjustBrowserView(personalization_, &browser_view_width);
+  if (IsPersonalizationEnabled())
+    Personalization::AdjustBrowserView(personalization_, &browser_view_width);
 #endif
-    toolbar_->SetBounds(0, toolbar_y, browser_view_width, ps.height());
-    return toolbar_y + ps.height();
-  }
-  toolbar_->SetVisible(false);
-  return top;
+  bool visible = IsToolbarVisible();
+  int y = top -
+      ((visible && IsTabStripVisible()) ? kToolbarTabStripVerticalOverlap : 0);
+  int height = visible ? toolbar_->GetPreferredSize().height() : 0;
+  toolbar_->SetVisible(visible);
+  toolbar_->SetBounds(0, y, browser_view_width, height);
+  return y + height;
 }
 
 int BrowserView::LayoutBookmarkAndInfoBars(int top) {
-  if (SupportsWindowFeature(FEATURE_BOOKMARKBAR)) {
-    // If we have an Info-bar showing, and we're showing the New Tab Page, and
-    // the Bookmark bar isn't visible on all tabs, then we need to show the
-    // Info bar _above_ the Bookmark bar, since the Bookmark bar is styled to
-    // look like it's part of the New Tab Page...
-    if (active_bookmark_bar_ &&
-        bookmark_bar_view_->OnNewTabPage() &&
-        !bookmark_bar_view_->IsAlwaysShown()) {
-      top = LayoutInfoBar(top);
-      return LayoutBookmarkBar(top);
-    }
-
-    // If we're showing a regular bookmark bar and it's not below an infobar,
-    // make it overlap the toolbar so that the bar items can be drawn higher.
-    if (active_bookmark_bar_)
-      top -= bookmark_bar_view_->GetToolbarOverlap();
+  if (bookmark_bar_view_.get()) {
+    // 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_view_->IsDetachedStyle())
+      return LayoutBookmarkBar(LayoutInfoBar(top));
 
     // Otherwise, Bookmark bar first, Info bar second.
     top = LayoutBookmarkBar(top);
@@ -1219,24 +1195,27 @@ int BrowserView::LayoutBookmarkAndInfoBars(int top) {
 }
 
 int BrowserView::LayoutBookmarkBar(int top) {
-  if (SupportsWindowFeature(FEATURE_BOOKMARKBAR) && active_bookmark_bar_) {
-    gfx::Size ps = active_bookmark_bar_->GetPreferredSize();
-    if (!active_info_bar_ || show_bookmark_bar_pref_.GetValue())
-      top -= kSeparationLineHeight;
-    active_bookmark_bar_->SetBounds(0, top, width(), ps.height());
-    top += ps.height();
+  DCHECK(bookmark_bar_view_.get());
+  bool visible = IsBookmarkBarVisible();
+  int height, y = top;
+  if (visible) {
+    y -= kSeparationLineHeight + (bookmark_bar_view_->IsDetachedStyle() ?
+        0 : bookmark_bar_view_->GetToolbarOverlap());
+    height = bookmark_bar_view_->GetPreferredSize().height();
+  } else {
+    height = 0;
   }
-  return top;
+  bookmark_bar_view_->SetVisible(visible);
+  bookmark_bar_view_->SetBounds(0, y, width(), height);
+  return y + height;
 }
 
 int BrowserView::LayoutInfoBar(int top) {
-  if (SupportsWindowFeature(FEATURE_INFOBAR)) {
-    // Layout the InfoBar container.
-    gfx::Size ps = infobar_container_->GetPreferredSize();
-    infobar_container_->SetBounds(0, top, width(), ps.height());
-    top += ps.height();
-  }
-  return top;
+  bool visible = SupportsWindowFeature(FEATURE_INFOBAR);
+  int height = visible ? infobar_container_->GetPreferredSize().height() : 0;
+  infobar_container_->SetVisible(visible);
+  infobar_container_->SetBounds(0, top, width(), height);
+  return top + height;
 }
 
 void BrowserView::LayoutTabContents(int top, int bottom) {
@@ -1245,12 +1224,14 @@ void BrowserView::LayoutTabContents(int top, int bottom) {
 
 int BrowserView::LayoutDownloadShelf() {
   int bottom = height();
-  if (SupportsWindowFeature(FEATURE_DOWNLOADSHELF) && active_download_shelf_) {
-    gfx::Size ps = active_download_shelf_->GetPreferredSize();
-    active_download_shelf_->SetBounds(0, bottom - ps.height(), width(),
-                                      ps.height());
+  if (active_download_shelf_) {
+    bool visible = SupportsWindowFeature(FEATURE_DOWNLOADSHELF);
+    int height =
+        visible ? active_download_shelf_->GetPreferredSize().height() : 0;
+    active_download_shelf_->SetVisible(visible);
+    active_download_shelf_->SetBounds(0, bottom - height, width(), height);
     active_download_shelf_->Layout();
-    bottom -= ps.height();
+    bottom -= height;
   }
   return bottom;
 }
@@ -1268,8 +1249,9 @@ void BrowserView::LayoutStatusBubble(int top) {
 
 bool BrowserView::MaybeShowBookmarkBar(TabContents* contents) {
   views::View* new_bookmark_bar_view = NULL;
+  views::View* old_bookmark_bar_view = bookmark_bar_view_.get();
   if (SupportsWindowFeature(FEATURE_BOOKMARKBAR) && contents) {
-    if (!bookmark_bar_view_.get()) {
+    if (!old_bookmark_bar_view) {
       bookmark_bar_view_.reset(new BookmarkBarView(contents->profile(),
                                                    browser_.get()));
       bookmark_bar_view_->SetParentOwned(false);
@@ -1278,13 +1260,9 @@ bool BrowserView::MaybeShowBookmarkBar(TabContents* contents) {
     }
     bookmark_bar_view_->SetPageNavigator(contents);
     new_bookmark_bar_view = bookmark_bar_view_.get();
-    if (!show_bookmark_bar_pref_.GetValue() &&
-        new_bookmark_bar_view->GetPreferredSize().height() == 0) {
-      new_bookmark_bar_view = NULL;
-    }
   }
   return UpdateChildViewAndLayout(new_bookmark_bar_view,
-                                  &active_bookmark_bar_);
+                                  &old_bookmark_bar_view);
 }
 
 bool BrowserView::MaybeShowInfoBar(TabContents* contents) {
@@ -1372,8 +1350,7 @@ void BrowserView::LoadAccelerators() {
   ACCEL* accelerators = static_cast<ACCEL*>(malloc(sizeof(ACCEL) * count));
   CopyAcceleratorTable(accelerator_table, accelerators, count);
 
-  views::FocusManager* focus_manager =
-      views::FocusManager::GetFocusManager(GetWidget()->GetHWND());
+  views::FocusManager* focus_manager = GetFocusManager();
   DCHECK(focus_manager);
 
   // Let's build our own accelerator table.
diff --git a/chrome/browser/views/frame/browser_view.h b/chrome/browser/views/frame/browser_view.h
index c8cce70..6f945b8 100644
--- a/chrome/browser/views/frame/browser_view.h
+++ b/chrome/browser/views/frame/browser_view.h
@@ -11,7 +11,6 @@
 #include "chrome/browser/hang_monitor/hung_window_detector.h"
 #include "chrome/browser/tabs/tab_strip_model.h"
 #include "chrome/browser/views/frame/browser_frame.h"
-#include "chrome/common/pref_member.h"
 #include "chrome/views/client_view.h"
 #include "chrome/views/window_delegate.h"
 #ifdef CHROME_PERSONALIZATION
@@ -151,9 +150,6 @@ class BrowserView : public BrowserWindow,
   // supports the specified feature.
   bool SupportsWindowFeature(WindowFeature feature) const;
 
-  // Returns the set of WindowFeatures supported by the specified BrowserType.
-  static unsigned int FeaturesForBrowserType(Browser::Type type);
-
   // Register preferences specific to this view.
   static void RegisterBrowserViewPrefs(PrefService* prefs);
 
@@ -351,7 +347,6 @@ class BrowserView : public BrowserWindow,
 
   // Tool/Info bars that we are currently showing. Used for layout.
   views::View* active_bookmark_bar_;
-  views::View* active_info_bar_;
   views::View* active_download_shelf_;
 
   // The TabStrip.
@@ -375,9 +370,6 @@ class BrowserView : public BrowserWindow,
   // A mapping between accelerators and commands.
   scoped_ptr<std::map<views::Accelerator, int>> accelerator_table_;
 
-  // A PrefMember to track the "always show bookmark bar" pref.
-  BooleanPrefMember show_bookmark_bar_pref_;
-
   // True if we have already been initialized.
   bool initialized_;