Merge 68816 - Fixes bug where we would show an instant preview when we thought the
search engine supported instant but it doesn't.
BUG=66133
TEST=covered by interactive ui test. You can also manually test
Review URL: http://codereview.chromium.org/5771001
TBR=sky@chromium.org
Review URL: http://codereview.chromium.org/5720002
git-svn-id: svn://svn.chromium.org/chrome/branches/597/src@68818 0039d316-1c4b-4281-b951-d872f2087c98
diff --git a/chrome/browser/instant/instant_browsertest.cc b/chrome/browser/instant/instant_browsertest.cc
index f9a0d7e1..daf4b96 100644
--- a/chrome/browser/instant/instant_browsertest.cc
+++ b/chrome/browser/instant/instant_browsertest.cc
@@ -13,6 +13,7 @@
#include "chrome/browser/location_bar.h"
#include "chrome/browser/profile.h"
#include "chrome/browser/renderer_host/render_view_host.h"
+#include "chrome/browser/renderer_host/render_widget_host_view.h"
#include "chrome/browser/search_engines/template_url.h"
#include "chrome/browser/search_engines/template_url_model.h"
#include "chrome/browser/tab_contents/tab_contents.h"
@@ -59,10 +60,16 @@
model->SetDefaultSearchProvider(template_url);
}
- // Type a character to get instant to trigger.
- void SetupLocationBar() {
+ virtual void FindLocationBar() {
+ if (location_bar_)
+ return;
location_bar_ = browser()->window()->GetLocationBar();
ASSERT_TRUE(location_bar_);
+ }
+
+ // Type a character to get instant to trigger.
+ void SetupLocationBar() {
+ FindLocationBar();
location_bar_->location_entry()->SetUserText(L"a");
}
@@ -96,7 +103,7 @@
}
void SetLocationBarText(const std::wstring& text) {
- ASSERT_TRUE(location_bar_);
+ ASSERT_NO_FATAL_FAILURE(FindLocationBar());
location_bar_->location_entry()->SetUserText(text);
ui_test_utils::WaitForNotification(
NotificationType::INSTANT_CONTROLLER_SHOWN);
@@ -179,6 +186,63 @@
1, "window.onchangecalls", preview_));
}
+// Makes sure that if the server doesn't support the instant API we don't show
+// anything.
+IN_PROC_BROWSER_TEST_F(InstantTest, SearchServerDoesntSupportInstant) {
+ ASSERT_TRUE(test_server()->Start());
+ ASSERT_NO_FATAL_FAILURE(SetupInstantProvider("empty.html"));
+ ASSERT_NO_FATAL_FAILURE(FindLocationBar());
+ location_bar_->location_entry()->SetUserText(L"a");
+ ASSERT_TRUE(browser()->instant());
+ // Because we typed in a search string we should think we're showing instant
+ // results.
+ EXPECT_TRUE(browser()->instant()->IsShowingInstant());
+ // But because we're waiting to determine if the page really supports instant
+ // we shouldn't be showing the preview.
+ EXPECT_FALSE(browser()->instant()->is_active());
+
+ // When the response comes back that the page doesn't support instant the tab
+ // should be closed.
+ ui_test_utils::WaitForNotification(NotificationType::TAB_CLOSED);
+ EXPECT_FALSE(browser()->instant()->IsShowingInstant());
+ EXPECT_FALSE(browser()->instant()->is_active());
+}
+
+// Verifies transitioning from loading a non-search string to a search string
+// with the provider not supporting instant works (meaning we don't display
+// anything).
+IN_PROC_BROWSER_TEST_F(InstantTest, NonSearchToSearchDoesntSupportInstant) {
+ ASSERT_TRUE(test_server()->Start());
+ ASSERT_NO_FATAL_FAILURE(SetupInstantProvider("empty.html"));
+ GURL url(test_server()->GetURL("files/instant/empty.html"));
+ ASSERT_NO_FATAL_FAILURE(SetLocationBarText(UTF8ToWide(url.spec())));
+ // The preview should be active and showing.
+ ASSERT_TRUE(browser()->instant()->is_active());
+ TabContentsWrapper* initial_tab = browser()->instant()->GetPreviewContents();
+ ASSERT_TRUE(initial_tab);
+ RenderWidgetHostView* rwhv =
+ initial_tab->tab_contents()->GetRenderWidgetHostView();
+ ASSERT_TRUE(rwhv);
+ ASSERT_TRUE(rwhv->IsShowing());
+
+ // Now type in some search text.
+ location_bar_->location_entry()->SetUserText(L"a");
+
+ // Instant should still be live.
+ ASSERT_TRUE(browser()->instant()->is_active());
+ // Because we typed in a search string we should think we're showing instant
+ // results.
+ EXPECT_TRUE(browser()->instant()->MightSupportInstant());
+ // Instant should not be current (it's still loading).
+ EXPECT_FALSE(browser()->instant()->IsCurrent());
+
+ // When the response comes back that the page doesn't support instant the tab
+ // should be closed.
+ ui_test_utils::WaitForNotification(NotificationType::TAB_CLOSED);
+ EXPECT_FALSE(browser()->instant()->IsShowingInstant());
+ EXPECT_FALSE(browser()->instant()->is_active());
+}
+
// Verify that the onsubmit event is dispatched upon pressing enter.
// TODO(sky): Disabled, http://crbug.com/62940.
IN_PROC_BROWSER_TEST_F(InstantTest, DISABLED_OnSubmitEvent) {
diff --git a/chrome/browser/instant/instant_controller.cc b/chrome/browser/instant/instant_controller.cc
index 6853c82..8747e9f 100644
--- a/chrome/browser/instant/instant_controller.cc
+++ b/chrome/browser/instant/instant_controller.cc
@@ -371,29 +371,27 @@
}
void InstantController::InstantLoaderDoesntSupportInstant(
- InstantLoader* loader,
- bool needs_reload,
- const GURL& url_to_load) {
+ InstantLoader* loader) {
DCHECK(!loader->ready()); // We better not be showing this loader.
DCHECK(loader->template_url_id());
+ VLOG(1) << " provider does not support instant";
+
+ // Don't attempt to use instant for this search engine again.
BlacklistFromInstant(loader->template_url_id());
if (loader_manager_->active_loader() == loader) {
- // The loader is active. Continue to use it, but make sure it isn't tied to
- // to the search engine anymore. ClearTemplateURLID ends up showing the
- // loader.
- loader_manager_->RemoveLoaderFromInstant(loader);
- loader->ClearTemplateURLID();
-
- if (needs_reload) {
- string16 suggested_text;
- loader->Update(tab_contents_, 0, url_to_load, last_transition_type_,
- loader->user_text(), false, &suggested_text);
- }
+ // The loader is active, shut down instant.
+ DestroyPreviewContents();
} else {
+ if (loader_manager_->current_loader() == loader && is_active_) {
+ // There is a pending loader and we're active. Hide the preview. When then
+ // pending loader finishes loading we'll notify the delegate to show.
+ DCHECK(loader_manager_->pending_loader());
+ is_active_ = false;
+ delegate_->HideInstant();
+ }
loader_manager_->DestroyLoader(loader);
- loader = NULL;
}
}
diff --git a/chrome/browser/instant/instant_controller.h b/chrome/browser/instant/instant_controller.h
index 08630d1f..6b1345a 100644
--- a/chrome/browser/instant/instant_controller.h
+++ b/chrome/browser/instant/instant_controller.h
@@ -186,9 +186,7 @@
virtual gfx::Rect GetInstantBounds();
virtual bool ShouldCommitInstantOnMouseUp();
virtual void CommitInstantLoader(InstantLoader* loader);
- virtual void InstantLoaderDoesntSupportInstant(InstantLoader* loader,
- bool needs_reload,
- const GURL& url_to_load);
+ virtual void InstantLoaderDoesntSupportInstant(InstantLoader* loader);
virtual void AddToBlacklist(InstantLoader* loader, const GURL& url);
diff --git a/chrome/browser/instant/instant_loader.cc b/chrome/browser/instant/instant_loader.cc
index 40cb151..f97d190f 100644
--- a/chrome/browser/instant/instant_loader.cc
+++ b/chrome/browser/instant/instant_loader.cc
@@ -557,7 +557,6 @@
CommandLine* cl = CommandLine::ForCurrentProcess();
if (cl->HasSwitch(switches::kInstantURL))
instant_url = GURL(cl->GetSwitchValueASCII(switches::kInstantURL));
- initial_instant_url_ = url;
preview_contents_->controller().LoadURL(
instant_url, GURL(), transition_type);
frame_load_observer_.reset(
@@ -647,20 +646,6 @@
delegate_->CommitInstantLoader(this);
}
-void InstantLoader::ClearTemplateURLID() {
- // This should only be invoked for sites we thought supported instant.
- DCHECK(template_url_id_);
-
- // The frame load observer should have completed.
- DCHECK(!frame_load_observer_.get());
-
- // We shouldn't be ready.
- DCHECK(!ready());
-
- template_url_id_ = 0;
- ShowPreview();
-}
-
void InstantLoader::SetCompleteSuggestedText(
const string16& complete_suggested_text) {
ShowPreview();
@@ -759,16 +744,9 @@
}
void InstantLoader::PageDoesntSupportInstant(bool needs_reload) {
- GURL url_to_load = url_;
-
- // Because we didn't process any of the requests to load in Update we're
- // actually at initial_instant_url_. We need to reset url_ so that callers see
- // the correct state.
- url_ = initial_instant_url_;
-
frame_load_observer_.reset(NULL);
- delegate_->InstantLoaderDoesntSupportInstant(this, needs_reload, url_to_load);
+ delegate_->InstantLoaderDoesntSupportInstant(this);
}
void InstantLoader::ProcessBoundsChange() {
diff --git a/chrome/browser/instant/instant_loader.h b/chrome/browser/instant/instant_loader.h
index 412a522..bae1a89 100644
--- a/chrome/browser/instant/instant_loader.h
+++ b/chrome/browser/instant/instant_loader.h
@@ -67,10 +67,6 @@
bool ShouldCommitInstantOnMouseUp();
void CommitInstantLoader();
- // Resets the template_url_id_ to zero and shows this loader. This is only
- // intended to be invoked from InstantLoaderDoesntSupportInstant.
- void ClearTemplateURLID();
-
virtual void Observe(NotificationType type,
const NotificationSource& source,
const NotificationDetails& details);
@@ -141,14 +137,11 @@
// If we're showing instant results this is the ID of the TemplateURL driving
// the results. A value of 0 means there is no TemplateURL.
- TemplateURLID template_url_id_;
+ const TemplateURLID template_url_id_;
// The url we're displaying.
GURL url_;
- // The URL first used to load instant results.
- GURL initial_instant_url_;
-
// Delegate of the preview TabContents. Used to detect when the user does some
// gesture on the TabContents and the preview needs to be activated.
scoped_ptr<TabContentsDelegateImpl> preview_tab_contents_delegate_;
diff --git a/chrome/browser/instant/instant_loader_delegate.h b/chrome/browser/instant/instant_loader_delegate.h
index 9628f5a..bfe1518 100644
--- a/chrome/browser/instant/instant_loader_delegate.h
+++ b/chrome/browser/instant/instant_loader_delegate.h
@@ -36,12 +36,8 @@
virtual void CommitInstantLoader(InstantLoader* loader) = 0;
// Invoked if the loader was created with the intention that the site supports
- // instant, but it turned out the site doesn't support instant. If
- // |needs_reload| is true, |Update| was invoked on the loader with a url that
- // has changed since the initial url, and |url_to_load| is that url.
- virtual void InstantLoaderDoesntSupportInstant(InstantLoader* loader,
- bool needs_reload,
- const GURL& url_to_load) = 0;
+ // instant, but it turned out the site doesn't support instant.
+ virtual void InstantLoaderDoesntSupportInstant(InstantLoader* loader) = 0;
// Adds the specified url to the set of urls instant won't prefetch for.
virtual void AddToBlacklist(InstantLoader* loader, const GURL& url) = 0;
diff --git a/chrome/browser/instant/instant_loader_manager_unittest.cc b/chrome/browser/instant/instant_loader_manager_unittest.cc
index 4bfefdc..98af6da 100644
--- a/chrome/browser/instant/instant_loader_manager_unittest.cc
+++ b/chrome/browser/instant/instant_loader_manager_unittest.cc
@@ -30,9 +30,7 @@
virtual void CommitInstantLoader(InstantLoader* loader) {
}
- virtual void InstantLoaderDoesntSupportInstant(InstantLoader* loader,
- bool needs_reload,
- const GURL& url_to_load) {
+ virtual void InstantLoaderDoesntSupportInstant(InstantLoader* loader) {
}
virtual void AddToBlacklist(InstantLoader* loader, const GURL& url) {
diff --git a/chrome/test/data/instant/empty.html b/chrome/test/data/instant/empty.html
new file mode 100644
index 0000000..e69de29
--- /dev/null
+++ b/chrome/test/data/instant/empty.html
