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..a7b21c694f7e376abbe7d131fe5a42222c9e312f 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,27 @@ void BrowserViewLayout::ViewRemoved(views::View* host, views::View* view) {
 }
 
 void BrowserViewLayout::Layout(views::View* host) {
+  // 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
+  // contents to continue to display from that origin.
+  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 +303,25 @@ 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.

[Peter Kasting, 2013/01/29 00:36:29]

Nit: How about just:

// Now set the contents to display at their previous origin if we just hid the
bookmark and/or infobars.

[kuan, 2013/01/29 01:00:11]

Done.

+  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);
+    int preview_height = contents_container_->preview_height();
+    // Only preserve the previous content origin if:
+    // - suggesions are not ready yet i.e. |preview_height| is 0
+    // - suggestions completely cover the bookmark bar/infobars that are hidden.

[Peter Kasting, 2013/01/29 00:36:29]

Nit: This confused me for a while.  See if this explanation is accurate instead:

// Special case: While normally the suggestions appear to "cover" any
bookmark/infobars, if the suggestions are very short, they might not fully cover
that gap, and leaving the contents at their original height would leave an
odd-looking blank space.  In this case, we allow the contents to go ahead and
shift upward.  (We don't shift up in the zero-preview-height case since that
corresponds to when the suggestions aren't actually ready to be displayed yet.)

This also makes me wonder: Do you really need to check for a zero preview height
here, if you ensure that we don't hide the bookmark bar/infobars until the
preview is ready?

[kuan, 2013/01/29 01:00:11]

Done.

The zero-preview-height case was what i tried, but failed, to explain in my
previous comments.

Layout() is triggered separately when bookmark bar and infobars are hidden.
Bookmark bar triggers Layout() from Browser class after ContentsContainer has
setup the preview for suggestions.
Infobar container triggers Layout() from itself, regardless if ContentsContainer
has setup the preview.  In this case, preview height cld be 0 in Layout() if
ContentsContainer hasn't setup the preview yet.

i slightly modified the last sentence of your comments to hopefully explain
this.

+    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 +362,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);