InstantExtended: Committed NTP

chrome/browser/ui/browser_instant_controller.cc

Index: chrome/browser/ui/browser_instant_controller.cc
diff --git a/chrome/browser/ui/browser_instant_controller.cc b/chrome/browser/ui/browser_instant_controller.cc
index 4118de59b0de8e390a68fa3136e01b1275fbafad..eacae9d26f90113e81b138c829c724880cef73d7 100644
--- a/chrome/browser/ui/browser_instant_controller.cc
+++ b/chrome/browser/ui/browser_instant_controller.cc
@@ -19,6 +19,7 @@
 #include "chrome/browser/ui/webui/ntp/app_launcher_handler.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/pref_names.h"
+#include "chrome/common/url_constants.h"
 #include "content/public/browser/notification_service.h"
 #include "grit/theme_resources.h"
 #include "ui/gfx/color_utils.h"
@@ -95,6 +96,44 @@ void BrowserInstantController::RegisterUserPrefs(PrefServiceSyncable* prefs) {
                              PrefServiceSyncable::SYNCABLE_PREF);
 }
 
+bool BrowserInstantController::MaybeSwapInInstantNTPContents(
+    const GURL& url,
+    content::WebContents* source_contents,
+    content::WebContents** target_contents) {
+  if (url != GURL(chrome::kChromeUINewTabURL))
+    return false;
+
+  scoped_ptr<content::WebContents> instant_ntp = instant_.ReleaseNTPContents();
+  if (!instant_ntp)
+    return false;
+
+  *target_contents = instant_ntp.get();
+  instant_ntp->GetController().PruneAllButActive();
+  if (source_contents) {
+    instant_ntp->GetController().CopyStateFromAndPrune(
+        &source_contents->GetController());
+    ReplaceWebContentsAt(
+        browser_->tab_strip_model()->GetIndexOfWebContents(source_contents),
+        instant_ntp.Pass());
+  } else {
+    // If |source_contents| is NULL, then the caller is responsible for
+    // inserting instant_ntp into the tabstrip and will take ownership.
+    ignore_result(instant_ntp.release());
+  }
+  // Focus the omnibox since the omnibox should have focus whenever the New Tab
+  // Page is opened.
+  //
+  // TODO(samarth): this doesn't handle all cases, e.g., if you arrive at the
+  // NTP using back/forward buttons. Doing this properly will require telling
+  // the content layer that navigations to this page should result in the
+  // omnibox being focused.
+  MessageLoop::current()->PostTask(
+      FROM_HERE,
+      base::Bind(&BrowserInstantController::FocusOmniboxForNTP,

[sky, 2013/02/05 21:40:13]

Why the delay here? What if this is destroyed before the task completes. Or what
if something else runs before the task that wants to change focus?

[samarth, 2013/02/05 22:41:57]

Yeah, you'right: this will probably break.

I needed to delay because later in the same event loop, the active WebContents
will take focus away from the omnibox. But the real solution is to fix
WebContents::ShouldFocusLocationBarByDefault. I'll follow up with jam@ about
that.

Removed this code.

+                 base::Unretained(this)));
+  return true;
+}
+
 bool BrowserInstantController::OpenInstant(WindowOpenDisposition disposition) {
   // Unsupported dispositions.
   if (disposition == NEW_BACKGROUND_TAB || disposition == NEW_WINDOW)
@@ -110,23 +149,17 @@ bool BrowserInstantController::OpenInstant(WindowOpenDisposition disposition) {
       INSTANT_COMMIT_PRESSED_ENTER : INSTANT_COMMIT_PRESSED_ALT_ENTER);
 }
 
-void BrowserInstantController::CommitInstant(content::WebContents* preview,
-                                             bool in_new_tab) {
+void BrowserInstantController::CommitInstant(
+    scoped_ptr<content::WebContents> preview,
+    bool in_new_tab) {
   if (in_new_tab) {
     // TabStripModel takes ownership of |preview|.
-    browser_->tab_strip_model()->AddWebContents(preview, -1,
+    browser_->tab_strip_model()->AddWebContents(preview.release(), -1,
         instant_.last_transition_type(), TabStripModel::ADD_ACTIVE);
   } else {
     int index = browser_->tab_strip_model()->active_index();
-    DCHECK_NE(TabStripModel::kNoTab, index);
-    content::WebContents* active_tab =
-        browser_->tab_strip_model()->GetWebContentsAt(index);
-    // TabStripModel takes ownership of |preview|.
-    browser_->tab_strip_model()->ReplaceWebContentsAt(index, preview);
-    // InstantUnloadHandler takes ownership of |active_tab|.
-    instant_unload_handler_.RunUnloadListenersOrDestroy(active_tab, index);
-
-    GURL url = preview->GetURL();
+    const GURL& url = preview->GetURL();
+    ReplaceWebContentsAt(index, preview.Pass());
     DCHECK(browser_->profile()->GetExtensionService());
     if (browser_->profile()->GetExtensionService()->IsInstalledApp(url)) {
       AppLauncherHandler::RecordAppLaunchType(
@@ -135,6 +168,21 @@ void BrowserInstantController::CommitInstant(content::WebContents* preview,
   }
 }
 
+void BrowserInstantController::ReplaceWebContentsAt(
+    int index,
+    scoped_ptr<content::WebContents> new_contents) {
+  DCHECK_NE(TabStripModel::kNoTab, index);
+  content::WebContents* old_contents =
+      browser_->tab_strip_model()->GetWebContentsAt(index);
+  // TabStripModel takes ownership of |new_contents|.
+  browser_->tab_strip_model()->ReplaceWebContentsAt(
+      index, new_contents.release());
+  // TODO(samarth): use scoped_ptr instead of comments to document ownership
+  // transfer.
+  // InstantUnloadHandler takes ownership of |old_contents|.
+  instant_unload_handler_.RunUnloadListenersOrDestroy(old_contents, index);
+}
+
 void BrowserInstantController::SetInstantSuggestion(
     const InstantSuggestion& suggestion) {
   browser_->window()->GetLocationBar()->SetInstantSuggestion(suggestion);
@@ -319,4 +367,13 @@ void BrowserInstantController::OnThemeAreaHeightChanged(int height) {
   instant_.ThemeAreaHeightChanged(theme_area_height_);
 }
 
+void BrowserInstantController::FocusOmniboxForNTP() {
+  const content::WebContents* active_contents =
+      browser_->tab_strip_model()->GetActiveWebContents();
+  if (active_contents && active_contents->GetURL() ==
+      GURL(chrome::kChromeUINewTabURL)) {
+    browser_->window()->GetLocationBar()->GetLocationEntry()->SetFocus();
+  }
+}
+
 }  // namespace chrome