Don't destroy instant loaders if you don't have to.

As long as the omnibox retains focus, don't delete any instant loaders. This
includes cases where the user is not typing actively, or when the user switches
tabs (but retaining focus in the omnibox). The rationale is that the user may
type at any time, so it's useful to keep the instant loaders ready to process
the user query.

BUG=none
TEST=PreloadInstantTest.*


Review URL: http://codereview.chromium.org/8085010

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@103938 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 124 insertions, 17 deletions

diff --git a/chrome/browser/instant/instant_browsertest.cc b/chrome/browser/instant/instant_browsertest.cc
index 4ef1537..c6fb545 100644
--- a/chrome/browser/instant/instant_browsertest.cc
+++ b/chrome/browser/instant/instant_browsertest.cc
@@ -21,6 +21,7 @@
 #include "chrome/browser/ui/omnibox/omnibox_view.h"
 #include "chrome/browser/ui/tab_contents/tab_contents_wrapper.h"
 #include "chrome/common/chrome_notification_types.h"
+#include "chrome/common/chrome_switches.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
@@ -35,7 +36,8 @@ class InstantTest : public InProcessBrowserTest {
  public:
   InstantTest()
       : location_bar_(NULL),
-        preview_(NULL) {
+        preview_(NULL),
+        template_url_id_(0) {
     set_show_window(true);
     EnableDOMAutomation();
   }
@@ -73,6 +75,7 @@ class InstantTest : public InProcessBrowserTest {
 
     model->Add(template_url);
     model->SetDefaultSearchProvider(template_url);
+    template_url_id_ = template_url->id();
   }
 
   void FindLocationBar() {
@@ -305,9 +308,16 @@ class InstantTest : public InProcessBrowserTest {
         CheckBoolValueFromJavascript(true, "true", tab->tab_contents()));
   }
 
+  InstantLoaderManager* manager() const {
+    return browser()->instant()->loader_manager_.get();
+  }
+
  protected:
   LocationBar* location_bar_;
   TabContents* preview_;
+
+  // ID of the default search engine's template_url (in the installed model).
+  TemplateURLID template_url_id_;
 };
 
 // TODO(tonyg): Add the following tests:
@@ -1025,3 +1035,68 @@ IN_PROC_BROWSER_TEST_F(InstantTest, PendingRenderViewHost) {
   notification_observer.Wait();
   EXPECT_EQ(contents->GetURL().spec(), std::string(chrome::kChromeUIAboutURL));
 }
+
+// Tests that instant search is preloaded whenever the omnibox gets focus.
+// DISABLED http://crbug.com/80118
+#if defined(OS_LINUX)
+IN_PROC_BROWSER_TEST_F(InstantTest, DISABLED_PreloadsInstant) {
+#else
+IN_PROC_BROWSER_TEST_F(InstantTest, PreloadsInstant) {
+#endif  // OS_LINUX
+  CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kPreloadInstantSearch);
+
+  // The omnibox gets focus before the test begins. At that time, there's no
+  // instant controller (which is only created after EnableInstant()), so no
+  // preloading happens. Unfocus the omnibox with ClickOnView(), so that when
+  // we focus it again, the controller will preload instant search.
+  ASSERT_TRUE(test_server()->Start());
+  EnableInstant();
+  SetupInstantProvider("search.html");
+  ASSERT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser()));
+  ui_test_utils::ClickOnView(browser(), VIEW_ID_TAB_CONTAINER);
+
+  // Verify that there are no instant loaders initially.
+  EXPECT_TRUE(!manager() || !manager()->num_instant_loaders());
+
+  ui_test_utils::WindowedNotificationObserver instant_support_observer(
+      chrome::NOTIFICATION_INSTANT_SUPPORT_DETERMINED,
+      NotificationService::AllSources());
+
+  // Focusing the omnibox should cause instant to be preloaded.
+  FindLocationBar();
+  location_bar_->FocusLocation(false);
+  ASSERT_TRUE(manager());
+  EXPECT_EQ(1u, manager()->num_instant_loaders());
+
+  // Stash a pointer to the instant loader for use below.
+  InstantLoader* loader = manager()->GetInstantLoader(template_url_id_);
+  ASSERT_TRUE(loader);
+
+  instant_support_observer.Wait();
+
+  // However, instant should still not be active.
+  EXPECT_FALSE(browser()->instant()->is_active());
+  EXPECT_FALSE(browser()->instant()->is_displayable());
+  EXPECT_FALSE(browser()->instant()->IsShowingInstant());
+  EXPECT_FALSE(browser()->instant()->MightSupportInstant());
+
+  // Adding a new tab shouldn't delete (or recreate) the loader, since the
+  // omnibox doesn't lose focus. Comparing pointers is not the best way to
+  // assert this, but short of hooking the loader constructor or destructor,
+  // there seems to be no cleaner way.
+  AddBlankTabAndShow(browser());
+  EXPECT_EQ(loader, manager()->GetInstantLoader(template_url_id_));
+
+  // Doing a search should still use the same loader for the preview.
+  SetLocationBarText("def");
+  EXPECT_EQ(loader, manager()->GetInstantLoader(template_url_id_));
+  EXPECT_EQ(loader, manager()->current_loader());
+
+  // Verify that the preview is in fact showing instant search.
+  EXPECT_TRUE(browser()->instant()->is_active());
+  EXPECT_TRUE(browser()->instant()->is_displayable());
+  EXPECT_TRUE(browser()->instant()->IsShowingInstant());
+  EXPECT_TRUE(browser()->instant()->MightSupportInstant());
+  EXPECT_TRUE(browser()->instant()->IsCurrent());
+}
diff --git a/chrome/browser/instant/instant_controller.cc b/chrome/browser/instant/instant_controller.cc
index 5dadb5d..fdbf678 100644
--- a/chrome/browser/instant/instant_controller.cc
+++ b/chrome/browser/instant/instant_controller.cc
@@ -179,18 +179,15 @@ bool InstantController::Update(TabContentsWrapper* tab_contents,
                                string16* suggested_text) {
   suggested_text->clear();
 
-  if (tab_contents != tab_contents_)
-    DestroyPreviewContents();
-
   const GURL& url = match.destination_url;
   tab_contents_ = tab_contents;
   commit_on_mouse_up_ = false;
   last_transition_type_ = match.transition;
   const TemplateURL* template_url = NULL;
 
+  // The url should not normally be empty or invalid.
   if (url.is_empty() || !url.is_valid()) {
-    // Assume we were invoked with GURL() and should destroy all.
-    DestroyPreviewContents();
+    DestroyPreviewContentsAndLeaveActive();
     return false;
   }
 
@@ -264,9 +261,8 @@ void InstantController::DestroyPreviewContentsAndLeaveActive() {
     delegate_->HideInstant();
   }
 
-  // TODO(sky): this shouldn't nuke the loader. It should just nuke non-instant
-  // loaders and hide instant loaders.
-  loader_manager_.reset(new InstantLoaderManager(this));
+  if (loader_manager_.get())
+    loader_manager_->DestroyNonInstantLoaders();
   show_timer_.Stop();
   update_timer_.Stop();
 }
@@ -410,9 +406,6 @@ void InstantController::OnAutocompleteGotFocus(
     return;
   }
 
-  if (is_active_)
-    return;
-
   TemplateURLService* model = TemplateURLServiceFactory::GetForProfile(
       tab_contents->profile());
   if (!model)
@@ -422,8 +415,6 @@ void InstantController::OnAutocompleteGotFocus(
   if (!template_url || !template_url->instant_url() || !template_url->id())
     return;
 
-  if (tab_contents != tab_contents_)
-    DestroyPreviewContents();
   tab_contents_ = tab_contents;
 
   if (!loader_manager_.get())
diff --git a/chrome/browser/instant/instant_controller.h b/chrome/browser/instant/instant_controller.h
index 36ab76f..19ebcfa 100644
--- a/chrome/browser/instant/instant_controller.h
+++ b/chrome/browser/instant/instant_controller.h
@@ -72,9 +72,8 @@ class InstantController : public InstantLoaderDelegate {
   // Returns false if there is no instant preview showing.
   static bool CommitIfCurrent(InstantController* controller);
 
-  // Invoked as the user types in the omnibox with the url to navigate to.  If
-  // the url is empty and there is a preview TabContents it is destroyed. If url
-  // is non-empty and the preview TabContents has not been created it is
+  // Invoked as the user types in the omnibox with the url to navigate to. If
+  // the url is valid and a preview TabContents has not been created, it is
   // created. If |verbatim| is true search results are shown for |user_text|
   // rather than the best guess as to what the search thought the user meant.
   // |verbatim| only matters if the AutocompleteMatch is for a search engine
diff --git a/chrome/browser/instant/instant_loader_manager.cc b/chrome/browser/instant/instant_loader_manager.cc
index a564907..d47288c 100644
--- a/chrome/browser/instant/instant_loader_manager.cc
+++ b/chrome/browser/instant/instant_loader_manager.cc
@@ -141,6 +141,16 @@ void InstantLoaderManager::RemoveLoaderFromInstant(InstantLoader* loader) {
   instant_loaders_.erase(i);
 }
 
+void InstantLoaderManager::DestroyNonInstantLoaders() {
+  if (current_loader_ && !current_loader_->template_url_id())
+    delete current_loader_;
+  current_loader_ = NULL;
+
+  if (pending_loader_ && !pending_loader_->template_url_id())
+    delete pending_loader_;
+  pending_loader_ = NULL;
+}
+
 InstantLoader* InstantLoaderManager::GetInstantLoader(TemplateURLID id) {
   Loaders::iterator i = instant_loaders_.find(id);
   return i == instant_loaders_.end() ? CreateLoader(id) : i->second;
diff --git a/chrome/browser/instant/instant_loader_manager.h b/chrome/browser/instant/instant_loader_manager.h
index 7f2d995..7202190 100644
--- a/chrome/browser/instant/instant_loader_manager.h
+++ b/chrome/browser/instant/instant_loader_manager.h
@@ -65,6 +65,9 @@ class InstantLoaderManager {
   // If |loader| is in |instant_loaders_| it is removed.
   void RemoveLoaderFromInstant(InstantLoader* loader);
 
+  // Destroys non-instant loaders (leaves instant loaders alone).
+  void DestroyNonInstantLoaders();
+
   // Returns the loader for loading instant results with the specified id. If
   // there is no loader for the specified id a new one is created.
   InstantLoader* GetInstantLoader(TemplateURLID id);
diff --git a/chrome/browser/instant/instant_loader_manager_unittest.cc b/chrome/browser/instant/instant_loader_manager_unittest.cc
index 9854184..fc877ab 100644
--- a/chrome/browser/instant/instant_loader_manager_unittest.cc
+++ b/chrome/browser/instant/instant_loader_manager_unittest.cc
@@ -309,3 +309,32 @@ TEST_F(InstantLoaderManagerTest, UpdateWithReadyPending) {
   manager.UpdateLoader(0, &loader);
   ASSERT_NE(loader.get(), non_instant_loader);
 }
+
+// Make sure that DestroyNonInstantLoaders works.
+TEST_F(InstantLoaderManagerTest, DestroyNonInstantLoaders) {
+  InstantLoaderDelegateImpl delegate;
+  InstantLoaderManager manager(&delegate);
+  scoped_ptr<InstantLoader> loader;
+
+  manager.UpdateLoader(0, &loader);
+  InstantLoader* non_instant_loader = manager.current_loader();
+  EXPECT_TRUE(non_instant_loader);
+  EXPECT_EQ(0, non_instant_loader->template_url_id());
+  EXPECT_EQ(NULL, loader.get());
+  MarkReady(non_instant_loader);
+
+  manager.UpdateLoader(1, &loader);
+  InstantLoader* instant_loader = manager.pending_loader();
+  EXPECT_TRUE(instant_loader);
+  EXPECT_EQ(1, instant_loader->template_url_id());
+  EXPECT_EQ(NULL, loader.get());
+  EXPECT_EQ(1u, manager.num_instant_loaders());
+
+  manager.DestroyNonInstantLoaders();
+
+  EXPECT_EQ(NULL, manager.current_loader());
+  EXPECT_EQ(NULL, manager.pending_loader());
+
+  // The instant loader should still exist.
+  EXPECT_EQ(1u, manager.num_instant_loaders());
+}