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

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

Index: chrome/browser/ui/views/frame/browser_view_layout.cc
diff --git a/chrome/browser/ui/views/frame/browser_view_layout.cc b/chrome/browser/ui/views/frame/browser_view_layout.cc
index 00311c5eb48adc645296308410b000f51fd54005..37bad4755153cd38fb94856e8a0797e80509ebfb 100644
--- a/chrome/browser/ui/views/frame/browser_view_layout.cc
+++ b/chrome/browser/ui/views/frame/browser_view_layout.cc
@@ -7,6 +7,7 @@
 #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"
@@ -22,6 +23,7 @@
 #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 {
 
@@ -257,6 +259,35 @@ void BrowserViewLayout::ViewRemoved(views::View* host, views::View* view) {
 }
 
 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()) {
@@ -280,6 +311,37 @@ void BrowserViewLayout::Layout(views::View* host) {
   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
@@ -320,7 +382,7 @@ int BrowserViewLayout::LayoutTabStripRegion() {
       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);