Persist the Instant API to committed search result pages.

chrome/browser/instant/instant_loader.cc

Index: chrome/browser/instant/instant_loader.cc
diff --git a/chrome/browser/instant/instant_loader.cc b/chrome/browser/instant/instant_loader.cc
index 2df213ce5053ad813160707b7c1ed9cc9098b72d..e5b9aeaa8da1a7cf2aeda2845e9a600641e9ee84 100644
--- a/chrome/browser/instant/instant_loader.cc
+++ b/chrome/browser/instant/instant_loader.cc
@@ -47,8 +47,7 @@ class InstantLoaderUserData : public base::SupportsUserData::Data {
 class InstantLoader::WebContentsDelegateImpl
     : public ConstrainedWindowTabHelperDelegate,
       public CoreTabHelperDelegate,
-      public content::WebContentsDelegate,
-      public content::WebContentsObserver {
+      public content::WebContentsDelegate {
  public:
   explicit WebContentsDelegateImpl(InstantLoader* loader);
 
@@ -56,10 +55,6 @@ class InstantLoader::WebContentsDelegateImpl
     return is_pointer_down_from_activate_;
   }
 
-  // Start observing |web_contents| instead of whatever is currently being
-  // observed. If |web_contents| is NULL, effectively stops observing.
-  void ObserveContents(content::WebContents* web_contents);
-
  private:
   // Overridden from ConstrainedWindowTabHelperDelegate:
   virtual bool ShouldFocusConstrainedWindow() OVERRIDE;
@@ -83,29 +78,7 @@ class InstantLoader::WebContentsDelegateImpl
   virtual void DragEnded() OVERRIDE;
   virtual bool OnGoToEntryOffset(int offset) OVERRIDE;
 
-  // 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;
-
-  // Message from renderer indicating the page has suggestions.
-  void OnSetSuggestions(int page_id,
-                        const std::vector<InstantSuggestion>& suggestions);
-
-  // Message from the renderer determining whether it supports the Instant API.
-  void OnInstantSupportDetermined(int page_id, bool result);
-
-  // Message from the renderer requesting the preview be shown.
-  void OnShowInstantPreview(int page_id,
-                            InstantShownReason reason,
-                            int height,
-                            InstantSizeUnits units);
-
   void CommitFromPointerReleaseIfNecessary();
-  void MaybeSetAndNotifyInstantSupportDetermined(bool supports_instant);
 
   InstantLoader* const loader_;
 
@@ -121,11 +94,6 @@ InstantLoader::WebContentsDelegateImpl::WebContentsDelegateImpl(
       is_pointer_down_from_activate_(false) {
 }
 
-void InstantLoader::WebContentsDelegateImpl::ObserveContents(
-    content::WebContents* web_contents) {
-  Observe(web_contents);
-}
-
 bool InstantLoader::WebContentsDelegateImpl::ShouldFocusConstrainedWindow() {
   // Return false so that constrained windows are not initially focused. If we
   // did otherwise the preview would prematurely get committed when focus goes
@@ -194,68 +162,6 @@ bool InstantLoader::WebContentsDelegateImpl::OnGoToEntryOffset(int offset) {
   return false;
 }
 
-void InstantLoader::WebContentsDelegateImpl::DidFinishLoad(
-    int64 frame_id,
-    const GURL& validated_url,
-    bool is_main_frame,
-    content::RenderViewHost* render_view_host) {
-  if (is_main_frame && !loader_->supports_instant_)
-    Send(new ChromeViewMsg_DetermineIfPageSupportsInstant(routing_id()));
-}
-
-bool InstantLoader::WebContentsDelegateImpl::OnMessageReceived(
-    const IPC::Message& message) {
-  bool handled = true;
-  IPC_BEGIN_MESSAGE_MAP(WebContentsDelegateImpl, message)
-    IPC_MESSAGE_HANDLER(ChromeViewHostMsg_SetSuggestions, OnSetSuggestions)
-    IPC_MESSAGE_HANDLER(ChromeViewHostMsg_InstantSupportDetermined,
-                        OnInstantSupportDetermined)
-    IPC_MESSAGE_HANDLER(ChromeViewHostMsg_ShowInstantPreview,
-                        OnShowInstantPreview);
-    IPC_MESSAGE_UNHANDLED(handled = false)
-  IPC_END_MESSAGE_MAP()
-  return handled;
-}
-
-void InstantLoader::WebContentsDelegateImpl::OnSetSuggestions(
-    int page_id,
-    const std::vector<InstantSuggestion>& suggestions) {
-  DCHECK(loader_->preview_contents());
-  DCHECK(loader_->preview_contents_->web_contents());
-  content::NavigationEntry* entry = loader_->preview_contents_->web_contents()->
-                                        GetController().GetActiveEntry();
-  if (entry && page_id == entry->GetPageID()) {
-    MaybeSetAndNotifyInstantSupportDetermined(true);
-    loader_->controller_->SetSuggestions(loader_, suggestions);
-  }
-}
-
-void InstantLoader::WebContentsDelegateImpl::OnInstantSupportDetermined(
-    int page_id,
-    bool result) {
-  DCHECK(loader_->preview_contents());
-  DCHECK(loader_->preview_contents_->web_contents());
-  content::NavigationEntry* entry = loader_->preview_contents_->web_contents()->
-                                        GetController().GetActiveEntry();
-  if (entry && page_id == entry->GetPageID())
-    MaybeSetAndNotifyInstantSupportDetermined(result);
-}
-
-void InstantLoader::WebContentsDelegateImpl::OnShowInstantPreview(
-    int page_id,
-    InstantShownReason reason,
-    int height,
-    InstantSizeUnits units) {
-  DCHECK(loader_->preview_contents());
-  DCHECK(loader_->preview_contents_->web_contents());
-  content::NavigationEntry* entry = loader_->preview_contents_->web_contents()->
-                                        GetController().GetActiveEntry();
-  if (entry && page_id == entry->GetPageID()) {
-    MaybeSetAndNotifyInstantSupportDetermined(true);
-    loader_->controller_->ShowInstantPreview(loader_, reason, height, units);
-  }
-}
-
 void InstantLoader::WebContentsDelegateImpl
     ::CommitFromPointerReleaseIfNecessary() {
   if (is_pointer_down_from_activate_) {
@@ -264,15 +170,6 @@ void InstantLoader::WebContentsDelegateImpl
   }
 }
 
-void InstantLoader::WebContentsDelegateImpl
-    ::MaybeSetAndNotifyInstantSupportDetermined(bool supports_instant) {
-  // If we already determined that the loader supports Instant, nothing to do.
-  if (!loader_->supports_instant_) {
-    loader_->supports_instant_ = supports_instant;
-    loader_->controller_->InstantSupportDetermined(loader_, supports_instant);
-  }
-}
-
 // InstantLoader ---------------------------------------------------------------
 
 // static
@@ -286,7 +183,7 @@ InstantLoader* InstantLoader::FromWebContents(
 InstantLoader::InstantLoader(InstantController* controller,
                              const std::string& instant_url,
                              const TabContents* tab_contents)
-    : controller_(controller),
+    : InstantClient(controller, NULL),

[sky, 2012/11/27 01:07:51]

I get why you subclassed here, but it feels a bit iffy. It seems like
InstantLoader and what is InstantClient want to have an InstantClient rather
than be InstantClient.

[samarth, 2012/11/27 18:09:43]

This might be what sky is already saying, but what about having InstantClient
(or some other name) be in charge of multiplexing between the active tab and the
preview contents? That has two benefits:
1) You don't need to split up the loader functionality into two places (which
will make things harder to find and understand later)
2) We can reduce the burden on InstantController which is already handling tons
of state and tricky logic.

[sreeram, 2012/11/27 18:41:33]

I understand the style guide's preference for composition over inheritance. If
we do that here, I think we'll have something like this (say the new stuff is
called InstantTab; and the object that both InstantTab and InstantLoader hold is
called InstantClient):

void InstantController::SetSuggestions(InstantClient* client, ...) {
  if (CurrentClient()->client() != client || ...)
    return;

I.e., when calling back from the loader to InstantController, we'll have to pass
in something that's common to both InstantTab and InstantLoader, and then do
this roundabout check to see which one actually called the InstantController.
("roundabout" because there's really no reason for InstantTab/InstantLoader to
expose their client() to InstantController otherwise.)

The above is a somewhat small point, but overall I think inheritance makes the
code leaner in this case. I think composition will just give us a whole bunch of
shim methods for no good reason.

To samarth's point: I don't think we can do the multiplexing outside the
InstantController easily. The logic is quite complex, and depends on
InstantController state (such as search_mode_ and is_focused_ and such), so we'd
have to expose all of that as well.

[sreeram, 2012/11/29 07:33:19]

Done. Composition wins.

       preview_delegate_(new WebContentsDelegateImpl(
           ALLOW_THIS_IN_INITIALIZER_LIST(this))),
       preview_contents_(
@@ -296,7 +193,6 @@ InstantLoader::InstantLoader(InstantController* controller,
                   tab_contents->web_contents(),
                   tab_contents->web_contents()->GetController().
                                           GetSessionStorageNamespaceMap()))),
-      supports_instant_(false),
       instant_url_(instant_url) {
 }
 
@@ -308,60 +204,30 @@ void InstantLoader::Init() {
   // This HTTP header and value are set on loads that originate from instant.
   const char kInstantHeader[] = "X-Purpose: Instant";
   DVLOG(1) << "LoadURL: " << instant_url_;
-  preview_contents_->web_contents()->GetController().LoadURL(GURL(instant_url_),
-      content::Referrer(), content::PAGE_TRANSITION_GENERATED, kInstantHeader);
-  preview_contents_->web_contents()->WasHidden();
+  contents()->GetController().LoadURL(GURL(instant_url_), content::Referrer(),
+      content::PAGE_TRANSITION_GENERATED, kInstantHeader);
+  contents()->WasHidden();
 }
 
-void InstantLoader::Update(const string16& user_text, bool verbatim) {
-  // TODO: Support real cursor position.
-  last_navigation_ = history::HistoryAddPageArgs();
-  content::RenderViewHost* rvh =
-      preview_contents_->web_contents()->GetRenderViewHost();
-  rvh->Send(new ChromeViewMsg_SearchBoxChange(rvh->GetRoutingID(), user_text,
-                    verbatim, user_text.size(), user_text.size()));
+void InstantLoader::Cancel(const string16& text) {
+  Send(new ChromeViewMsg_SearchBoxCancel(routing_id(), text));
 }
 
 void InstantLoader::SetOmniboxBounds(const gfx::Rect& bounds) {
-  content::RenderViewHost* rvh =
-      preview_contents_->web_contents()->GetRenderViewHost();
-  rvh->Send(new ChromeViewMsg_SearchBoxResize(rvh->GetRoutingID(), bounds));
-}
-
-void InstantLoader::SendAutocompleteResults(
-    const std::vector<InstantAutocompleteResult>& results) {
-  content::RenderViewHost* rvh =
-      preview_contents_->web_contents()->GetRenderViewHost();
-  rvh->Send(new ChromeViewMsg_SearchBoxAutocompleteResults(rvh->GetRoutingID(),
-                                                           results));
+  Send(new ChromeViewMsg_SearchBoxResize(routing_id(), bounds));
 }
 
 void InstantLoader::SendThemeBackgroundInfo(
       const ThemeBackgroundInfo& theme_info) {
-  content::RenderViewHost* rvh =
-      preview_contents_->web_contents()->GetRenderViewHost();
-  rvh->Send(new ChromeViewMsg_SearchBoxThemeChanged(rvh->GetRoutingID(),
-                                                    theme_info));
+  Send(new ChromeViewMsg_SearchBoxThemeChanged(routing_id(), theme_info));
 }
 
 void InstantLoader::SendThemeAreaHeight(int height) {
-  content::RenderViewHost* rvh =
-      preview_contents_->web_contents()->GetRenderViewHost();
-  rvh->Send(new ChromeViewMsg_SearchBoxThemeAreaHeightChanged(
-      rvh->GetRoutingID(), height));
-}
-
-void InstantLoader::OnUpOrDownKeyPressed(int count) {
-  content::RenderViewHost* rvh =
-      preview_contents_->web_contents()->GetRenderViewHost();
-  rvh->Send(new ChromeViewMsg_SearchBoxUpOrDownKeyPressed(rvh->GetRoutingID(),
-                                                          count));
+  Send(new ChromeViewMsg_SearchBoxThemeAreaHeightChanged(routing_id(), height));
 }
 
 void InstantLoader::SearchModeChanged(const chrome::search::Mode& mode) {
-  content::RenderViewHost* rvh =
-      preview_contents_->web_contents()->GetRenderViewHost();
-  rvh->Send(new ChromeViewMsg_SearchBoxModeChanged(rvh->GetRoutingID(), mode));
+  Send(new ChromeViewMsg_SearchBoxModeChanged(routing_id(), mode));
 }
 
 void InstantLoader::DidNavigate(
@@ -369,55 +235,56 @@ void InstantLoader::DidNavigate(
   last_navigation_ = add_page_args;
 }
 
-TabContents* InstantLoader::ReleasePreviewContents(InstantCommitType type,
-                                                   const string16& text) {
-  content::RenderViewHost* rvh =
-      preview_contents_->web_contents()->GetRenderViewHost();
-  if (type == INSTANT_COMMIT_FOCUS_LOST)
-    rvh->Send(new ChromeViewMsg_SearchBoxCancel(rvh->GetRoutingID(), text));
-  else
-    rvh->Send(new ChromeViewMsg_SearchBoxSubmit(rvh->GetRoutingID(), text));
-  CleanupPreviewContents();
-  return preview_contents_.release();
-}
-
 void InstantLoader::CleanupPreviewContents() {
-  content::WebContents* old_contents = preview_contents_->web_contents();
-  old_contents->RemoveUserData(&kUserDataKey);
-  old_contents->SetDelegate(NULL);
-  preview_delegate_->ObserveContents(NULL);
+  contents()->RemoveUserData(&kUserDataKey);
+  contents()->SetDelegate(NULL);
 
-  BlockedContentTabHelper::FromWebContents(old_contents)->
+  BlockedContentTabHelper::FromWebContents(contents())->
       SetAllContentsBlocked(false);
-  ConstrainedWindowTabHelper::FromWebContents(old_contents)->set_delegate(NULL);
-  TabSpecificContentSettings::FromWebContents(old_contents)->
+  ConstrainedWindowTabHelper::FromWebContents(contents())->set_delegate(NULL);
+  TabSpecificContentSettings::FromWebContents(contents())->
       SetPopupsBlocked(false);
-  CoreTabHelper::FromWebContents(old_contents)->set_delegate(NULL);
+  CoreTabHelper::FromWebContents(contents())->set_delegate(NULL);
   if (ThumbnailTabHelper* thumbnail_tab_helper =
-          ThumbnailTabHelper::FromWebContents(old_contents))
+          ThumbnailTabHelper::FromWebContents(contents()))
     thumbnail_tab_helper->set_enabled(true);
 
 #if defined(OS_MACOSX)
   if (content::RenderWidgetHostView* rwhv =
-          old_contents->GetRenderWidgetHostView())
+          contents()->GetRenderWidgetHostView())
     rwhv->SetTakesFocusOnlyOnMouseDown(false);
   registrar_.Remove(this, content::NOTIFICATION_RENDER_VIEW_HOST_CHANGED,
                     content::Source<content::NavigationController>(
-                        &old_contents->GetController()));
+                        &contents()->GetController()));
 #endif
+
+  content::WebContentsObserver::Observe(NULL);
 }
 
 bool InstantLoader::IsPointerDownFromActivate() const {
   return preview_delegate_->is_pointer_down_from_activate();
 }
 
+bool InstantLoader::OnMessageReceived(const IPC::Message& message) {
+  if (InstantClient::OnMessageReceived(message))
+    return true;
+
+  bool handled = true;
+  IPC_BEGIN_MESSAGE_MAP(InstantLoader, message)
+    IPC_MESSAGE_HANDLER(ChromeViewHostMsg_ShowInstantPreview,
+                        OnShowInstantPreview);
+    IPC_MESSAGE_UNHANDLED(handled = false)
+  IPC_END_MESSAGE_MAP()
+  return handled;
+}
+
 void InstantLoader::Observe(int type,
                             const content::NotificationSource& source,
                             const content::NotificationDetails& details) {
 #if defined(OS_MACOSX)
   if (type == content::NOTIFICATION_RENDER_VIEW_HOST_CHANGED) {
     if (content::RenderWidgetHostView* rwhv =
-            preview_contents_->web_contents()->GetRenderWidgetHostView())
+            contents()->GetRenderWidgetHostView())
       rwhv->SetTakesFocusOnlyOnMouseDown(true);
     return;
   }
@@ -426,23 +293,23 @@ void InstantLoader::Observe(int type,
 }
 
 void InstantLoader::SetupPreviewContents() {
-  content::WebContents* new_contents = preview_contents_->web_contents();
-  new_contents->SetUserData(&kUserDataKey, new InstantLoaderUserData(this));
-  WebContentsDelegateImpl* new_delegate = preview_delegate_.get();
-  new_contents->SetDelegate(new_delegate);
-  new_delegate->ObserveContents(new_contents);
+  content::WebContentsObserver::Observe(preview_contents_->web_contents());
+
+  contents()->SetUserData(&kUserDataKey, new InstantLoaderUserData(this));
+  contents()->SetDelegate(preview_delegate_.get());
 
   // Disable popups and such (mainly to avoid losing focus and reverting the
   // preview prematurely).
-  BlockedContentTabHelper::FromWebContents(new_contents)->
+  BlockedContentTabHelper::FromWebContents(contents())->
       SetAllContentsBlocked(true);
-  ConstrainedWindowTabHelper::FromWebContents(new_contents)->
-      set_delegate(new_delegate);
-  TabSpecificContentSettings::FromWebContents(new_contents)->
+  ConstrainedWindowTabHelper::FromWebContents(contents())->
+      set_delegate(preview_delegate_.get());
+  TabSpecificContentSettings::FromWebContents(contents())->
       SetPopupsBlocked(true);
-  CoreTabHelper::FromWebContents(new_contents)->set_delegate(new_delegate);
+  CoreTabHelper::FromWebContents(contents())->
+      set_delegate(preview_delegate_.get());
   if (ThumbnailTabHelper* thumbnail_tab_helper =
-          ThumbnailTabHelper::FromWebContents(new_contents))
+          ThumbnailTabHelper::FromWebContents(contents()))
     thumbnail_tab_helper->set_enabled(false);
 
 #if defined(OS_MACOSX)
@@ -450,17 +317,17 @@ void InstantLoader::SetupPreviewContents() {
   // SetTakesFocusOnlyOnMouseDown() as a result of the RENDER_VIEW_HOST_CHANGED
   // notification.
   if (content::RenderWidgetHostView* rwhv =
-          new_contents->GetRenderWidgetHostView())
+          contents()->GetRenderWidgetHostView())
     rwhv->SetTakesFocusOnlyOnMouseDown(true);
   registrar_.Add(this, content::NOTIFICATION_RENDER_VIEW_HOST_CHANGED,
                  content::Source<content::NavigationController>(
-                     &new_contents->GetController()));
+                     &contents()->GetController()));
 #endif
 }
 
 void InstantLoader::ReplacePreviewContents(content::WebContents* old_contents,
                                            content::WebContents* new_contents) {
-  DCHECK_EQ(old_contents, preview_contents_->web_contents());
+  DCHECK_EQ(old_contents, contents());
   CleanupPreviewContents();
   // We release here without deleting so that the caller still has the
   // responsibility for deleting the TabContents.
@@ -469,3 +336,15 @@ void InstantLoader::ReplacePreviewContents(content::WebContents* old_contents,
   SetupPreviewContents();
   controller_->SwappedTabContents(this);
 }
+
+void InstantLoader::OnShowInstantPreview(int page_id,
+                                         InstantShownReason reason,
+                                         int height,
+                                         InstantSizeUnits units) {
+  content::NavigationEntry* entry =
+      contents()->GetController().GetActiveEntry();
+  if (entry && page_id == entry->GetPageID()) {
+    NotifyInstantSupportDetermined(true);
+    controller_->ShowInstantPreview(this, reason, height, units);
+  }
+}