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

chrome/browser/infobars/infobar_container.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 "build/build_config.h"
 
 // TODO(pkasting): Port Mac to use this.
 #if defined(TOOLKIT_VIEWS) || defined(TOOLKIT_GTK)
 
 #include "chrome/browser/infobars/infobar_container.h"
@@ skipping 157 matching lines @@
       break;
     }
 
     default:
       NOTREACHED();
       break;
   }
 }
 
 void InfoBarContainer::ModeChanged(const chrome::search::Mode& old_mode,
                                    const chrome::search::Mode& new_mode) {

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

I'm not terribly thrilled by having three different functions which all do
similar-but-slightly-different-looking things to show and hide infobars in
response to various different instant signals.  It was already suboptimal with
two handlers, now it's worse.

Can we refactor this so that the infobar controller has a single "hide/show"
access point, with no knowledge of instant, and the instant system (e.g. the
instant controller) is responsible for calling this at the right times?

Even if we have to put some knowledge of instant into this class, we can at
least clean things up better.  I notice all three functions either hide or show
infobars, and they all use the same code to do each, always guarded by
conditional checks of |infobars_shown_| (possibly plus other things).  This
suggests that we ought to be able to at least  streamline these into calls to
some common core.  (But I'd still prefer for that core to be the only thing in
this file.)

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

+1

Kuan, I think we may be able to collapse these three.  Something like:
  1. Only have the single |ModeChanged| method
  2. Fold |visible| the |Mode| data structure, calling it |top_bar_visible|
maybe.

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

i've consolidated all 3 notifications into 1: ModelChanged.  however, as per our
offline discussion, i've kept |top_bars_visible_| as part of |SearchModel|, not
|Mode|, 'cos visibility needs to be handled different from |Mode::mode| and
|Mode::origin|.

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

i've consolidated all 3 different functions into just 1.

i decided not to have BrowserInstantController call InfoBarContainer to
show/hide bars, because closing the path from BrowserInstantcontroller to
InfoBarContainer requires either:
1) adding access to InfoBarContainer in base BrowserWindow and exposing a
show/hide method in InfoBarContainer, so that BrowserInstantController can call
them 
2) adding an InstantControllerObserver class that BrowsrInstantController fires
notifications to, with Browser and InfoBarContainer implementing the observer to
handle the notifications.

i feel that since we already have SearchModelObserver, having Browser and
InfobarContainer implement this observer is cleaner than creating a whole new
InstantControllerObserver that they still need to implement.  i think this is
also better than exposing InfoBarContainer in base BrowserWindow class.

-  // Hide infobars when showing Instant Extended suggestions.
-  if (new_mode.is_search_suggestions()) {
-    // If suggestions are being shown on a |DEFAULT| page, delay the hiding
-    // until notification that Instant overlay is ready is received via
-    // OverlayStateChanged(); this prevents jankiness caused by infobars hiding
-    // followed by suggestions appearing.
-    if (new_mode.is_origin_default())
-      return;
-    HideAllInfoBars();
-    OnInfoBarStateChanged(false);
-  } else {
+  // If new mode is |SEARCH_SUGGESTIONS| or |SEARCH_RESULTS|, don't update
+  // infobars' states now:
+  // - for |DEFAULT| page, wait for instant overlay to show or hide, via
+  //   OverlayStateChanged().
+  // - for |NTP| and |SERP| pages, wait for SearchBox API callback via
+  //   TopBarsVisibilityChanged().
+  // For other mode transitions, show infobars now.
+  if (new_mode.is_search())
+    return;
+  if (!infobars_shown_) {
     ChangeInfoBarService(infobar_service_);
     infobars_shown_time_ = base::TimeTicks::Now();
   }
 }
 
-void InfoBarContainer::OverlayStateChanged(const InstantOverlayModel& model) {
-  // If suggestions are being shown on a |DEFAULT| page, hide the infobars now.
-  // See comments for ModeChanged() for explanation.
-  if (model.mode().is_search_suggestions() &&
-      model.mode().is_origin_default()) {
+void InfoBarContainer::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 && !infobars_shown_) {
+    ChangeInfoBarService(infobar_service_);
+    infobars_shown_time_ = base::TimeTicks::Now();
+  } else if (!visible && infobars_shown_) {
     HideAllInfoBars();
     OnInfoBarStateChanged(false);
   }
 }
 
+void InfoBarContainer::OverlayStateChanged(const InstantOverlayModel& model) {
+  // This is invoked by a platform-specific implementation of
+  // |InstantOverlayController| to update infobars' states according to instant
+  // overlay state.  Only handle |DEFAULT| pages for this callback.
+  if (!model.mode().is_origin_default())
+    return;
+  if (model.mode().is_search_suggestions()) {
+    // Hide infobars if they're shown.
+    if (infobars_shown_) {
+      HideAllInfoBars();
+      OnInfoBarStateChanged(false);
+    }
+  } else if (!infobars_shown_) {
+    // Show hidden infobars.
+    ChangeInfoBarService(infobar_service_);
+    infobars_shown_time_ = base::TimeTicks::Now();
+  }
+}
+
 size_t InfoBarContainer::HideInfoBar(InfoBarDelegate* delegate,
                                      bool use_animation) {
   bool should_animate = use_animation &&
       ((base::TimeTicks::Now() - infobars_shown_time_) >
           base::TimeDelta::FromMilliseconds(50));
 
   // Search for the infobar associated with |delegate|.  We cannot search for
   // |delegate| in |tab_helper_|, because an InfoBar remains alive until its
   // close animation completes, while the delegate is removed from the tab
   // immediately.
@@ skipping 56 matching lines @@
     return InfoBar::kDefaultArrowTargetHeight;
   // When the first infobar is animating closed, we animate the second infobar's
   // arrow target height from the default to the top target height.  Note that
   // the animation values here are going from 1.0 -> 0.0 as the top bar closes.
   return top_arrow_target_height_ + static_cast<int>(
       (InfoBar::kDefaultArrowTargetHeight - top_arrow_target_height_) *
           first_infobar_animation.GetCurrentValue());
 }
 
 #endif  // TOOLKIT_VIEWS || defined(TOOLKIT_GTK)