Upstream rewrite of Instant.

chrome/browser/instant/instant_controller.h

Index: chrome/browser/instant/instant_controller.h
diff --git a/chrome/browser/instant/instant_controller.h b/chrome/browser/instant/instant_controller.h
index 8f2a84442104315411d9ad84ecf5e932b93cac54..98537ad1a22d230df2164fd46640c245076f9186 100644
--- a/chrome/browser/instant/instant_controller.h
+++ b/chrome/browser/instant/instant_controller.h
@@ -5,45 +5,41 @@
 #ifndef CHROME_BROWSER_INSTANT_INSTANT_CONTROLLER_H_
 #define CHROME_BROWSER_INSTANT_INSTANT_CONTROLLER_H_
 
-#include <set>
-#include <string>
+#include <map>
+#include <vector>
 
 #include "base/basictypes.h"
 #include "base/memory/scoped_ptr.h"
-#include "base/memory/scoped_vector.h"
-#include "base/memory/weak_ptr.h"
 #include "base/string16.h"
+#include "base/timer.h"
 #include "chrome/browser/instant/instant_commit_type.h"
 #include "chrome/browser/instant/instant_loader_delegate.h"
-#include "chrome/browser/search_engines/template_url_id.h"
 #include "chrome/common/instant_types.h"
-#include "content/public/common/page_transition_types.h"
-#include "googleurl/src/gurl.h"
-#include "ui/gfx/native_widget_types.h"
 #include "ui/gfx/rect.h"
 
 struct AutocompleteMatch;
+class GURL;
 class InstantControllerDelegate;
 class InstantLoader;
-class InstantTest;
 class PrefService;
 class Profile;
 class TabContents;
 class TemplateURL;
 
-// InstantController maintains a WebContents that is intended to give a preview
-// of a URL. InstantController is owned by Browser.
+// |InstantController| maintains a WebContents that is intended to give a

[sky, 2012/07/19 17:55:19]

Use |s for members/arguments, () for functions and generally nothing for names.

[Jered, 2012/07/19 22:01:33]

Done.

+// preview of search results. InstantController is owned by Browser.
 //
-// At any time the WebContents maintained by InstantController may be destroyed
-// by way of |DestroyPreviewContents|, which results in |HideInstant| being
-// invoked on the delegate. Similarly the preview may be committed at any time
-// by invoking |CommitCurrentPreview|, which results in |CommitInstant|
-// being invoked on the delegate. Also see |PrepareForCommit| below.
+// At any time the WebContents maintained by InstantController may be hidden
+// from view by way of |Hide|, which may result in |HideInstant| being invoked
+// on the delegate. Similarly the preview may be committed at any time by
+// invoking |CommitCurrentPreview|, which results in |CommitInstant| being
+// invoked on the delegate.
 class InstantController : public InstantLoaderDelegate {
  public:
-  // Amount of time to wait before starting the instant animation.
+  // Amount of time to wait before starting the animation for suggested text.
   static const int kAutoCommitPauseTimeMS = 1000;
-  // Duration of the instant animation in which the colors change.
+
+  // Duration of the suggested text animation in which the colors change.
   static const int kAutoCommitFadeInTimeMS = 300;
 
   // InstantController may operate in one of these modes:
@@ -64,211 +60,164 @@ class InstantController : public InstantLoaderDelegate {
   InstantController(InstantControllerDelegate* delegate, Mode mode);
   virtual ~InstantController();
 
-  // Registers instant related preferences.
+  // Registers Instant related preferences.
   static void RegisterUserPrefs(PrefService* prefs);
 
-  // Records instant metrics.
+  // Records Instant metrics.
   static void RecordMetrics(Profile* profile);
 
-  // Returns true if instant is enabled in the given |profile|'s preferences.
+  // Returns true if Instant is enabled in the given |profile|'s preferences.
   static bool IsEnabled(Profile* profile);
 
-  // Enables instant.
+  // Enables Instant.
   static void Enable(Profile* profile);
 
-  // Disables instant.
+  // Disables Instant.
   static void Disable(Profile* profile);
 
-  // Invoked as the user types in the omnibox with the url to navigate to. If
-  // the url is valid and a preview WebContents has not been created, it is
-  // created. If |verbatim| is true search results are shown for |user_text|
-  // rather than the best guess as to what the search thought the user meant.
-  // |verbatim| only matters if the AutocompleteMatch is for a search engine
-  // that supports instant. Returns true if the attempt to update does not
-  // result in the preview WebContents being destroyed.
+  // Invoked as the user types into the omnibox. |user_text| is what the user
+  // has typed. |suggested_text| is the current inline autocomplete text. It
+  // may be replaced by Instant's autocomplete suggestion, if any. If |verbatim|
+  // is true, search results are shown for |user_text| rather than the best
+  // guess as to what Instant thinks the user means. Returns true if the update
+  // is processed by Instant (i.e., if |match| is a search rather than a URL).
   bool Update(const AutocompleteMatch& match,
               const string16& user_text,
               bool verbatim,
-              string16* suggested_text);
+              string16* suggested_text,
+              InstantCompleteBehavior* complete_behavior);
 
-  // Sets the bounds of the omnibox (in screen coordinates). The bounds are
-  // remembered until the preview is committed or destroyed. This is only used
-  // when showing results for a search provider that supports instant.
+  // Sets the bounds of the omnibox dropdown, in screen coordinates.
   void SetOmniboxBounds(const gfx::Rect& bounds);
 
-  // Notifies the delegate to hide the preview and destroys the preview
-  // WebContents. Does nothing if the preview WebContents has not been created.
-  void DestroyPreviewContents();
+  // The preview TabContents. May be NULL if |ReleasePreviewContents| has been
+  // called, with no subsequent successful call to |Update|. InstantController
+  // retains ownership of the object.
+  TabContents* GetPreviewContents() const;
 
-  // Notifies the delegate to hide the preview but leaves it around in hopes it
-  // can be subsequently used. The preview will not be used until Update() (with
-  // valid parameters) is invoked.
+  // Hides the preview, but doesn't destroy it, in hopes it can be subsequently
+  // reused. The preview will not be used until a call to |Update| succeeds.
   void Hide();
 
-  // Returns true if we're showing the last URL passed to |Update|. If this is
-  // false a commit does not result in committing the last url passed to update.
-  // A return value of false happens if we're in the process of determining if
-  // the page supports instant.
+  // Returns true if we're showing the last text passed to |Update|. If this is
+  // false, a commit does not result in committing the most recent |Update|.
   bool IsCurrent() const;
 
-  // Returns true if the caller should proceed with committing the preview. A
-  // return value of false means that there is no valid preview to commit. This
-  // is used by Browser, when the user presses <Enter>, to decide whether to
-  // load the omnibox contents through Instant or otherwise. This is needed
-  // because calls to |Update| don't necessarily result in a preview being
-  // shown, such as in the HIDDEN and SILENT modes.
-  bool PrepareForCommit();
-
-  // Invoked when the user does some gesture that should trigger making the
-  // current previewed page the permanent page.  Returns the TabContents that
-  // contains the committed preview.
+  // Unconditionally commits the preview. Returns the TabContents that contains
+  // the committed preview.
   TabContents* CommitCurrentPreview(InstantCommitType type);
 
-  // Accepts the currently showing instant preview, if any, and returns true.
-  // Returns false if there is no instant preview showing.
-  bool CommitIfCurrent();
-
-  // Sets InstantController so that when the mouse is released the preview is
-  // committed.
-  void SetCommitOnMouseUp();
-
-  bool commit_on_mouse_up() const { return commit_on_mouse_up_; }
-
-  // Returns true if the mouse is down as the result of activating the preview
-  // content.
-  bool IsMouseDownFromActivate();
+  // Releases the preview WebContents passing ownership to the caller. This is
+  // intended to be called when the preview WebContents is committed. This does
+  // not notify the delegate.
+  TabContents* ReleasePreviewContents(InstantCommitType type)
+      WARN_UNUSED_RESULT;

[sky, 2012/07/19 17:55:19]

Wrap 'InstantCommitType type' so that WARN_UNUSED_RESULT isn't dangling (similar
to how const is treated).

[Jered, 2012/07/19 22:01:33]

Done.

 
-  // The autocomplete edit that was initiating the current instant session has
+  // The autocomplete edit that was initiating the current Instant session has
   // lost focus. Commit or discard the preview accordingly.
-  void OnAutocompleteLostFocus(gfx::NativeView view_gaining_focus);
+  void OnAutocompleteLostFocus();
 
-  // The autocomplete edit has gained focus. Preload the instant URL of the
+  // The autocomplete edit has gained focus. Preload the Instant URL of the
   // default search engine, in anticipation of the user typing a query.
   void OnAutocompleteGotFocus();
 
-  // Releases the preview WebContents passing ownership to the caller. This is
-  // intended to be called when the preview WebContents is committed. This does
-  // not notify the delegate. |tab_contents| is the underlying tab onto which
-  // the preview will be committed. It can be NULL when the underlying tab is
-  // irrelevant, for example when |type| is INSTANT_COMMIT_DESTROY.
-  // WARNING: be sure and invoke CompleteRelease after adding the returned
-  // WebContents to a tabstrip.
-  TabContents* ReleasePreviewContents(InstantCommitType type,
-                                      TabContents* tab_contents);
-
-  // Does cleanup after the preview contents has been added to the tabstrip.
-  // Invoke this if you explicitly invoke ReleasePreviewContents.
-  void CompleteRelease(TabContents* tab);
-
-  // The preview TabContents; may be null.
-  TabContents* GetPreviewContents() const;
+  // Returns whether the preview will be committed when the mouse is released.
+  bool commit_on_mouse_up() const;
 
-  // Returns true if the preview TabContents is ready to be displayed. In
-  // some situations this may return false yet GetPreviewContents() returns
-  // non-NULL.
-  bool is_displayable() const { return is_displayable_; }
-
-  // Returns the transition type of the last AutocompleteMatch passed to Update.
-  content::PageTransition last_transition_type() const {
-    return last_transition_type_;
-  }
-
-  // InstantLoaderDelegate
-  virtual void InstantStatusChanged(InstantLoader* loader) OVERRIDE;
-  virtual void SetSuggestedTextFor(InstantLoader* loader,
-                                   const string16& text,
-                                   InstantCompleteBehavior behavior) OVERRIDE;
-  virtual gfx::Rect GetInstantBounds() OVERRIDE;
-  virtual bool ShouldCommitInstantOnMouseUp() OVERRIDE;
+  // InstantLoaderDelegate:
+  virtual void SetSuggestions(
+      InstantLoader* loader,
+      const std::vector<std::string>& suggestions,
+      InstantCompleteBehavior behavior) OVERRIDE;
   virtual void CommitInstantLoader(InstantLoader* loader) OVERRIDE;
-  virtual void InstantLoaderDoesntSupportInstant(
-      InstantLoader* loader) OVERRIDE;
-  virtual void AddToBlacklist(InstantLoader* loader,
-                              const GURL& url) OVERRIDE;
+  virtual void InstantLoaderPreviewLoaded(InstantLoader* loader) OVERRIDE;
+  virtual void InstantSupportDetermined(InstantLoader* loader,
+                                        bool supports_instant) OVERRIDE;
   virtual void SwappedTabContents(InstantLoader* loader) OVERRIDE;
-  virtual void InstantLoaderContentsFocused() OVERRIDE;
+  virtual void InstantLoaderContentsFocused(InstantLoader* loader) OVERRIDE;
 
  private:
   friend class InstantTest;
 
-  typedef std::set<std::string> HostBlacklist;
-
-  // Updates |is_displayable_| and if necessary notifies the delegate.
-  void UpdateIsDisplayable();
+  // Creates a new loader if necessary (for example, if the |instant_id| has
+  // changed since the last time we created the loader).
+  void ResetLoader(const string16& instant_id,
+                   const GURL& instant_url,
+                   const TabContents* tab_contents);
 
-  // Updates InstantLoaderManager and its current InstantLoader. This is invoked
-  // internally from Update.
-  void UpdateLoader(TabContents* tab_contents,
-                    const TemplateURL* template_url,
-                    const GURL& url,
-                    content::PageTransition transition_type,
-                    const string16& user_text,
-                    bool verbatim,
-                    string16* suggested_text);
+  // Destroys the |loader_| and its preview contents.
+  void DeleteLoader();
 
-  // Returns true if |template_url| is a valid TemplateURL for use by instant.
-  bool IsValidInstantTemplateURL(const TemplateURL* template_url);
+  // Counterpart to |Hide|. Asks the |delegate_| to display the preview.
+  void Show();
 
-  // Marks the loader as not supporting instant.
-  void BlacklistFromInstant();
+  // Send the omnibox dropdown bounds to the page.
+  void SendBoundsToPage();
 
-  // Returns true if the specified id has been blacklisted from supporting
-  // instant.
-  bool IsBlacklistedFromInstant(TemplateURLID id);
+  // If |template_url| is a valid TemplateURL for use with Instant, fills in

[sky, 2012/07/19 17:55:19]

Description should include kInstantURL.

[Jered, 2012/07/19 22:01:33]

Done.

+  // |instant_id| and |instant_url| and returns true; returns false otherwise.
+  bool GetInstantURL(const TemplateURL* template_url,
+                     string16* instant_id,
+                     GURL* instant_url) const;
 
-  // Clears the set of search engines blacklisted.
-  void ClearBlacklist();
+  // Returns true if the preview is no longer relevant, say because the last
+  // |Update| was for a URL and not a search query, or the user switched tabs.
+  bool IsOutOfDate() const;
 
-  // Deletes |loader| after a delay. At the time we determine a site doesn't
-  // want to participate in instant we can't destroy the loader (because
-  // destroying the loader destroys the WebContents and the WebContents is on
-  // the stack). Instead we place the loader in |loaders_to_destroy_| and
-  // schedule a task.
-  void ScheduleDestroy(InstantLoader* loader);
+  InstantControllerDelegate* const delegate_;
 
-  // Destroys all loaders scheduled for destruction in |ScheduleForDestroy|.
-  void DestroyLoaders();
+  scoped_ptr<InstantLoader> loader_;
 
-  InstantControllerDelegate* delegate_;
+  // See the enum description above.
+  const Mode mode_;
 
-  // True if |loader_| is ready to be displayed.
-  bool is_displayable_;
+  // The active tab at the time of the last |Update|. Used by |IsOutOfDate| to
+  // know whether the user switched tabs. ***NEVER DEREFERENCE THIS POINTER.***
+  // It may be a dangling pointer to a freed object. Should only be used for
+  // pointer comparisons.
+  const void* last_tab_contents_;
 
-  // Set to true in Hide() and false in Update(). Used when we persist the
-  // |loader_|, but it isn't up to date.
-  bool is_out_of_date_;
+  // The most recent full omnibox query text known to us. If this is empty, it
+  // could also mean that the omnibox text was a URL (or something else that
+  // we shouldn't be processing).
+  string16 last_full_text_;
 
-  scoped_ptr<InstantLoader> loader_;
+  // The most recent user_text passed to |Update|.
+  string16 last_user_text_;
 
-  // See description above setter.
-  gfx::Rect omnibox_bounds_;
+  // The most recent verbatim passed to |Update|.
+  bool last_verbatim_;
 
-  // See description above CommitOnMouseUp.
-  bool commit_on_mouse_up_;
+  // The most recent suggestion received from the page, minus any prefix that
+  // the user has typed.
+  string16 last_suggestion_;
 
-  // See description above getter.
-  content::PageTransition last_transition_type_;
+  // The most recent autocomplete behavior for |last_suggestion_|.
+  InstantCompleteBehavior last_complete_behavior_;
 
-  // The IDs of any search engines that don't support instant. We assume all
-  // search engines support instant, but if we determine an engine doesn't
-  // support instant it is added to this list. The list is cleared out on every
-  // reset/commit.
-  std::set<TemplateURLID> blacklisted_ids_;
+  // True if the preview is currently being displayed. Guaranteed to be false
+  // if |IsOutOfDate| is true.
+  bool is_showing_;
 
-  // Used by ScheduleForDestroy; see it for details.
-  base::WeakPtrFactory<InstantController> weak_factory_;
+  // True if we've received a response from the loader for the last |Update|,
+  // thus indicating that the page is ready to be shown.
+  bool loader_ready_;

[sky, 2012/07/19 17:55:19]

This name is confusing. Its not whether the loader is ready, rather the loader
has processed the last request. Maybe loader_processed_last_update_.

[Jered, 2012/07/19 22:01:33]

Done.

 
-  // List of InstantLoaders to destroy. See ScheduleForDestroy for details.
-  ScopedVector<InstantLoader> loaders_to_destroy_;
+  // Current omnibox bounds.
+  gfx::Rect omnibox_bounds_;
 
-  // The URL of the most recent match passed to |Update|.
-  GURL last_url_;
+  // Last bounds passed to the page.
+  gfx::Rect last_omnibox_bounds_;
 
-  // The most recent user_text passed to |Update|.
-  string16 last_user_text_;
+  // Timer used to update the bounds of the omnibox.
+  base::OneShotTimer<InstantController> update_bounds_timer_;
 
-  // See the enum description above.
-  const Mode mode_;
+  // For each key K => value N, the map says that we found that the search
+  // engine identified by keyword K didn't support the Instant API in each of
+  // the last N times that we loaded it. If an ID isn't present in the map at
+  // all or has a value 0, it means that search engine supports the Instant API
+  // (or that we assume it does, since we haven't determined it doesn't).
+  std::map<string16, int> blacklisted_ids_;

[sky, 2012/07/19 17:55:19]

This assumes that once it sees a keyword the TemplateURL associated with it
never changes. That is wrong. There is nothing stopping the user from
manipulating the set of keywords in various ways during the session.

[Jered, 2012/07/19 22:01:33]

What is the proper id to use? Does this need to map from URLs?

[sky, 2012/07/19 23:43:49]

The id from the TemplateURL never changes. That said, the user can change the
TemplateURL, but not the instant_url (we don't surface ui for that).

[Jered, 2012/07/20 16:10:18]

How about just using the instant url?

[Jered, 2012/07/20 21:21:26]

Done.

 
   DISALLOW_COPY_AND_ASSIGN(InstantController);
 };