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

chrome/browser/ui/browser.cc

 // Copyright 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/browser.h"
 
 #if defined(OS_WIN)
 #include <windows.h>
 #include <shellapi.h>
 #endif  // defined(OS_WIN)
@@ skipping 1209 matching lines @@
 }
 
 void Browser::ShowFirstRunBubble() {
   window()->GetLocationBar()->ShowFirstRunBubble();
 }
 
 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) {
   if (!window())
     return;
 
   // If the download occurs in a new tab, and it's not a save page
   // download (started before initial navigation completed) close it.
   WebContents* source = download->GetWebContents();
   if (source && source->GetController().IsInitialNavigation() &&
@@ skipping 557 matching lines @@
       break;
     }
 
     default:
       NOTREACHED() << "Got a notification we didn't register for.";
   }
 }
 
 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);
   }
-  UpdateBookmarkBarState(BOOKMARK_BAR_STATE_CHANGE_TAB_STATE);
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // Browser, Command and state updating (private):
 
 void Browser::OnDevToolsDisabledChanged() {
   if (profile_->GetPrefs()->GetBoolean(prefs::kDevToolsDisabled))
     content::DevToolsManager::GetInstance()->CloseAllClientHosts();
 }
 
@@ skipping 289 matching lines @@
   } else {
     WebContents* web_contents = tab_strip_model_->GetActiveWebContents();
     BookmarkTabHelper* bookmark_tab_helper =
         web_contents ? BookmarkTabHelper::FromWebContents(web_contents) : NULL;
     if (bookmark_tab_helper && bookmark_tab_helper->ShouldShowBookmarkBar())
       state = BookmarkBar::DETACHED;
     else
       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;
 
   bookmark_bar_state_ = state;
 
   if (!window_)
     return;  // This is called from the constructor when window_ is NULL.
 
   if (reason == BOOKMARK_BAR_STATE_CHANGE_TAB_SWITCH) {
@@ skipping 89 matching lines @@
   if (contents && !allow_js_access) {
     contents->web_contents()->GetController().LoadURL(
         target_url,
         content::Referrer(),
         content::PAGE_TRANSITION_LINK,
         std::string());  // No extra headers.
   }
 
   return contents != NULL;
 }