InstantExtended: Committed NTP

chrome/browser/instant/instant_page.h

Index: chrome/browser/instant/instant_page.h
diff --git a/chrome/browser/instant/instant_page.h b/chrome/browser/instant/instant_page.h
new file mode 100644
index 0000000000000000000000000000000000000000..ffb957f0daf207a9cd85892ba1a7166377c5ef74
--- /dev/null
+++ b/chrome/browser/instant/instant_page.h
@@ -0,0 +1,222 @@
+// Copyright 2013 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_INSTANT_INSTANT_PAGE_H_
+#define CHROME_BROWSER_INSTANT_INSTANT_PAGE_H_
+
+#include <vector>
+
+#include "base/basictypes.h"
+#include "base/compiler_specific.h"
+#include "base/string16.h"
+#include "chrome/common/instant_types.h"
+#include "content/public/browser/web_contents_observer.h"
+
+namespace chrome {
+namespace search {
+struct Mode;
+}
+}
+
+namespace content {
+class WebContents;
+}
+
+namespace gfx {
+class Rect;
+}
+

[sreeram, 2013/01/22 19:26:59]

Add these includes:
content/public/common/page_transition_types.h

and these declarations:
class GURL;

I can explain the principle behind it, if you wanna know.

[samarth, 2013/01/25 21:08:40]

Done.

+// InstantPage is used to exchange messages with a page that implements the
+// Instant/Embedded Search API (http://dev.chromium.org/embeddedsearch).
+// InstantPage is not used directly but via one of its derived classes:
+// InstantOverlay, InstantNTP and InstantTab.
+class InstantPage : public content::WebContentsObserver {
+ public:
+  // InstantPage calls its delegate in response to messages received from the
+  // page or in response to certain changes in the page. Each method is called
+  // with the |contents| corresponding to the page we are observing.
+  class Delegate {
+   public:
+    // Called upon determination of Instant API support. Either in response to
+    // the page loading or because we recevied some other message.

[sreeram, 2013/01/22 19:26:59]

"received"

[samarth, 2013/01/25 21:08:40]

Done.

+    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 to |url|.
+    virtual void InstantPageAboutToNavigateMainFrame(
+        const content::WebContents* contents,
+        const GURL& url) = 0;
+
+    // Called when the page has suggestions. Usually in response to Change(),

[sreeram, 2013/01/22 19:26:59]

Change -> Update

[samarth, 2013/01/25 21:08:40]

Done.

+    // SendAutocompleteResults() or UpOrDownKeyPressed().
+    virtual void SetSuggestions(
+        const content::WebContents* contents,
+        const std::vector<InstantSuggestion>& suggestions) = 0;
+
+    // Called when the page wants to be shown. Usually in response to Change(),

[sreeram, 2013/01/22 19:26:59]

Change -> Update

[samarth, 2013/01/25 21:08:40]

Done.

+    // SendAutocompleteResults() or SearchModeChanged().
+    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(
+        const content::WebContents* contents) = 0;
+
+    // Called when the page wants the browser to stop capturing user key
+    // strokes.
+    virtual void StopCapturingKeyStrokes(content::WebContents* contents) = 0;

[sreeram, 2013/01/22 19:26:59]

Make the comments on these two methods clearer. Talk about what it means to
"capture user key strokes". I.e., ostensibly the focus is in the |contents|, but
we'd like the omnibox to actually grab the keystrokes.

[samarth, 2013/01/25 21:08:40]

Done.

+
+    // 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;
+
+    // Called when the page is considered stale.
+    virtual void OnStalePage(const content::WebContents* contents) = 0;

[sreeram, 2013/01/22 19:26:59]

This is too hacky for my taste. Figure out a cleaner way for InstantController
to receive this call (i.e., don't shim InstantLoader calls through
InstantPage::Delegate just out of convenience). Let InstantController implement
an additional interface if necessary.

There are bunch of other methods like this (InstantPageFocused,
InstantPageClicked, etc.) that also should be moved out.

[samarth, 2013/01/25 21:08:40]

You're right.  Got rid of the InstantLoader related callbacks from here.

+
+    // Called when |new_contents| has been swapped into the page (usually
+    // because a prerendered page was navigated to).
+    virtual void InstantPageSwappedContents(
+        content::WebContents* new_contents) = 0;
+
+    // Called when the page gains focus.
+    virtual void InstantPageFocused(const content::WebContents* contents) = 0;
+
+    // Called when the user clicks on the page.
+    virtual void InstantPageClicked(const content::WebContents* contents) = 0;
+
+    // Called when a URL is about to be opened using the underlying contents.
+    // Should return true to allow the URL to open.
+    virtual bool InstantPageAboutToOpenURL(
+        const content::WebContents* contents) = 0;

[sreeram, 2013/01/22 19:26:59]

Could you reorder these so that the three "open URL" methods are placed
together, and better explain the differences amongst them? E.g., NavigateToURL()
is an explicit request from the page to the browser to open a URL, sent through
the searchbox API, because the user chose a URL suggestion (since pages can't
navigate to some URLs, such as chrome:// on their own). I don't yet appreciate
why we need both InstantPageAboutToOpenURL() as well as
InstantPageAboutToNavigateMainFrame(), but I understand the former is needed to
process redirects properly.

@shishir: Can we get rid of  InstantPageAboutToNavigateMainFrame() and do
everything we need using just InstantPageAboutToOpenURL()?

[Shishir, 2013/01/25 20:41:13]

In the present code there are two places where the InstantLoader detects that it
is trying to navigate. 

For same process navigations, the webContentsObserver's method
DidCommitProvisionalLoadForFrame is called which should call
InstantPageAboutToNavigateMainFrame.

The other code path is through the WebContentDelegate call to OpenURLFromTab
(for cross process navigations) which does not strictly belong here since
InstantPage is not a webContentsDelegate. So for eg. InstantTab will never get
that call. 

The calls are slightly different in the sense that in the first case, whether IC
commits the page or not, the navigation will happen. But in the second case, it
is upto the WebContentsDelegate to make the navigation happen.

I am not sure how the InstantNTP is supposed to behave for these cases.

[samarth, 2013/01/25 21:08:40]

Added more comments for NavigateToURL.  Got rid of the third.
 
Emailed shishir about this.

+
+   protected:
+    virtual ~Delegate();
+  };
+
+  virtual ~InstantPage();
+
+  // The WebContents corresponding to the page we're talking to. May be NULL.
+  content::WebContents* contents() const { return contents_; }
+
+  // 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).
+  bool supports_instant() const { return supports_instant_; }
+
+  // Tells the page that the user typed |text| into the omnibox. If |verbatim|
+  // is false, the page predicts the query the user means to type and fetches
+  // results for the prediction. If |verbatim| is true, |text| is taken as the
+  // exact query (no prediction is made).
+  virtual void Update(const string16& text,

[sreeram, 2013/01/22 19:26:59]

No need for this to be virtual, I think.

[samarth, 2013/01/25 21:08:40]

This is virtual to allow InstantOverlay to override it.  It needs to also update
last_navigation_ each time update is called.  Two other alternatives:
(1) Force InstantController to do this manually.  Seems like it could break
easily.
(2) Add a OnBeforeUpdate() or similar callback.

+                      size_t selection_start,
+                      size_t selection_end,
+                      bool verbatim);
+
+  // Tells the page that the user pressed Enter in the omnibox.
+  void Submit(const string16& text);
+
+  // Tells the page that the user clicked on it. Nothing is being cancelled; the
+  // poor choice of name merely reflects the IPC of the same (poor) name.
+  void Cancel(const string16& text);
+
+  // Tells the page the bounds of the omnibox dropdown (in screen coordinates).
+  // This is used by the page to offset the results to avoid them being covered
+  // by the omnibox dropdown.
+  void SetPopupBounds(const gfx::Rect& bounds);
+
+  // Tells the page what size start and end margins to use.
+  void SetMarginSize(const int start, const int end);

[sreeram, 2013/01/22 19:26:59]

Remove the const. This falls into const-craziness territory.
void SetMarginSize(int start, int end);

Also, the comment for this method is very weak. It should explain what "margin"
means, and why it's different from the other methods or how it's used. I.e.,
more like the comment for SetPopupBounds() above.

[samarth, 2013/01/25 21:08:40]

Agree that these consts are useless.  Removed.

 
Done.

+
+  // Tells the renderer to determine if the page supports the Instant API, which
+  // results in a call to InstantSupportDetermined() when the reply is received.
+  void DetermineIfPageSupportsInstant();
+
+  // Tells the page about the available autocomplete results.
+  void SendAutocompleteResults(
+      const std::vector<InstantAutocompleteResult>& results);
+
+  // Tells the page that the user pressed Up or Down in the omnibox. |count| is
+  // a repeat count, negative for moving up, positive for moving down.
+  void UpOrDownKeyPressed(int count);
+
+  // Tells the page that the active tab's search mode has changed.
+  void SearchModeChanged(const chrome::search::Mode& mode);
+
+  // Tells the page about the current theme background.
+  void SendThemeBackgroundInfo(const ThemeBackgroundInfo& theme_info);
+
+  // Tells the page about the current theme area height.
+  void SendThemeAreaHeight(int height);
+
+  // Tells the page whether it is allowed to display Instant results.
+  void SetDisplayInstantResults(bool display_instant_results);
+
+  // Tells the page whether the browser is capturing user key strokes.
+  void KeyCaptureChanged(bool is_key_capture_enabled);
+
+ protected:
+  // Doesn't take ownership of |delegate|.
+  explicit InstantPage(Delegate* delegate);
+
+  // Sets |contents| as the page to communicate with. |contents| may be NULL,
+  // which effectively stops all communication.
+  void SetContents(content::WebContents* contents);
+
+  Delegate* delegate() const { return delegate_; }
+
+  // These functions are called before processing messages received from the
+  // page. By default, all messages are handled, but any derived classes may
+  // choose to ingore some or all of the received messages by overriding these
+  // methods.
+  virtual bool ShouldProcessRenderViewGone() const;
+  virtual bool ShouldProcessAboutToNavigateMainFrame() const;
+  virtual bool ShouldProcessSetSuggestions() const;
+  virtual bool ShouldProcessShowInstantPreview() const;
+  virtual bool ShouldProcessStartCapturingKeyStrokes() const;
+  virtual bool ShouldProcessStopCapturingKeyStrokes() const;
+  virtual bool ShouldProcessNavigateToURL() const;

[sreeram, 2013/01/22 19:26:59]

I'd remove "const" from these methods. In general, unless you have a very good
reason, don't force those overriding you to be const. They may want to update
some state.

[samarth, 2013/01/25 21:08:40]

I wouldn't expect a "bool ShouldProcess..." method to alter any state.  I argue
that if we do need to do so in the future, we should rename the method (and then
make it non-const).

+
+ private:
+  // Overridden from content::WebContentsObserver:
+  virtual void DidFinishLoad(
+      int64 frame_id,
+      const GURL& validated_url,
+      bool is_main_frame,
+      content::RenderViewHost* render_view_host) OVERRIDE;
+  virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE;
+  virtual void RenderViewGone(base::TerminationStatus status) OVERRIDE;
+  virtual void DidCommitProvisionalLoadForFrame(
+      int64 frame_id,
+      bool is_main_frame,
+      const GURL& url,
+      content::PageTransition transition_type,
+      content::RenderViewHost* render_view_host) OVERRIDE;
+
+  void OnSetSuggestions(int page_id,
+                        const std::vector<InstantSuggestion>& suggestions);
+  void OnInstantSupportDetermined(int page_id, bool supports_instant);
+  void OnShowInstantPreview(int page_id,
+                            InstantShownReason reason,
+                            int height,
+                            InstantSizeUnits units);
+  void OnStartCapturingKeyStrokes(int page_id);
+  void OnStopCapturingKeyStrokes(int page_id);
+  void OnSearchBoxNavigate(int page_id, const GURL& url,
+                           content::PageTransition transition);
+
+  Delegate* const delegate_;
+  content::WebContents* contents_;
+  bool supports_instant_;

[sreeram, 2013/01/22 19:26:59]

DISALLOW_COPY_AND_ASSIGN()?

[samarth, 2013/01/25 21:08:40]

Is it necessary here?  I'm adding them to the subclasses and you can't
instantiate this class directly.

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

The style guide is pretty clear on this point.  Always required unless your
class is meant to be copy/assignable.

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

Done.

+};
+
+#endif  // CHROME_BROWSER_INSTANT_INSTANT_PAGE_H_