alternate ntp: fix website page jankiness when suggestions show and top bars are hidden

chrome/browser/ui/views/frame/browser_view_layout.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_layout.h"
 
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/ui/find_bar/find_bar.h"
 #include "chrome/browser/ui/find_bar/find_bar_controller.h"
+#include "chrome/browser/ui/search/search_model.h"
 #include "chrome/browser/ui/view_ids.h"
 #include "chrome/browser/ui/views/bookmarks/bookmark_bar_view.h"
 #include "chrome/browser/ui/views/download/download_shelf_view.h"
 #include "chrome/browser/ui/views/frame/browser_frame.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/browser/ui/views/frame/contents_container.h"
 #include "chrome/browser/ui/views/immersive_mode_controller.h"
 #include "chrome/browser/ui/views/infobars/infobar_container_view.h"
 #include "chrome/browser/ui/views/tabs/tab_strip.h"
 #include "chrome/browser/ui/views/toolbar_view.h"
 #include "ui/base/hit_test.h"
 #include "ui/gfx/point.h"
 #include "ui/gfx/scrollbar_size.h"
 #include "ui/gfx/size.h"
 #include "ui/views/controls/single_split_view.h"
+#include "ui/views/controls/webview/webview.h"
 
 namespace {
 
 // The visible height of the shadow above the tabs. Clicks in this area are
 // treated as clicks to the frame, rather than clicks to the tab.
 const int kTabShadowSize = 2;
 // The vertical overlap between the TabStrip and the Toolbar.
 const int kToolbarTabStripVerticalOverlap = 3;
 // The number of pixels the bookmark bar should overlap the spacer by if the
 // spacer is visible.
@@ skipping 215 matching lines @@
 
 void BrowserViewLayout::ViewRemoved(views::View* host, views::View* view) {
   switch (view->id()) {
     case VIEW_ID_BOOKMARK_BAR:
       active_bookmark_bar_ = NULL;
       break;
   }
 }
 
 void BrowserViewLayout::Layout(views::View* host) {
+  // If pinned bookmark bar and infobars are hidden because suggestions are
+  // showing on a website page, the real estate released by the hidden bars
+  // after this layout will cause the contents to shift up visually, resulting
+  // in jankiness.  To prevent the contents from shifting up, remember its
+  // origin (relative to |BrowserView|) before this layout starts, so that it
+  // can be restored after this layout finishes.
+  // Another way to preserve the contents page position is to determine the
+  // total height of all hidden bars.  While that's possible with bookmark bar
+  // (i.e active_bookmark_bar_->height() -
+  //      views::NonClientFrameView::kClientEdgeThickness +
+  //      active_bookmark_bar_->GetToolbarOverlap(false)),
+  // it's not possible with infobar container.  By the time Layout() is called,
+  // infobar container has already hidden all its bars, causing its
+  // GetVerticalOverlap() to return 0; as a result, the actual height of the
+  // infobars without the overlap cannot be determined.

[Peter Kasting, 2013/01/28 21:51:00]

This comment confused the heck out of me.

How about something like this, assuming I understand the problem (which I'm not
sure I do):

Showing instant extended suggestions causes us to temporarily hide any visible
bookmark bar and infobars.  In turn, this hiding would normally cause the
content below the suggestions to shift upwards, which looks surprising (since
from the user's perspective, we're "covering" rather than "removing" the
bookmark bar/infobars).  To prevent this, we save off the content origin here,
then once we finish laying things out, force the content to continue to display
from that origin.

Note that this avoids talking about alternative solutions (which is just
confusing).

[kuan, 2013/01/29 00:17:08]

Done.

+  const chrome::search::Mode& mode = browser()->search_model()->mode();
+  views::WebView* contents = browser_view_->contents_container_;
+  gfx::Point old_contents_origin;
+  if (mode.is_search_suggestions() && mode.is_origin_default() &&
+      // Bookmark bar is visible now but will be hidden during layout.
+      ((active_bookmark_bar_ && active_bookmark_bar_->visible() &&
+        browser()->bookmark_bar_state() == BookmarkBar::HIDDEN) ||
+      // Infobar container is visible now but will be hidden during layout.
+       (browser_view_->infobar_container_->visible() && !InfobarVisible()))) {
+    old_contents_origin = contents->bounds().origin();
+    views::View::ConvertPointToTarget(contents->parent(), browser_view_,
+                                      &old_contents_origin);
+  }
+
   vertical_layout_rect_ = browser_view_->GetLocalBounds();
   int top = LayoutTabStripRegion();
   if (browser_view_->IsTabStripVisible()) {
     int x = browser_view_->tabstrip_->GetMirroredX() +
         browser_view_->GetMirroredX() +
         browser_view_->frame()->GetThemeBackgroundXInset();
     browser_view_->tabstrip_->SetBackgroundOffset(gfx::Point(x,
         browser_view_->frame()->GetTabStripInsets(false).top));
   }
   top = LayoutToolbar(top);
   top = LayoutBookmarkAndInfoBars(top);
   // During immersive mode reveal the content stays near the top of the view.
   if (browser_view_->immersive_mode_controller()->IsRevealed()) {
     top = browser_view_->tabstrip_->y();
     if (!browser_view_->immersive_mode_controller()->hide_tab_indicators())
       top += TabStrip::GetImmersiveHeight();
   }
 
   int bottom = LayoutDownloadShelf(browser_view_->height());
   int active_top_margin = GetTopMarginForActiveContent();
   top -= active_top_margin;
   contents_container_->SetActiveTopMargin(active_top_margin);
   LayoutTabContents(top, bottom);
+
+  // If previous origin of contents page relative to |BrowserView| needs to be
+  // preserved, set the vertical difference between new and previous origins as
+  // the active top margin in contents container, so that the contents don't
+  // shift up due to hiding of bookmark and info bars.
+  if (active_top_margin == 0 && !old_contents_origin.IsOrigin()) {
+    gfx::Point new_contents_origin(contents->bounds().origin());
+    views::View::ConvertPointToTarget(contents->parent(), browser_view_,
+                                      &new_contents_origin);
+    active_top_margin = old_contents_origin.y() - new_contents_origin.y();
+    DCHECK_GE(active_top_margin, 0);
+    // If ContentsContainer::SetPreview() has not been called yet, preview
+    // height will be 0, in which case, preserve the previous contents y origin,
+    // so that it can be restored in an upcoming Layout() call after the preview
+    // has been set.
+    // Otherwise, if preview is set, preserve previous contents origin only if
+    // preview is taller than all hidden bars, so as to prevent a blank area
+    // from showing above the contents (i.e. below the preview).
+    // Note that if Layout() is triggered by hiding of:
+    // - bookmark bar, preview would have been set because bookmark bar state is
+    //   updated from |Browser| after |InstantPreviewControllerView| receives
+    //   the notification that preview is ready and sets it.
+    // - infobars, there's no guarantee that preview has been set; if infobar
+    //   container receives instant-preview-ready notification before
+    //   |InstantPreviewControllerView|, there won't be preview here yet;
+    //   hence the speical handling of the 0 preview height case.

[Peter Kasting, 2013/01/28 21:51:00]

Again, all these comments are really confusing.  Don't assume readers know what
previews are, how instant works, or even what the "active top margin is" (which
I tried to look up and found like no documentation on, awesome).  Try and write
succinctly but for a reader who doesn't have any particular domain knowledge.  I
can't suggest an alternate set of comments in here because to be honest I simply
can't understand what any of this is doing well enough.

[kuan, 2013/01/29 00:17:08]

Done.  i apologize my attempt to explain everything in detail backfired; i've
shortened the comments, and added comments for SetActiveTopMargin in
contents_container.h.

+    int preview_height = contents_container_->preview_height();
+    if (preview_height == 0 || active_top_margin < preview_height)
+      contents_container_->SetActiveTopMargin(active_top_margin);
+  }
+
   // This must be done _after_ we lay out the WebContents since this
   // code calls back into us to find the bounding box the find bar
   // must be laid out within, and that code depends on the
   // TabContentsContainer's bounds being up to date.
   if (browser()->HasFindBarController()) {
     browser()->GetFindBarController()->find_bar()->MoveWindowIfNecessary(
         gfx::Rect(), true);
   }
 }
 
@@ skipping 20 matching lines @@
     tabstrip->SetVisible(false);
     tabstrip->SetBounds(0, 0, 0, 0);
     return 0;
   }
   // This retrieves the bounds for the tab strip based on whether or not we show
   // anything to the left of it, like the incognito avatar.
   gfx::Rect tabstrip_bounds(
       browser_view_->frame()->GetBoundsForTabStrip(tabstrip));
   gfx::Point tabstrip_origin(tabstrip_bounds.origin());
   views::View::ConvertPointToTarget(browser_view_->parent(), browser_view_,
-                                  &tabstrip_origin);
+                                    &tabstrip_origin);
   tabstrip_bounds.set_origin(tabstrip_origin);
 
   tabstrip->SetVisible(true);
   tabstrip->SetBoundsRect(tabstrip_bounds);
   int bottom = tabstrip_bounds.bottom();
 
   // The metro window switcher sits at the far right edge of the tabstrip
   // a |kWindowSwitcherOffsetX| pixels from the right edge.
   // Only visible if there is more than one type of window to switch between.
   // TODO(mad): update this code when more window types than just incognito
@@ skipping 162 matching lines @@
   }
   return bottom;
 }
 
 bool BrowserViewLayout::InfobarVisible() const {
   views::View* infobar_container = browser_view_->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);
 }