InstantExtended: Committed NTP

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 5c4d01c09014faab7f61288380bbc2758e2b0388..e8fb460ffd9b377137a0e664489836651cc5d145 100644
--- a/chrome/browser/instant/instant_controller.h
+++ b/chrome/browser/instant/instant_controller.h
@@ -19,6 +19,7 @@
 #include "base/timer.h"
 #include "chrome/browser/instant/instant_commit_type.h"
 #include "chrome/browser/instant/instant_model.h"
+#include "chrome/browser/instant/instant_page.h"
 #include "chrome/browser/ui/omnibox/omnibox_edit_model.h"
 #include "chrome/common/instant_types.h"
 #include "chrome/common/search_types.h"
@@ -29,7 +30,8 @@
 
 struct AutocompleteMatch;
 class AutocompleteProvider;
-class InstantLoader;
+class InstantNTP;
+class InstantOverlay;
 class InstantTab;
 class TemplateURL;
 
@@ -45,10 +47,12 @@ class WebContents;
 #define LOG_INSTANT_DEBUG_EVENT(controller, message) \
     controller->LogDebugEvent(message)
 
-// InstantController maintains a WebContents that is intended to give a preview
-// of search suggestions and results. InstantController is owned by Browser via
-// BrowserInstantController.
-class InstantController {
+// InstantController drives Chrome Instant, i.e., the browser implementation of
+// the Embedded Search API (see http://dev.chromium.org/embeddedsearch). It
+// maintains a WebContents used to show search suggestions and results in an

[dhollowa, 2013/01/29 02:37:53]

The wording makes it sound like there are two web-contents of interest where
there is really three.  Could this be reworded so that the core job of
coordinating the three "pages" is described?  And maybe how each page is backed
by a web-contents (owned by Instant or not).

[samarth, 2013/01/29 05:42:01]

Moved some of the comments from below into here.  Better?

+// overlay and a WebContents used for the New Tab Page. InstantController is
+// owned by Browser via BrowserInstantController.
+class InstantController : public InstantPage::Delegate {
  public:
   // The URL for the local omnibox popup.
   static const char* kLocalOmniboxPopupURL;
@@ -78,6 +82,10 @@ class InstantController {
               bool escape_pressed,
               bool is_keyword_search);
 
+  // Releases and returns the NTP WebContents. May be NULL. Loads a new
+  // WebContents for the NTP.
+  scoped_ptr<content::WebContents> ReleaseNTPContents() WARN_UNUSED_RESULT;
+
   // Sets the bounds of the omnibox popup, in screen coordinates.
   void SetPopupBounds(const gfx::Rect& bounds);
 
@@ -111,7 +119,7 @@ class InstantController {
                            gfx::NativeView view_gaining_focus);
 
   // The search mode in the active tab has changed. Pass the message down to
-  // the loader which will notify the renderer. Create |instant_tab_| if the
+  // the overlay which will notify the renderer. Create |instant_tab_| if the
   // |new_mode| reflects an Instant search results page.
   void SearchModeChanged(const chrome::search::Mode& old_mode,
                          const chrome::search::Mode& new_mode);
@@ -132,51 +140,16 @@ class InstantController {
   // The theme area height has changed. Pass the message to the preview page.
   void ThemeAreaHeightChanged(int height);
 
-  // Returns the transition type of the last AutocompleteMatch passed to Update.
-  content::PageTransition last_transition_type() const {
-    return last_transition_type_;
-  }
-
-  const InstantModel* model() const { return &model_; }
-
-  // Invoked by the page when it has suggested text.
-  void SetSuggestions(const content::WebContents* contents,
-                      const std::vector<InstantSuggestion>& suggestions);
-
-  // Invoked by the page when its support for the Instant API is determined.
-  void InstantSupportDetermined(const content::WebContents* contents,
-                                bool supports_instant);
-
-  // Invoked by InstantLoader to request that the preview be shown.
-  void ShowInstantPreview(InstantShownReason reason,
-                          int height,
-                          InstantSizeUnits units);
-
-  // Invoked by InstantLoader to request the browser to start capturing user key
-  // strokes.
-  void StartCapturingKeyStrokes();
-
-  // Invoked by InstantLoader to request the browser to stop capturing user key
-  // strokes.
-  void StopCapturingKeyStrokes();
-
-  // Invoked by InstantLoader when it has swapped a different WebContents into
-  // the preview, usually because a prerendered page was navigated to.
-  void SwappedWebContents();
-
-  // Invoked by InstantLoader when the preview gains focus, usually due to the
-  // user clicking on it.
-  void InstantLoaderContentsFocused();
-
-  // Invoked by the InstantLoader when its RenderView crashes.
-  void InstantLoaderRenderViewGone();
+  // Called when someone else swapped in a different contents in the |overlay_|.
+  void SwappedOverlayContents();

[dhollowa, 2013/01/29 02:37:53]

To keep terminology straight, how about renaming
InstantOverlay::ReplaceAndReleaseContents to InstantOverlay::SwapContents?

[samarth, 2013/01/29 05:42:01]

Done.

 
-  // Invoked by InstantLoader when the instant page is about to navigate.
-  void InstantLoaderAboutToNavigateMainFrame(const GURL& url);
+  // Called when contents for |overlay_| received focus.
+  void FocusedOverlayContents();
 
-  // Invoked by the InstantLoader when the instant page wants to navigate to
-  // the speicfied URL.
-  void NavigateToURL(const GURL& url, content::PageTransition transition);
+  // Called when the |overlay_| might be stale. If it's actually stale, and the
+  // omnibox doesn't have focus, and the preview isn't showing, the |overlay_|
+  // is deleted and recreated. Else the refresh is skipped.
+  void HandleStaleOverlay();

[dhollowa, 2013/01/29 02:37:53]

This is not always about being stale (in the temporal sense) it may be due to
focus or visibility changes.  So how about we rename this OverlayMayBeChanged()?
 (Better names welcome...).

[samarth, 2013/01/29 05:42:01]

How about ReloadOverlayIfStale?

 
   // Adds a new event to |debug_events_| and also DVLOG's it. Ensures that
   // |debug_events_| doesn't get too large.
@@ -187,33 +160,68 @@ class InstantController {
     return debug_events_;
   }
 
+  // Returns the transition type of the last AutocompleteMatch passed to Update.
+  content::PageTransition last_transition_type() const {
+    return last_transition_type_;
+  }
+
+  const InstantModel* model() const { return &model_; }
+
  private:
   FRIEND_TEST_ALL_PREFIXES(InstantTest, OmniboxFocusLoadsInstant);
   FRIEND_TEST_ALL_PREFIXES(InstantTest, SetWithTemplateURL);
   FRIEND_TEST_ALL_PREFIXES(InstantTest, NonInstantSearchProvider);
-  FRIEND_TEST_ALL_PREFIXES(InstantTest, InstantLoaderRefresh);
+  FRIEND_TEST_ALL_PREFIXES(InstantTest, InstantOverlayRefresh);
   FRIEND_TEST_ALL_PREFIXES(InstantExtendedTest, ExtendedModeIsOn);
   FRIEND_TEST_ALL_PREFIXES(InstantExtendedTest, OmniboxFocusLoadsInstant);
+  FRIEND_TEST_ALL_PREFIXES(InstantExtendedTest, NTPIsPreloaded);
+  FRIEND_TEST_ALL_PREFIXES(InstantExtendedTest, PreloadedNTPIsUsedInNewTab);
+  FRIEND_TEST_ALL_PREFIXES(InstantExtendedTest, PreloadedNTPIsUsedInSameTab);
+  FRIEND_TEST_ALL_PREFIXES(InstantExtendedTest, ProcessIsolation);
+
+  // Overridden from InstantPage::Delegate:
+  virtual void InstantSupportDetermined(
+      const content::WebContents* contents,
+      bool supports_instant) OVERRIDE;
+  virtual void InstantPageRenderViewGone(
+      const content::WebContents* contents) OVERRIDE;
+  virtual void InstantPageAboutToNavigateMainFrame(
+      const content::WebContents* contents,
+      const GURL& url) OVERRIDE;
+  virtual void SetSuggestions(
+      const content::WebContents* contents,
+      const std::vector<InstantSuggestion>& suggestions) OVERRIDE;
+  virtual void ShowInstantPreview(
+      const content::WebContents* contents,
+      InstantShownReason reason,
+      int height,
+      InstantSizeUnits units) OVERRIDE;
+  virtual void StartCapturingKeyStrokes(
+      const content::WebContents* contents) OVERRIDE;
+  virtual void StopCapturingKeyStrokes(content::WebContents* contents) OVERRIDE;
+  virtual void NavigateToURL(
+      const content::WebContents* contents,
+      const GURL& url,
+      content::PageTransition transition) OVERRIDE;
 
   // Helper for OmniboxFocusChanged. Commit or discard the preview.
   void OmniboxLostFocus(gfx::NativeView view_gaining_focus);
 
-  // Ensures that |loader_| uses the Instant URL returned by GetInstantURL(),
-  // creating a new loader if necessary. In extended mode, will fallback to
+  // Creates a new NTP, using the instant_url property of the default
+  // TemplateURL.
+  void ResetNTP();
+
+  // Ensures that |overlay_| uses the Instant URL returned by GetInstantURL(),
+  // creating a new overlay if necessary. In extended mode, will fallback to
   // using the kLocalOmniboxPopupURL as the Instant URL in case GetInstantURL()
   // returns false. Returns true if an Instant URL could be determined.
-  bool EnsureLoaderIsCurrent();
-
-  // Recreates the |loader_| with the input |instant_url|. The caller should
-  // ensure that the |loader_| is not already on the stack since it is deleted
-  // in this call.
-  void CreateLoader(const std::string& instant_url,
-                    const content::WebContents* active_tab);
+  bool EnsureOverlayIsCurrent();
 
-  // Called when the |loader_| might be stale. If it's actually stale, and the
-  // omnibox doesn't have focus, and the preview isn't showing, the |loader_| is
-  // deleted and recreated. Else the refresh is skipped.
-  void OnStaleLoader();
+  // Recreates the |overlay_| with |instant_url|. The caller should ensure that

[dhollowa, 2013/01/29 02:37:53]

nit: This comment is overly specific - it can just say that |overlay_| is
deleted.  There are many things the caller should not do with deleted objects. 
;-)

[samarth, 2013/01/29 05:42:01]

Done.

+  // the |overlay_| is not already on the stack since it is deleted in this
+  // call.
+  void CreateOverlay(const std::string& instant_url,
+                     const content::WebContents* active_tab);
 
   // If the active tab is an Instant search results page, sets |instant_tab_| to
   // point to it. Else, deletes any existing |instant_tab_|.
@@ -221,17 +229,17 @@ class InstantController {
 
   // Hide the preview. Also sends an onchange event (with blank query) to the
   // preview, telling it to clear out results for any old queries.
-  void HideLoader();
+  void HideOverlay();
 
-  // Like HideLoader(), but doesn't call OnStaleLoader(). Use HideLoader()
-  // unless you are going to call loader_.reset() yourself subsequently.
+  // Like HideOverlay(), but doesn't call OnStaleOverlay(). Use HideOverlay()
+  // unless you are going to call overlay_.reset() yourself subsequently.
   void HideInternal();
 
-  // Counterpart to HideLoader(). Asks the |browser_| to display the preview
-  // with the given |height|.
-  void ShowLoader(InstantShownReason reason,
-                  int height,
-                  InstantSizeUnits units);
+  // Counterpart to HideOverlay(). Asks the |browser_| to display the preview
+  // with the given |height| in |units|.
+  void ShowOverlay(InstantShownReason reason,
+                   int height,
+                   InstantSizeUnits units);
 
   // Send the omnibox popup bounds to the page.
   void SendPopupBoundsToPage();
@@ -246,9 +254,17 @@ class InstantController {
   //   - If the default Instant URL is present returns it.
   //
   // Returns true if a valid Instant URL could be found that is not blacklisted.
-  bool GetInstantURL(const content::WebContents* active_tab,
+  bool GetInstantURL(Profile* profile,

[dhollowa, 2013/01/29 02:37:53]

nit: This will fit on one line now.

[samarth, 2013/01/29 05:42:01]

Done.

                      std::string* instant_url) const;
 
+  // Adds the URL for the page to the blacklist. Deletes the contents held and
+  // recreates a new page.
+  void BlacklistAndResetOverlay();
+  void BlacklistAndResetNTP();
+
+  // Removes |url| from the blacklist.
+  void RemoveFromBlacklist(const std::string& url);
+
   chrome::BrowserInstantController* const browser_;
 
   // Whether the extended API and regular API are enabled. If both are false,
@@ -259,19 +275,29 @@ class InstantController {
   // If true, the instant URL is set to kLocalOmniboxPopupURL.
   const bool use_local_preview_only_;
 
-  // The state of the preview page, i.e., the page owned by |loader_|. Ignored
+  // The state of the preview page, i.e., the page owned by |overlay_|. Ignored
   // if |instant_tab_| is in use.
   InstantModel model_;
 
-  // The preview WebContents.
-  scoped_ptr<InstantLoader> loader_;
-
-  // A committed WebContents that supports Instant. If non-NULL, the |loader_|
-  // is guaranteed to be hidden and messages will be sent to this instead.
+  // InstantController maintains 3 instances of InstantPage in extended mode:
+  //   - |overlay_| is used to show search suggestions and results in an
+  //     overlay over a non-search page.
+  //   - |ntp_| is a preloaded search page that will be swapped-in the next
+  //     time the user navigates to the New Tab Page. It is never shown to the
+  //     user in an uncommitted state.
+  //   - |instant_tab_| is the currently active tab, if it supports the Instant
+  //     API.
+  // All three may be non-NULL. If |instant_tab_| is not NULL, then |overlay_|
+  // is guaranteed to be hidden and messages will be sent to |instant_tab_|
+  // instead.
+  //
+  // In non-extended mode, only |overlay_| is ever non-NULL.
+  scoped_ptr<InstantOverlay> overlay_;
+  scoped_ptr<InstantNTP> ntp_;
   scoped_ptr<InstantTab> instant_tab_;
 
   // The most recent full_text passed to Update(). If empty, we'll not accept
-  // search suggestions from |loader_| or |instant_tab_|.
+  // search suggestions from |overlay_| or |instant_tab_|.
   string16 last_omnibox_text_;
 
   // True if the last Update() had an inline autocompletion. Used only to make
@@ -314,9 +340,6 @@ class InstantController {
   // Timer used to update the bounds of the omnibox popup.
   base::OneShotTimer<InstantController> update_bounds_timer_;
 
-  // Timer used to ensure that the Instant page does not get too stale.
-  base::OneShotTimer<InstantController> stale_loader_timer_;
-
   // For each key K => value N, the map says that we found that the search
   // engine identified by Instant URL K didn't support the Instant API, or
   // caused RenderView crashes in each of the last N times that we loaded it.