alternate ntp: implement Show/HideBars API to reduce jank when showing/hiding bars

chrome/browser/ui/browser.cc

Index: chrome/browser/ui/browser.cc
diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc
index 332016db7727acfe8adaab524d6eb037dc94077f..24895e8b454325960180d5754604eadbc32d4921 100644
--- a/chrome/browser/ui/browser.cc
+++ b/chrome/browser/ui/browser.cc
@@ -1227,14 +1227,9 @@ void Browser::MaybeUpdateBookmarkBarStateForInstantOverlay(
     const chrome::search::Mode& mode) {
   // This is invoked by a platform-specific implementation of
   // |InstantOverlayController| to update bookmark bar state according to
-  // Instant overlay state.
-  // ModeChanged() updates bookmark bar state for all mode transitions except
-  // when new mode is |SEARCH_SUGGESTIONS|, because that needs to be done when
-  // the suggestions are ready.
-  if (mode.is_search_suggestions() &&
-      bookmark_bar_state_ == BookmarkBar::SHOW) {
+  // Instant overlay state.  Only handle |DEFAULT| pages for this callback.
+  if (mode.is_origin_default())
     UpdateBookmarkBarState(BOOKMARK_BAR_STATE_CHANGE_TAB_STATE);
-  }
 }
 
 void Browser::ShowDownload(content::DownloadItem* download) {
@@ -1812,22 +1807,30 @@ void Browser::Observe(int type,
 
 void Browser::ModeChanged(const chrome::search::Mode& old_mode,
                           const chrome::search::Mode& new_mode) {
-  // If new mode is |SEARCH_SUGGESTIONS|, don't update bookmark bar state now;
-  // wait till the Instant overlay is ready to show suggestions before hiding
-  // the bookmark bar (in MaybeUpdateBookmarkBarStateForInstantOverlay()).
-  // TODO(kuan): but for now, only delay updating bookmark bar state if origin
-  // is |DEFAULT|; other origins require more complex logic to be implemented
-  // to prevent jankiness caused by hiding bookmark bar, so just hide the
-  // bookmark bar immediately and tolerate the jankiness for a while.
+  // If new mode is |SEARCH_SUGGESTIONS| or |SEARCH_RESULTS|, don't update

[dhollowa, 2013/03/08 00:03:32]

A note here mentioning the similar code in infobars would be helpful.  I.e. that
they should stay in sync.  A corresponding note in the infobar code would be
good too.

[Peter Kasting, 2013/03/08 00:51:36]

Better yet, move all this code out to a common location that explicitly tells
this code and the infobar controller what to do, and make this code (and that
code) dumb receivers of these explicit instructions instead of listeners for the
more low-level callbacks.

I already want that for the infobar code anyway (see comments there), the fact
that we have similar code here just suggests even more strongly that
centralizing these decisions in the instant controller or some similar place is
a good move.

[dhollowa, 2013/03/08 01:14:19]

+1

Maybe a utility of the form: c::s::Visibility VisibilityChange(old_mode,
new_mod) {...}

Then here in the handlers:
  switch (VisibiltyChange(old_mode, new_mod))
    NO_CHANGE: return;
    SHOW: ...
    HIDE:
  }


  if (Vis

[kuan, 2013/03/12 18:00:38]

i've provided a chrome::search::ShouldHandleTopBarsVisibilityChange utility
function that both Browser and InfoBarContainer classes call to determine the
logic.

+  // bookmark bar state now:
+  // - for |DEFAULT| page, wait for Instant overlay to show or hide, via
+  //   MaybeUpdateBookmarkBarStateForInstantOverlay().
+  // - for |NTP| and |SERP| pages, wait for SearchBox API callback via
+  //   TopBarsVisibilityChanged().
   // For other mode transitions, update bookmark bar state accordingly.
-  if (new_mode.is_search_suggestions() &&
-      new_mode.is_origin_default() &&
-      bookmark_bar_state_ == BookmarkBar::SHOW) {
+  if (new_mode.is_search())
     return;
-  }
   UpdateBookmarkBarState(BOOKMARK_BAR_STATE_CHANGE_TAB_STATE);
 }
 
+void Browser::TopBarsVisibilityChanged(const chrome::search::Mode& mode,
+                                       bool visible) {
+  // Only handle non-|DEFAULT| pages in |SEARCH_SUGGESTIONS| or
+  // |SEARCH_RESULTS| modes for this callback.
+  if (!mode.is_search() || mode.is_origin_default())
+    return;
+  if ((visible && bookmark_bar_state() == BookmarkBar::HIDDEN) ||
+      (!visible && bookmark_bar_state() != BookmarkBar::HIDDEN)) {
+    UpdateBookmarkBarState(BOOKMARK_BAR_STATE_CHANGE_TAB_STATE);
+  }
+}
+
 ///////////////////////////////////////////////////////////////////////////////
 // Browser, Command and state updating (private):
 
@@ -2135,11 +2138,31 @@ void Browser::UpdateBookmarkBarState(BookmarkBarStateChangeReason reason) {
       state = BookmarkBar::HIDDEN;
   }
 
-  // Don't allow the bookmark bar to be shown in suggestions mode.
+  // Bookmark bar may need to be hidden for |SEARCH_SUGGESTIONS| and
+  // |SEARCH_RESULTS| modes as per SearchBox API or Instant overlay.
 #if !defined(OS_MACOSX)

[dhollowa, 2013/03/08 00:03:32]

Please add a note as to why this doesn't apply to Mac.

[kuan, 2013/03/12 18:00:38]

Done.

-  if (search_model_->mode().is_search_suggestions())
-    state = BookmarkBar::HIDDEN;
-#endif
+  const chrome::search::Mode& mode = search_model_->mode();
+  if (state != BookmarkBar::HIDDEN && mode.is_search()) {
+    // Hide bookmark bar if:
+    // - for |DEFAULT| pages: Instant overlay is showing.
+    // - for non-|DEFAULT| pages: if bookmark bar is detached or SearchBox API
+    //   says so.
+    if (mode.is_origin_default()) {
+      // TODO(kuan): change GetOverlayContents() to overlay() after sreeram's
+      // refactoring, which should correctly indicate show/hide state of Instant
+      // overlay.  For now, GetOverlayContents() returns non-NULL even when
+      // Instant overlay is hidden, resulting in an empty bookmark bar; this is
+      // tracked at http://crbug.com/175776.
+      if (instant_controller_ &&
+          instant_controller_->instant()->model()->GetOverlayContents()) {
+        state = BookmarkBar::HIDDEN;
+      }
+    } else if (state == BookmarkBar::DETACHED ||
+               !search_model_->show_top_bars()) {
+      state = BookmarkBar::HIDDEN;
+    }
+  }
+#endif  // !defined(OS_MACOSX)
 
   if (state == bookmark_bar_state_)
     return;