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

chrome/browser/infobars/infobar_container.h

Index: chrome/browser/infobars/infobar_container.h
diff --git a/chrome/browser/infobars/infobar_container.h b/chrome/browser/infobars/infobar_container.h
index 508bf76dd23826c5d8ecda213526b6d07069a2ea..08c28cf4320c8ff0acc45016bb0f1595779d586f 100644
--- a/chrome/browser/infobars/infobar_container.h
+++ b/chrome/browser/infobars/infobar_container.h
@@ -9,6 +9,7 @@
 
 #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"
@@ -18,6 +19,7 @@
 class InfoBar;
 class InfoBarDelegate;
 class InfoBarService;
+class InstantModel;
 
 namespace chrome {
 namespace search {
@@ -33,16 +35,21 @@ class SearchModel;
 // 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:
@@ -61,10 +68,11 @@ class InfoBarContainer : public content::NotificationObserver,
     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
@@ -126,6 +134,9 @@ class InfoBarContainer : public content::NotificationObserver,
   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
@@ -165,6 +176,9 @@ class InfoBarContainer : public content::NotificationObserver,
   // 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_;