InstantExtended: Committed NTP

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 b28fdc654de9ea38a96070194de439a5c1b83de5..7944c40bf90cf5386dfc61e2c05fca80e8394f8e 100644
--- a/chrome/browser/instant/instant_loader.cc
+++ b/chrome/browser/instant/instant_loader.cc
@@ -8,340 +8,71 @@
 #include "chrome/browser/extensions/api/web_navigation/web_navigation_api.h"
 #include "chrome/browser/favicon/favicon_tab_helper.h"
 #include "chrome/browser/history/history_tab_helper.h"
-#include "chrome/browser/instant/instant_controller.h"
 #include "chrome/browser/safe_browsing/safe_browsing_tab_observer.h"
+#include "chrome/browser/tab_contents/tab_util.h"
 #include "chrome/browser/ui/blocked_content/blocked_content_tab_helper.h"
 #include "chrome/browser/ui/search/search_tab_helper.h"
 #include "chrome/browser/ui/tab_contents/core_tab_helper.h"
-#include "chrome/browser/ui/tab_contents/core_tab_helper_delegate.h"
+#include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/notification_source.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/render_widget_host_view.h"
 #include "content/public/browser/site_instance.h"
-#include "content/public/browser/web_contents_delegate.h"
 #include "content/public/browser/web_contents_view.h"
-#include "ipc/ipc_message.h"
 
 namespace {
 
-int kUserDataKey;
+const int kStalePageTimeoutMS = 3 * 3600 * 1000;  // 3 hours
 
-class InstantLoaderUserData : public base::SupportsUserData::Data {
- public:
-  explicit InstantLoaderUserData(InstantLoader* loader) : loader_(loader) {}
+}  // namespace
 
-  InstantLoader* loader() const { return loader_; }
-
- private:
-  ~InstantLoaderUserData() {}
-
-  InstantLoader* const loader_;
-
-  DISALLOW_COPY_AND_ASSIGN(InstantLoaderUserData);
-};
-
-}
-
-// WebContentsDelegateImpl -----------------------------------------------------
-
-class InstantLoader::WebContentsDelegateImpl
-    : public CoreTabHelperDelegate,
-      public content::WebContentsDelegate {
- public:
-  explicit WebContentsDelegateImpl(InstantLoader* loader);
-
- private:
-  // Overridden from CoreTabHelperDelegate:
-  virtual void SwapTabContents(content::WebContents* old_contents,
-                               content::WebContents* new_contents) OVERRIDE;
-
-  // Overridden from content::WebContentsDelegate:
-  virtual bool ShouldSuppressDialogs() OVERRIDE;
-  virtual bool ShouldFocusPageAfterCrash() OVERRIDE;
-  virtual void LostCapture() OVERRIDE;
-  virtual void WebContentsFocused(content::WebContents* contents) OVERRIDE;
-  virtual bool CanDownload(content::RenderViewHost* render_view_host,
-                           int request_id,
-                           const std::string& request_method) OVERRIDE;
-  virtual void HandleMouseDown() OVERRIDE;
-  virtual void HandleMouseUp() OVERRIDE;
-  virtual void HandlePointerActivate() OVERRIDE;
-  virtual void HandleGestureEnd() OVERRIDE;
-  virtual void DragEnded() OVERRIDE;
-  virtual bool OnGoToEntryOffset(int offset) OVERRIDE;
-  virtual content::WebContents* OpenURLFromTab(
-      content::WebContents* source,
-      const content::OpenURLParams& params) OVERRIDE;
-
-  void MaybeCommitFromPointerRelease();
-
-  InstantLoader* const loader_;
-
-  DISALLOW_COPY_AND_ASSIGN(WebContentsDelegateImpl);
-};
-
-InstantLoader::WebContentsDelegateImpl::WebContentsDelegateImpl(
-    InstantLoader* loader)
-    : loader_(loader) {
-}
-
-void InstantLoader::WebContentsDelegateImpl::SwapTabContents(
-    content::WebContents* old_contents,
-    content::WebContents* new_contents) {
-  // If this is being called, something is swapping in to loader's |contents_|
-  // before we've added it to the tab strip.
-  loader_->ReplacePreviewContents(old_contents, new_contents);
-}
-
-bool InstantLoader::WebContentsDelegateImpl::ShouldSuppressDialogs() {
-  // Any message shown during Instant cancels Instant, so we suppress them.
-  return true;
-}
-
-bool InstantLoader::WebContentsDelegateImpl::ShouldFocusPageAfterCrash() {
-  return false;
-}
-
-void InstantLoader::WebContentsDelegateImpl::LostCapture() {
-  MaybeCommitFromPointerRelease();
-}
-
-void InstantLoader::WebContentsDelegateImpl::WebContentsFocused(
-    content::WebContents* /* contents */) {
-  // The preview is getting focus. Equivalent to it being clicked.
-  bool tmp = loader_->is_pointer_down_from_activate_;
-  loader_->is_pointer_down_from_activate_ = true;
-  loader_->controller_->InstantLoaderContentsFocused();
-  loader_->is_pointer_down_from_activate_ = tmp;
-}
-
-bool InstantLoader::WebContentsDelegateImpl::CanDownload(
-    content::RenderViewHost* /* render_view_host */,
-    int /* request_id */,
-    const std::string& /* request_method */) {
-  // Downloads are disabled.
-  return false;
-}
-
-void InstantLoader::WebContentsDelegateImpl::HandleMouseDown() {
-  loader_->is_pointer_down_from_activate_ = true;
-}
-
-void InstantLoader::WebContentsDelegateImpl::HandleMouseUp() {
-  MaybeCommitFromPointerRelease();
-}
-
-void InstantLoader::WebContentsDelegateImpl::HandlePointerActivate() {
-  loader_->is_pointer_down_from_activate_ = true;
-}
-
-void InstantLoader::WebContentsDelegateImpl::HandleGestureEnd() {
-  MaybeCommitFromPointerRelease();
+InstantLoader::Delegate::~Delegate() {
 }
 
-void InstantLoader::WebContentsDelegateImpl::DragEnded() {
-  // If the user drags, we won't get a mouse up (at least on Linux). Commit the
-  // Instant result when the drag ends, so that during the drag the page won't
-  // move around.
-  MaybeCommitFromPointerRelease();
+InstantLoader::InstantLoader(Delegate* delegate)
+  : delegate_(delegate),
+    contents_(NULL),
+    stale_page_timer_(false, false) {
 }
 
-bool InstantLoader::WebContentsDelegateImpl::OnGoToEntryOffset(int offset) {
-  return false;
+InstantLoader::~InstantLoader() {
 }
 
-content::WebContents* InstantLoader::WebContentsDelegateImpl::OpenURLFromTab(
-    content::WebContents* source,
-    const content::OpenURLParams& params) {
-  content::WebContents* preview = loader_->contents_.get();
-  if (loader_->controller_->CommitIfPossible(INSTANT_COMMIT_NAVIGATED))
-    return preview->GetDelegate()->OpenURLFromTab(source, params);
-  return NULL;
-}
+void InstantLoader::Load(const GURL& instant_url,
+                         Profile* profile,
+                         const content::WebContents* active_tab,
+                         const base::Closure& on_stale_callback) {
+  content::WebContents::CreateParams create_params(profile);
 
-void InstantLoader::WebContentsDelegateImpl::MaybeCommitFromPointerRelease() {
-  if (loader_->is_pointer_down_from_activate_) {
-    loader_->is_pointer_down_from_activate_ = false;
-    loader_->controller_->CommitIfPossible(INSTANT_COMMIT_FOCUS_LOST);
+  if (active_tab) {
+    create_params.initial_size = active_tab->GetView()->GetContainerSize();
+    create_params.site_instance = active_tab->GetSiteInstance()->
+        GetRelatedSiteInstance(instant_url);
+  } else {
+    create_params.site_instance = content::SiteInstance::CreateForURL(
+        profile, instant_url);
   }
-}
 
-// InstantLoader ---------------------------------------------------------------
-
-// static
-InstantLoader* InstantLoader::FromWebContents(
-    const content::WebContents* web_contents) {
-  InstantLoaderUserData* data = static_cast<InstantLoaderUserData*>(
-      web_contents->GetUserData(&kUserDataKey));
-  return data ? data->loader() : NULL;
-}
-
-InstantLoader::InstantLoader(InstantController* controller,
-                             const std::string& instant_url)
-    : client_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
-      controller_(controller),
-      delegate_(new WebContentsDelegateImpl(
-                        ALLOW_THIS_IN_INITIALIZER_LIST(this))),
-      instant_url_(instant_url),
-      supports_instant_(false),
-      is_pointer_down_from_activate_(false) {
-}
-
-InstantLoader::~InstantLoader() {
-}
-
-void InstantLoader::InitContents(const content::WebContents* active_tab) {
-  content::WebContents::CreateParams create_params(
-      active_tab->GetBrowserContext(),
-      active_tab->GetSiteInstance()->GetRelatedSiteInstance(
-          GURL(instant_url_)));
-  if (active_tab)
-    create_params.initial_size = active_tab->GetView()->GetContainerSize();
-  contents_.reset(content::WebContents::CreateWithSessionStorage(
-      create_params,
-      active_tab->GetController().GetSessionStorageNamespaceMap()));
-  SetupPreviewContents();
+  SetContents(scoped_ptr<content::WebContents>(
+      content::WebContents::Create(create_params)));
 
   // This HTTP header and value are set on loads that originate from Instant.
   const char kInstantHeader[] = "X-Purpose: Instant";
-  DVLOG(1) << "LoadURL: " << instant_url_;
-  contents_->GetController().LoadURL(GURL(instant_url_), content::Referrer(),
+  DVLOG(1) << "LoadURL: " << instant_url.spec();

[sreeram, 2013/01/29 13:35:42]

No need for the ".spec()". GURL provides an overload for operator<< that calls
possibly_invalid_spec().

[samarth, 2013/01/31 16:24:57]

Done.

+  contents_->GetController().LoadURL(
+      instant_url, content::Referrer(),
       content::PAGE_TRANSITION_GENERATED, kInstantHeader);
   contents_->WasHidden();
-}
-
-content::WebContents* InstantLoader::ReleaseContents() {
-  CleanupPreviewContents();
-  return contents_.release();
-}
-
-void InstantLoader::DidNavigate(
-    const history::HistoryAddPageArgs& add_page_args) {
-  last_navigation_ = add_page_args;
-}
-
-bool InstantLoader::IsUsingLocalPreview() const {
-  return instant_url_ == InstantController::kLocalOmniboxPopupURL;
-}
 
-void InstantLoader::Update(const string16& text,
-                           size_t selection_start,
-                           size_t selection_end,
-                           bool verbatim) {
-  last_navigation_ = history::HistoryAddPageArgs();
-  client_.Update(text, selection_start, selection_end, verbatim);
+  stale_page_timer_.Start(
+      FROM_HERE,
+      base::TimeDelta::FromMilliseconds(kStalePageTimeoutMS),
+      on_stale_callback);
 }
 
-void InstantLoader::Submit(const string16& text) {
-  client_.Submit(text);
-}
-
-void InstantLoader::Cancel(const string16& text) {
-  client_.Cancel(text);
-}
-
-void InstantLoader::SetPopupBounds(const gfx::Rect& bounds) {
-  client_.SetPopupBounds(bounds);
-}
-
-void InstantLoader::SetMarginSize(int start, int end) {
-  client_.SetMarginSize(start, end);
-}
-
-void InstantLoader::SendAutocompleteResults(
-    const std::vector<InstantAutocompleteResult>& results) {
-  client_.SendAutocompleteResults(results);
-}
-
-void InstantLoader::UpOrDownKeyPressed(int count) {
-  client_.UpOrDownKeyPressed(count);
-}
-
-void InstantLoader::SearchModeChanged(const chrome::search::Mode& mode) {
-  client_.SearchModeChanged(mode);
-}
-
-void InstantLoader::SendThemeBackgroundInfo(
-    const ThemeBackgroundInfo& theme_info) {
-  client_.SendThemeBackgroundInfo(theme_info);
-}
-
-void InstantLoader::SendThemeAreaHeight(int height) {
-  client_.SendThemeAreaHeight(height);
-}
-
-void InstantLoader::SetDisplayInstantResults(bool display_instant_results) {
-  client_.SetDisplayInstantResults(display_instant_results);
-}
-
-void InstantLoader::KeyCaptureChanged(bool is_key_capture_enabled) {
-  client_.KeyCaptureChanged(is_key_capture_enabled);
-}
-
-void InstantLoader::SetSuggestions(
-    const std::vector<InstantSuggestion>& suggestions) {
-  InstantSupportDetermined(true);
-  controller_->SetSuggestions(contents(), suggestions);
-}
-
-void InstantLoader::InstantSupportDetermined(bool supports_instant) {
-  // If we had already determined that the page supports Instant, nothing to do.
-  if (supports_instant_)
-    return;
-
-  supports_instant_ = supports_instant;
-  controller_->InstantSupportDetermined(contents(), supports_instant);
-}
-
-void InstantLoader::ShowInstantPreview(InstantShownReason reason,
-                                       int height,
-                                       InstantSizeUnits units) {
-  InstantSupportDetermined(true);
-  controller_->ShowInstantPreview(reason, height, units);
-}
-
-void InstantLoader::StartCapturingKeyStrokes() {
-  InstantSupportDetermined(true);
-  controller_->StartCapturingKeyStrokes();
-}
-
-void InstantLoader::StopCapturingKeyStrokes() {
-  InstantSupportDetermined(true);
-  controller_->StopCapturingKeyStrokes();
-}
-
-void InstantLoader::RenderViewGone() {
-  controller_->InstantLoaderRenderViewGone();
-}
-
-void InstantLoader::AboutToNavigateMainFrame(const GURL& url) {
-  controller_->InstantLoaderAboutToNavigateMainFrame(url);
-}
-
-void InstantLoader::NavigateToURL(const GURL& url,
-                                  content::PageTransition transition) {
-  InstantSupportDetermined(true);
-  controller_->NavigateToURL(url, transition);
-}
-
-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 =
-            contents_->GetRenderWidgetHostView())
-      rwhv->SetTakesFocusOnlyOnMouseDown(true);
-    return;
-  }
-  NOTREACHED();
-#endif
-}
-
-void InstantLoader::SetupPreviewContents() {
-  client_.SetContents(contents());
-  contents_->SetUserData(&kUserDataKey, new InstantLoaderUserData(this));
-  contents_->SetDelegate(delegate_.get());
+void InstantLoader::SetContents(scoped_ptr<content::WebContents> new_contents) {
+  contents_.reset(new_contents.release());
+  contents_->SetDelegate(this);
 
   // Set up various tab helpers. The rest will get attached when (if) the
   // contents is added to the tab strip.
@@ -356,7 +87,7 @@ void InstantLoader::SetupPreviewContents() {
 
   // A tab helper to catch prerender content swapping shenanigans.
   CoreTabHelper::CreateForWebContents(contents());
-  CoreTabHelper::FromWebContents(contents())->set_delegate(delegate_.get());
+  CoreTabHelper::FromWebContents(contents())->set_delegate(this);
 
   // Tab helpers used when committing a preview.
   chrome::search::SearchTabHelper::CreateForWebContents(contents());
@@ -383,9 +114,7 @@ void InstantLoader::SetupPreviewContents() {
 #endif
 }
 
-void InstantLoader::CleanupPreviewContents() {
-  client_.SetContents(NULL);
-  contents_->RemoveUserData(&kUserDataKey);
+scoped_ptr<content::WebContents> InstantLoader::ReleaseContents() {
   contents_->SetDelegate(NULL);
 
   // Undo tab helper work done in SetupPreviewContents().

[sreeram, 2013/01/29 13:35:42]

SetupPreviewContents() -> SetContents().

[samarth, 2013/01/31 16:24:57]

Done.

@@ -405,16 +134,90 @@ void InstantLoader::CleanupPreviewContents() {
                     content::Source<content::NavigationController>(
                         &contents_->GetController()));
 #endif
+
+  return contents_.Pass();
+}
+
+void InstantLoader::Observe(int type,
+                            const content::NotificationSource& source,
+                            const content::NotificationDetails& details) {

[sreeram, 2013/01/29 13:35:42]

Comment out source and details.

[samarth, 2013/01/31 16:24:57]

Done.

+#if defined(OS_MACOSX)
+  if (type == content::NOTIFICATION_RENDER_VIEW_HOST_CHANGED) {
+    if (content::RenderWidgetHostView* rwhv =
+            contents_->GetRenderWidgetHostView())
+      rwhv->SetTakesFocusOnlyOnMouseDown(true);
+    return;
+  }
+  NOTREACHED();
+#endif
 }
 
-void InstantLoader::ReplacePreviewContents(content::WebContents* old_contents,
-                                           content::WebContents* new_contents) {
+void InstantLoader::SwapTabContents(content::WebContents* old_contents,
+                                    content::WebContents* new_contents) {
   DCHECK_EQ(old_contents, contents());
-  CleanupPreviewContents();
-  // We release here without deleting so that the caller still has the
-  // responsibility for deleting the WebContents.
-  ignore_result(contents_.release());
-  contents_.reset(new_contents);
-  SetupPreviewContents();
-  controller_->SwappedWebContents();
+  // We release here without deleting since the caller has the responsibility
+  // for deleting the old WebContents.
+  scoped_ptr<content::WebContents> released_contents =
+      delegate_->SwapContents(scoped_ptr<content::WebContents>(new_contents));
+  ignore_result(released_contents.release());
+}
+
+// Overridden from content::WebContentsDelegate:

[sreeram, 2013/01/29 13:35:42]

No need for this comment.

[samarth, 2013/01/31 16:24:57]

Done.

+bool InstantLoader::ShouldSuppressDialogs() {
+  // Any message shown during Instant cancels Instant, so we suppress them.

[sreeram, 2013/01/29 13:35:42]

Any message -> Messages
(to match the "them" and ...Dialogs())

[samarth, 2013/01/31 16:24:57]

Done.

+  return true;
+}
+
+bool InstantLoader::ShouldFocusPageAfterCrash() {
+  return false;
+}
+
+void InstantLoader::LostCapture() {
+  delegate_->OnMouseUp();
+}
+
+void InstantLoader::WebContentsFocused(content::WebContents* contents) {

[sreeram, 2013/01/29 13:35:42]

/* contents */

[samarth, 2013/01/31 16:24:57]

Done.

[jam, 2013/02/01 17:44:34]

btw i just saw this. this isn't chrome style. please let's not spread
conventions from other projects into chrome.

[sreeram, 2013/02/01 17:50:02]

This style is in fact explicitly mentioned in the Google C++ style guide (and
the Chromium style guide makes no mention of it). See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

I agree that the style is not prevalent in Chrome, but it's not zero. A grep for
something like "/\*.*\*/ *[{,]$" will show you quite a lot of matches (including
false positives).

+  delegate_->OnFocus();
+}
+
+bool InstantLoader::CanDownload(content::RenderViewHost*,
+                                int,
+                                const std::string&) {

[sreeram, 2013/01/29 13:35:42]

Provide the names of the parameters, but comment them out.

[samarth, 2013/01/31 16:24:57]

Done.

+  // Downloads are disabled.
+  return false;
+}
+
+void InstantLoader::HandleMouseDown() {
+  delegate_->OnMouseDown();
+}
+
+void InstantLoader::HandleMouseUp() {
+  delegate_->OnMouseUp();
+}
+
+void InstantLoader::HandlePointerActivate() {
+  delegate_->OnMouseDown();
+}
+
+void InstantLoader::HandleGestureEnd() {
+  delegate_->OnMouseUp();
+}
+
+void InstantLoader::DragEnded() {
+  // If the user drags, we won't get a mouse up (at least on Linux). Commit
+  // the Instant result when the drag ends, so that during the drag the page
+  // won't move around.
+  delegate_->OnMouseUp();
+}
+
+bool InstantLoader::OnGoToEntryOffset(int offset) {

[sreeram, 2013/01/29 13:35:42]

/* offset */

[samarth, 2013/01/31 16:24:57]

Done.

+  return false;
+}
+
+content::WebContents* InstantLoader::OpenURLFromTab(
+    content::WebContents* source,
+    const content::OpenURLParams& params) {
+  if (delegate_->OnOpenURL())
+    return contents_->GetDelegate()->OpenURLFromTab(source, params);

[sreeram, 2013/01/29 13:35:42]

Isn't this an infinite recursion? contents_->GetDelegate() == this, no?

[samarth, 2013/01/31 16:24:57]

See my comments in the header about this.  (There was a bug here though:
contents_ would no longer be non-NULL since OnOpenURL could commit the page.)

+  return NULL;
 }