InstantExtended: Committed NTP

chrome/browser/instant/instant_api_wrapper.h

Index: chrome/browser/instant/instant_api_wrapper.h
diff --git a/chrome/browser/instant/instant_client.h b/chrome/browser/instant/instant_api_wrapper.h
similarity index 58%
rename from chrome/browser/instant/instant_client.h
rename to chrome/browser/instant/instant_api_wrapper.h
index 0f448684e8fad500ab34034ad137af4481cd9227..16f07558e98997fe0d8a63dfa35b54796068624e 100644
--- a/chrome/browser/instant/instant_client.h
+++ b/chrome/browser/instant/instant_api_wrapper.h
@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef CHROME_BROWSER_INSTANT_INSTANT_CLIENT_H_
-#define CHROME_BROWSER_INSTANT_INSTANT_CLIENT_H_
+#ifndef CHROME_BROWSER_INSTANT_INSTANT_API_WRAPPER_H_
+#define CHROME_BROWSER_INSTANT_INSTANT_API_WRAPPER_H_
 
 #include <vector>
 
@@ -27,54 +27,96 @@ namespace gfx {
 class Rect;
 }
 
-// InstantClient is used to exchange messages between its delegate and a page
-// that supports the Instant API (http://dev.chromium.org/searchbox).
-class InstantClient : public content::WebContentsObserver {
+// InstantAPIWrapper is used for exchanging messages with a page that supports
+// the Instant/Embedded Search API (http://dev.chromium.org/embeddedsearch).
+//
+// The messages received by InstantAPIWrapper are sent to a class that
+// implements InstantAPIWrapper::Receiver, unless the message is intercepted and
+// cancelled by InstantAPIWrapper::Controller.
+class InstantAPIWrapper : public content::WebContentsObserver {

[dhollowa, 2013/01/14 19:59:39]

Not a fan of this name.  How about just InstantPage?  I agree with Jered, the
separate class InstantPage doesn't really do enough to warrant its existence. 
Best collapse {InstantAPIWrapper + InstantPage} => {InstantPage} pulling the
state/data-members in too.

[samarth, 2013/01/22 15:59:06]

Done.

  public:
-  // When InstantClient receives messages from the page, it calls the following
-  // methods on its delegate.
-  class Delegate {
+  // When InstantAPIWrapper receives a message, it first checks with its
+  // Controller to see whether it should process the message.
+  class Controller {

[Jered, 2013/01/14 18:22:34]

Calling this a controller feels a bit off. Some of the methods mutate state, but
most seem to be part of a Policy about whether to route messages. Perhaps this
can be made a Policy object?

[dhollowa, 2013/01/14 19:59:39]

+1.  And this should not be pure-virtual, there should be a default
implementation for each policy.

The methods in the Policy should not have prefix "On" as this is typically used
for IPC handlers.  A "Should" prefix would be better.

I also favor keeping these "policy" methods as part of the InstantAPIWrapper
class itself (as protected methods) and not as an interface with pointers
floating around.

[samarth, 2013/01/22 15:59:06]

Done.

    public:
+    // Returns true if the page is known to support the Instant API. This starts
+    // out false, and is set to true whenever we get any message from the page.
+    // Once true, it never becomes false (the page isn't expected to drop API
+    // support suddenly).
+    virtual bool supports_instant() const = 0;

[Jered, 2013/01/14 18:22:34]

Consider just implementing these in the Wrapper, since I expect all implementors
will need it.

[samarth, 2013/01/22 15:59:06]

Done.

+    virtual void set_supports_instant(bool supports_instant) = 0;
+
+    // Called when Update() is called on the API.
+    virtual void OnUpdate() = 0;

[Jered, 2013/01/14 18:22:34]

Can whoever is calling Update() do this?

[samarth, 2013/01/22 15:59:06]

Just overriding Update() now in InstantOverlay.  (I don't want to have
InstantController to have to remember to clear the history every time it wants
to call Update on an overlay.)

+
+    // These functions are called before processing messages received from the
+    // page. If the controller returns false, the message is ignored.
+    // (See corresponding functions in Receiver for details for each of these.)
+    virtual bool OnRenderViewGone() const = 0;
+    virtual bool OnAboutToNavigateMainFrame() const = 0;
+    virtual bool OnSetSuggestions() const = 0;
+    virtual bool OnShowInstantPreview() const = 0;
+    virtual bool OnStartCapturingKeyStrokes() const = 0;
+    virtual bool OnStopCapturingKeyStrokes() const = 0;
+    virtual bool OnNavigateToURL() const = 0;
+
+   protected:
+    virtual ~Controller();
+  };
+
+  // InstantAPIWrapper calls one of the methods on Receiver after receiving a
+  // message from the page. Each method is called with the |contents|
+  // corresponding to the page from which the message was received.
+  class Receiver {

[dhollowa, 2013/01/14 19:59:39]

I preferred the original name "Delegate".

[samarth, 2013/01/22 15:59:06]

Done.

+   public:
+    // Called upon determination of Instant API support. Either in response to
+    // the page loading or because we recevied some other message.
+    virtual void InstantSupportDetermined(const content::WebContents* contents,
+                                          bool supports_instant) = 0;
+
+    // Called when the underlying RenderView crashed.
+    virtual void InstantPageRenderViewGone(
+        const content::WebContents* contents) = 0;
+
+    // Called when the page is about to navigate.
+    virtual void InstantPageAboutToNavigateMainFrame(
+        const content::WebContents* contents,
+        const GURL& url) = 0;
+
     // Called when the page has suggestions. Usually in response to Change(),
     // SendAutocompleteResults() or UpOrDownKeyPressed().
     virtual void SetSuggestions(
+        const content::WebContents* contents,
         const std::vector<InstantSuggestion>& suggestions) = 0;
 
-    // Called upon determination of Instant API support. Usually in response to
-    // SetContents() or when the page first finishes loading.
-    virtual void InstantSupportDetermined(bool supports_instant) = 0;
-
     // Called when the page wants to be shown. Usually in response to Change(),
     // SendAutocompleteResults() or SearchModeChanged().
-    virtual void ShowInstantPreview(InstantShownReason reason,
+    virtual void ShowInstantPreview(const content::WebContents* contents,
+                                    InstantShownReason reason,
                                     int height,
                                     InstantSizeUnits units) = 0;
 
     // Called when the page wants the browser to start capturing user key
     // strokes.
-    virtual void StartCapturingKeyStrokes() = 0;
+    virtual void StartCapturingKeyStrokes(
+        const content::WebContents* contents) = 0;
 
     // Called when the page wants the browser to stop capturing user key
     // strokes.
-    virtual void StopCapturingKeyStrokes() = 0;
-
-    // Called when the underlying RenderView crashes.
-    virtual void RenderViewGone() = 0;
-
-    // Called when the page is about to navigate.
-    virtual void AboutToNavigateMainFrame(const GURL& url) = 0;
+    virtual void StopCapturingKeyStrokes(content::WebContents* contents) = 0;
 
-    // Called when the SearchBox wants to navigate to the specified URL.
-    virtual void NavigateToURL(const GURL& url,
+    // Called when the page wants to navigate to the specified URL.
+    virtual void NavigateToURL(const content::WebContents* contents,
+                               const GURL& url,
                                content::PageTransition transition) = 0;
 
    protected:
-    virtual ~Delegate();
+    virtual ~Receiver();
   };
 
-  // Doesn't take ownership of |delegate|.
-  explicit InstantClient(Delegate* delegate);
-  virtual ~InstantClient();
+  // Doesn't take ownership of |controller| or |receiver|.
+  InstantAPIWrapper(Controller* controller, Receiver* receiver);
+  virtual ~InstantAPIWrapper();
 
   // Sets |contents| as the page to communicate with. |contents| can be NULL,
   // which effectively stops all communication.
@@ -159,9 +201,10 @@ class InstantClient : public content::WebContentsObserver {
   void SearchBoxNavigate(int page_id, const GURL& url,
                          content::PageTransition transition);
 
-  Delegate* const delegate_;
+  Controller* const controller_;
+  Receiver* const receiver_;
 
-  DISALLOW_COPY_AND_ASSIGN(InstantClient);
+  DISALLOW_COPY_AND_ASSIGN(InstantAPIWrapper);
 };
 
-#endif  // CHROME_BROWSER_INSTANT_INSTANT_CLIENT_H_
+#endif  // CHROME_BROWSER_INSTANT_INSTANT_API_WRAPPER_H_