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

chrome/browser/infobars/infobar_container.h

 // Copyright (c) 2011 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.
 
 #ifndef CHROME_BROWSER_INFOBARS_INFOBAR_CONTAINER_H_
 #define CHROME_BROWSER_INFOBARS_INFOBAR_CONTAINER_H_
 
 #include <vector>
 
 #include "base/compiler_specific.h"
 #include "base/time.h"
+#include "chrome/browser/instant/instant_model_observer.h"
 #include "chrome/browser/ui/search/search_model_observer.h"
 #include "chrome/common/search_types.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "third_party/skia/include/core/SkColor.h"
 
 class InfoBar;
 class InfoBarDelegate;
 class InfoBarService;
+class InstantModel;
 
 namespace chrome {
 namespace search {
 class SearchModel;
 }
 }
 
 // InfoBarContainer is a cross-platform base class to handle the visibility-
 // related aspects of InfoBars.  While InfoBars own themselves, the
 // InfoBarContainer is responsible for telling particular InfoBars that they
 // should be hidden or visible.
 //
 // Platforms need to subclass this to implement a few platform-specific
 // functions, which are pure virtual here.
 //
-// This class also observes changes to the SearchModel mode. If the user changes
-// into suggestions mode, it hides all the infobars temporarily.  When the user
-// changes back out of suggestions mode, it reshows any infobars, and starts a
-// 50 ms window during which any attempts to re-hide any infobars are handled
-// without animation.  This prevents glitchy-looking behavior when the user
-// navigates following a mode change, which otherwise would re-show the infobars
-// only to instantly animate them closed.  The window is canceled if a tab
-// change occurs.
+// This class also observes changes to the SearchModel and InstantModel modes.
+// It hides infobars temporarily if the user changes into suggestions mode when:

[Peter Kasting, 2013/01/28 21:51:00]

Readers may not know what "suggestions mode" means.  Refer them to other places
that fill out the terms you use here.

[kuan, 2013/01/29 00:17:08]

Done.

+// - on a website page: when instant preview is ready;
+// - not on a website page: immediately; this scenario requires more complex

[Peter Kasting, 2013/01/28 21:51:00]

It's not clear what "not on a website page" means.  In some sense, all content
Chrome shows in the content area is from a website.  chrome:// pages are still
websites.  Could you use clearer language?

[kuan, 2013/01/29 00:17:08]

Done.

+//   synchronization with renderer and will be implemented as the next step;

[dhollowa, 2013/01/25 21:24:08]

nit: this sounds like a TODO and should be marked as such.

[Peter Kasting, 2013/01/28 21:51:00]

Yes, please don't discuss implementation plans in comments except as TODOs. 
Comments should describe what the code does now.

[kuan, 2013/01/29 00:17:08]

Done.

[kuan, 2013/01/29 00:17:08]

Done.

+//   for now, hiding is immediate.
+// When the user changes back out of suggestions mode, it reshows any infobars,
+// and starts a 50 ms window during which any attempts to re-hide any infobars
+// are handled without animation.  This prevents glitchy-looking behavior when
+// the user navigates following a mode change, which otherwise would re-show the
+// infobars only to instantly animate them closed.  The window is canceled if a

[dhollowa, 2013/01/25 21:24:08]

nit: "The *animation* is canceled..." ?

[kuan, 2013/01/29 00:17:08]

i didn't write this comment, but hopefully i've clarified it.

+// tab change occurs.
 class InfoBarContainer : public content::NotificationObserver,
-                         public chrome::search::SearchModelObserver  {
+                         public chrome::search::SearchModelObserver,
+                         public InstantModelObserver  {
  public:
   class Delegate {
    public:
     // The separator color may vary depending on where the container is hosted.
     virtual SkColor GetInfoBarSeparatorColor() const = 0;
 
     // The delegate is notified each time the infobar container changes height,
     // as well as when it stops animating.
     virtual void InfoBarContainerStateChanged(bool is_animating) = 0;
 
     // The delegate needs to tell us whether "unspoofable" arrows should be
     // drawn, and if so, at what |x| coordinate.  |x| may be NULL.
     virtual bool DrawInfoBarArrows(int* x) const = 0;
 
    protected:
     virtual ~Delegate();
   };
 
-  // |search_model| may be NULL if this class is used in a window that does not
-  // support Instant Extended.
+  // |search_model| and |instant_model| may be NULL if this class is used in a
+  // window that does not support Instant Extended.
   InfoBarContainer(Delegate* delegate,
-                   chrome::search::SearchModel* search_model);
+                   chrome::search::SearchModel* search_model,

[dhollowa, 2013/01/25 21:24:08]

The const for InstantModel is nice.  Could you please add mutable to the
SearchModel observer list and const to the Add/RemoveObservers.

This will provide symmetry here.  And at the risk of going const-crazy... 
Maybe?:

const SearchModel* const search_model_;
const InstantModel* const instant_model_;

[Peter Kasting, 2013/01/27 23:57:28]

It does not seem at all correct that you should be able to add and remove
observers on a pointer-to-const.  That is not an internal implementation detail
that should be mutable.

If the InstantModel allows this, I claim that's a bug and the API should be
fixed.

[dhollowa, 2013/01/28 16:43:06]

That's a design choice really.  What I like about making the observer list
mutable is that it allows situations like this where view classes can take a
const-model to observe changes, but are required to go through a controller
class to change the (real) model itself.  Only controller classes then, should
have a non-const-model ptr.

That said, I'm happy to leave this change out of this CL, especially if there's
controversy.

+                   const InstantModel* instant_model);
   virtual ~InfoBarContainer();
 
   // Changes the InfoBarService for which this container is showing
   // infobars.  This will remove all current infobars from the container, add
   // the infobars from |infobar_service|, and show them all.  |infobar_service|
   // may be NULL.
   void ChangeInfoBarService(InfoBarService* infobar_service);
 
   // Returns the amount by which to overlap the toolbar above, and, when
   // |total_height| is non-NULL, set it to the height of the InfoBarContainer
@@ skipping 41 matching lines @@
 
   // content::NotificationObserver:
   virtual void Observe(int type,
                        const content::NotificationSource& source,
                        const content::NotificationDetails& details) OVERRIDE;
 
   // chrome::search::SearchModelObserver:
   virtual void ModeChanged(const chrome::search::Mode& old_mode,
                            const chrome::search::Mode& new_mode) OVERRIDE;
 
+  // InstantModelObserver:
+  virtual void PreviewStateChanged(const InstantModel& model) OVERRIDE;
+
   // Hides an InfoBar for the specified delegate, in response to a notification
   // from the selected InfoBarService.  The InfoBar's disappearance will be
   // animated if |use_animation| is true and it has been more than 50ms since
   // infobars were reshown due to an Instant Extended mode change. The InfoBar
   // will call back to RemoveInfoBar() to remove itself once it's hidden (which
   // may mean synchronously).  Returns the position within |infobars_| the
   // infobar was previously at.
   size_t HideInfoBar(InfoBarDelegate* delegate, bool use_animation);
 
   // Hides all infobars in this container without animation.
@@ skipping 19 matching lines @@
   InfoBars infobars_;
 
   // Tracks the most recent time infobars were re-shown after being hidden due
   // to Instant Extended's ModeChanged.
   base::TimeTicks infobars_shown_time_;
 
   // Tracks which search mode is active, as well as mode changes, for Instant
   // Extended.
   chrome::search::SearchModel* search_model_;
 
+  // Tracks state of instant preview for Instant Extended.
+  const InstantModel* instant_model_;  // Weak.
+
   // Calculated in SetMaxTopArrowHeight().
   int top_arrow_target_height_;
 
   DISALLOW_COPY_AND_ASSIGN(InfoBarContainer);
 };
 
 #endif  // CHROME_BROWSER_INFOBARS_INFOBAR_CONTAINER_H_