From 1946c9389f17ca1c3dac2d30d2eb40cb06d59413 Mon Sep 17 00:00:00 2001 From: "sky@chromium.org" Date: Wed, 15 Dec 2010 00:07:38 +0000 Subject: [PATCH] Fixes bug in instant that resulted in flickery fade. The problem would happen if instant wouldn't load the current url and you continued typing. For example, if the search engine doesn't support the instant API on each key press you would see the page fade out, then snap in. The fix is to make sure instant stays active until destroyed. That way if the preview is no longer valid, but instant is still active the page can remain faded out. This also adds a couple more tests for better coverage. BUG=66720 66539 TEST=see bug Review URL: http://codereview.chromium.org/5701008 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@69201 0039d316-1c4b-4281-b951-d872f2087c98 --- chrome/browser/gtk/browser_window_gtk.cc | 4 +- chrome/browser/gtk/browser_window_gtk.h | 2 +- chrome/browser/instant/instant_browsertest.cc | 168 ++++++++++++++++-- chrome/browser/instant/instant_controller.cc | 72 +++++--- chrome/browser/instant/instant_controller.h | 23 ++- chrome/browser/instant/instant_delegate.h | 4 +- chrome/browser/ui/browser.cc | 2 +- chrome/browser/ui/browser_window.h | 3 +- .../browser/ui/cocoa/browser_window_cocoa.h | 2 +- .../browser/ui/cocoa/browser_window_cocoa.mm | 4 +- .../location_bar/location_bar_view_mac.mm | 11 +- chrome/browser/ui/views/frame/browser_view.cc | 10 +- chrome/browser/ui/views/frame/browser_view.h | 2 +- .../ui/views/frame/contents_container.cc | 76 ++++++-- .../ui/views/frame/contents_container.h | 14 ++ chrome/test/data/instant/403.html | 2 + .../data/instant/403.html.mock-http-headers | 1 + chrome/test/test_browser_window.h | 2 +- 18 files changed, 331 insertions(+), 71 deletions(-) create mode 100644 chrome/test/data/instant/403.html create mode 100644 chrome/test/data/instant/403.html.mock-http-headers diff --git a/chrome/browser/gtk/browser_window_gtk.cc b/chrome/browser/gtk/browser_window_gtk.cc index cff9e27f8314c2..183b0b27c82220 100644 --- a/chrome/browser/gtk/browser_window_gtk.cc +++ b/chrome/browser/gtk/browser_window_gtk.cc @@ -1123,9 +1123,11 @@ void BrowserWindowGtk::ShowInstant(TabContents* preview_contents) { contents->CancelInstantFade(); } -void BrowserWindowGtk::HideInstant() { +void BrowserWindowGtk::HideInstant(bool instant_is_active) { contents_container_->PopPreviewContents(); MaybeShowBookmarkBar(false); + + // TODO(sky): honor instant_is_active. } gfx::Rect BrowserWindowGtk::GetInstantBounds() { diff --git a/chrome/browser/gtk/browser_window_gtk.h b/chrome/browser/gtk/browser_window_gtk.h index a00cf4bca4ec1c..3e6db38cf3a60d 100644 --- a/chrome/browser/gtk/browser_window_gtk.h +++ b/chrome/browser/gtk/browser_window_gtk.h @@ -128,7 +128,7 @@ class BrowserWindowGtk : public BrowserWindow, virtual void ToggleTabStripMode() {} virtual void PrepareForInstant(); virtual void ShowInstant(TabContents* preview_contents); - virtual void HideInstant(); + virtual void HideInstant(bool instant_is_active); virtual gfx::Rect GetInstantBounds(); // Overridden from NotificationObserver: diff --git a/chrome/browser/instant/instant_browsertest.cc b/chrome/browser/instant/instant_browsertest.cc index f362158d866284..8fb61f8b0f4c6a 100644 --- a/chrome/browser/instant/instant_browsertest.cc +++ b/chrome/browser/instant/instant_browsertest.cc @@ -66,22 +66,36 @@ class InstantTest : public InProcessBrowserTest { ASSERT_TRUE(location_bar_); } + TabContentsWrapper* GetPendingPreviewContents() { + return browser()->instant()->GetPendingPreviewContents(); + } + // Type a character to get instant to trigger. void SetupLocationBar() { FindLocationBar(); location_bar_->location_entry()->SetUserText(L"a"); } - // Wait for instant to load and ensure it is in the state we expect. - void SetupPreview() { - preview_ = browser()->instant()->GetPreviewContents()->tab_contents(); + // Waits for preview to be shown. + void WaitForPreviewToNavigate(bool use_current) { + InstantController* instant = browser()->instant(); + ASSERT_TRUE(instant); + TabContentsWrapper* tab = use_current ? + instant->GetPreviewContents() : GetPendingPreviewContents(); + ASSERT_TRUE(tab); + preview_ = tab->tab_contents(); ASSERT_TRUE(preview_); ui_test_utils::WaitForNavigation(&preview_->controller()); + } + + // Wait for instant to load and ensure it is in the state we expect. + void SetupPreview() { + // Wait for the preview to navigate. + WaitForPreviewToNavigate(true); - // Verify the initial setup of the search box. - ASSERT_TRUE(browser()->instant()); EXPECT_TRUE(browser()->instant()->IsShowingInstant()); - EXPECT_FALSE(browser()->instant()->is_active()); + EXPECT_FALSE(browser()->instant()->is_displayable()); + EXPECT_TRUE(browser()->instant()->is_active()); // When the page loads, the initial searchBox values are set and only a // resize will have been sent. @@ -152,6 +166,13 @@ class InstantTest : public InProcessBrowserTest { EXPECT_EQ(expected, result); } + // Sends a message to the renderer and waits for the response to come back to + // the browser. + void WaitForMessageToBeProcessedByRenderer(TabContentsWrapper* tab) { + ASSERT_NO_FATAL_FAILURE( + CheckBoolValueFromJavascript(true, "true", tab->tab_contents())); + } + protected: virtual void SetUpCommandLine(CommandLine* command_line) { command_line->AppendSwitch(switches::kEnablePredictiveInstant); @@ -185,6 +206,94 @@ IN_PROC_BROWSER_TEST_F(InstantTest, OnChangeEvent) { 1, "window.onchangecalls", preview_)); } +// Verify instant preview is shown correctly for a non-search query. +IN_PROC_BROWSER_TEST_F(InstantTest, ShowPreviewNonSearch) { + ASSERT_TRUE(test_server()->Start()); + 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()); + ASSERT_TRUE(browser()->instant()->is_displayable()); + ASSERT_TRUE(browser()->instant()->IsCurrent()); + ASSERT_TRUE(browser()->instant()->GetPreviewContents()); + RenderWidgetHostView* rwhv = + browser()->instant()->GetPreviewContents()->tab_contents()-> + GetRenderWidgetHostView(); + ASSERT_TRUE(rwhv); + ASSERT_TRUE(rwhv->IsShowing()); +} + +// Transition from non-search to search and make sure everything is shown +// correctly. +IN_PROC_BROWSER_TEST_F(InstantTest, NonSearchToSearch) { + ASSERT_TRUE(test_server()->Start()); + 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()); + ASSERT_TRUE(browser()->instant()->is_displayable()); + 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. + ASSERT_NO_FATAL_FAILURE(SetupInstantProvider("search.html")); + location_bar_->location_entry()->SetUserText(L"abc"); + + // Wait for the preview to navigate. + ASSERT_NO_FATAL_FAILURE(WaitForPreviewToNavigate(false)); + + // The controller is still determining if the provider really supports + // instant. As a result the tabcontents should not have changed. + TabContentsWrapper* current_tab = browser()->instant()->GetPreviewContents(); + ASSERT_EQ(current_tab, initial_tab); + // The preview should still be showing. + rwhv = current_tab->tab_contents()->GetRenderWidgetHostView(); + ASSERT_TRUE(rwhv); + ASSERT_TRUE(rwhv->IsShowing()); + + // Use MightSupportInstant as the controller is still determining if the + // page supports instant and hasn't actually commited yet. + EXPECT_TRUE(browser()->instant()->MightSupportInstant()); + + // Instant should still be active. + EXPECT_TRUE(browser()->instant()->is_active()); + EXPECT_TRUE(browser()->instant()->is_displayable()); + + // Because we're waiting on the page, instant isn't current. + ASSERT_FALSE(browser()->instant()->IsCurrent()); + + // Bounce a message to the renderer so that we know the instant has gotten a + // response back from the renderer as to whether the page supports instant. + ASSERT_NO_FATAL_FAILURE( + WaitForMessageToBeProcessedByRenderer(GetPendingPreviewContents())); + + // Reset the user text so that the page is told the text changed. We should be + // able to nuke this once 66104 is fixed. + location_bar_->location_entry()->SetUserText(L"abcd"); + + // Wait for the renderer to process it. + ASSERT_NO_FATAL_FAILURE( + WaitForMessageToBeProcessedByRenderer(GetPendingPreviewContents())); + + // We should have gotten a response back from the renderer that resulted in + // committing. + ASSERT_FALSE(GetPendingPreviewContents()); + ASSERT_TRUE(browser()->instant()->is_active()); + ASSERT_TRUE(browser()->instant()->is_displayable()); + TabContentsWrapper* new_tab = browser()->instant()->GetPreviewContents(); + ASSERT_TRUE(new_tab); + ASSERT_NE(new_tab, initial_tab); + RenderWidgetHostView* new_rwhv = + new_tab->tab_contents()->GetRenderWidgetHostView(); + ASSERT_TRUE(new_rwhv); + ASSERT_NE(new_rwhv, rwhv); + ASSERT_TRUE(new_rwhv->IsShowing()); +} + // Makes sure that if the server doesn't support the instant API we don't show // anything. IN_PROC_BROWSER_TEST_F(InstantTest, SearchServerDoesntSupportInstant) { @@ -198,26 +307,29 @@ IN_PROC_BROWSER_TEST_F(InstantTest, SearchServerDoesntSupportInstant) { 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()); + EXPECT_FALSE(browser()->instant()->is_displayable()); + // But instant should still be active. + EXPECT_TRUE(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()); + EXPECT_FALSE(browser()->instant()->is_displayable()); + EXPECT_TRUE(browser()->instant()->is_active()); + EXPECT_FALSE(browser()->instant()->IsCurrent()); } // 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). -// Flaky, http://crbug.com/66539. -IN_PROC_BROWSER_TEST_F(InstantTest, - FLAKY_NonSearchToSearchDoesntSupportInstant) { +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_displayable()); ASSERT_TRUE(browser()->instant()->is_active()); TabContentsWrapper* initial_tab = browser()->instant()->GetPreviewContents(); ASSERT_TRUE(initial_tab); @@ -230,6 +342,7 @@ IN_PROC_BROWSER_TEST_F(InstantTest, location_bar_->location_entry()->SetUserText(L"a"); // Instant should still be live. + ASSERT_TRUE(browser()->instant()->is_displayable()); ASSERT_TRUE(browser()->instant()->is_active()); // Because we typed in a search string we should think we're showing instant // results. @@ -241,7 +354,9 @@ IN_PROC_BROWSER_TEST_F(InstantTest, // should be closed. ui_test_utils::WaitForNotification(NotificationType::TAB_CLOSED); EXPECT_FALSE(browser()->instant()->IsShowingInstant()); - EXPECT_FALSE(browser()->instant()->is_active()); + EXPECT_FALSE(browser()->instant()->is_displayable()); + // But because the omnibox is still open, instant should be active. + ASSERT_TRUE(browser()->instant()->is_active()); } // Verifies the page was told a non-zero height. @@ -252,7 +367,7 @@ IN_PROC_BROWSER_TEST_F(InstantTest, ValidHeight) { ASSERT_NO_FATAL_FAILURE(SetupInstantProvider("old_api.html")); ASSERT_NO_FATAL_FAILURE(SetLocationBarText(L"a")); // The preview should be active. - ASSERT_TRUE(browser()->instant()->is_active()); + ASSERT_TRUE(browser()->instant()->is_displayable()); // And the height should be valid. TabContents* tab = browser()->instant()->GetPreviewContents()->tab_contents(); ASSERT_NO_FATAL_FAILURE( @@ -267,6 +382,33 @@ IN_PROC_BROWSER_TEST_F(InstantTest, ValidHeight) { EXPECT_GT(height, 0); } +// Verifies that if the server returns a 403 we don't show the preview and +// query the host again. +IN_PROC_BROWSER_TEST_F(InstantTest, HideOn403) { + ASSERT_TRUE(test_server()->Start()); + GURL url(test_server()->GetURL("files/instant/403.html")); + ASSERT_NO_FATAL_FAILURE(FindLocationBar()); + location_bar_->location_entry()->SetUserText(UTF8ToWide(url.spec())); + // The preview shouldn't be showing, but it should be loading. + ASSERT_TRUE(browser()->instant()->GetPreviewContents()); + ASSERT_TRUE(browser()->instant()->is_active()); + ASSERT_FALSE(browser()->instant()->is_displayable()); + + // When instant sees the 403, it should close the tab. + ui_test_utils::WaitForNotification(NotificationType::TAB_CLOSED); + ASSERT_FALSE(browser()->instant()->GetPreviewContents()); + ASSERT_TRUE(browser()->instant()->is_active()); + ASSERT_FALSE(browser()->instant()->is_displayable()); + + // Try loading another url on the server. Instant shouldn't create a new tab + // as the server returned 403. + GURL url2(test_server()->GetURL("files/instant/empty.html")); + location_bar_->location_entry()->SetUserText(UTF8ToWide(url2.spec())); + ASSERT_FALSE(browser()->instant()->GetPreviewContents()); + ASSERT_TRUE(browser()->instant()->is_active()); + ASSERT_FALSE(browser()->instant()->is_displayable()); +} + // 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 28a22de347d958..4e02f28ce2d3ff 100644 --- a/chrome/browser/instant/instant_controller.cc +++ b/chrome/browser/instant/instant_controller.cc @@ -38,6 +38,7 @@ InstantController::InstantController(Profile* profile, : delegate_(delegate), tab_contents_(NULL), is_active_(false), + is_displayable_(false), commit_on_mouse_up_(false), last_transition_type_(PageTransition::LINK), ALLOW_THIS_IN_INITIALIZER_LIST(destroy_factory_(this)), @@ -156,17 +157,24 @@ void InstantController::Update(TabContentsWrapper* tab_contents, last_transition_type_ = match.transition; const TemplateURL* template_url = NULL; - if (url.is_empty() || !url.is_valid() || - !ShouldShowPreviewFor(match, &template_url)) { + if (url.is_empty() || !url.is_valid()) { + // Assume we were invoked with GURL() and should destroy all. DestroyPreviewContents(); return; } + if (!ShouldShowPreviewFor(match, &template_url)) { + DestroyAndLeaveActive(); + return; + } + if (!loader_manager_.get()) loader_manager_.reset(new InstantLoaderManager(this)); - if (!is_active_) + if (!is_active_) { + is_active_ = true; delegate_->PrepareForInstant(); + } TemplateURLID template_url_id = template_url ? template_url->id() : 0; // Verbatim only makes sense if the search engines supports instant. @@ -206,13 +214,16 @@ void InstantController::DestroyPreviewContents() { return; } + // ReleasePreviewContents sets is_active_ to false, but we need to set it + // beore notifying the delegate so. + is_active_ = false; delegate_->HideInstant(); delete ReleasePreviewContents(INSTANT_COMMIT_DESTROY); } bool InstantController::IsCurrent() { - return loader_manager_.get() && loader_manager_->active_loader()->ready() && - !update_timer_.IsRunning(); + return loader_manager_.get() && loader_manager_->active_loader() && + loader_manager_->active_loader()->ready() && !update_timer_.IsRunning(); } void InstantController::CommitCurrentPreview(InstantCommitType type) { @@ -235,8 +246,10 @@ bool InstantController::IsMouseDownFromActivate() { void InstantController::OnAutocompleteLostFocus( gfx::NativeView view_gaining_focus) { - if (!is_active() || !GetPreviewContents()) + if (!is_active() || !GetPreviewContents()) { + DestroyPreviewContents(); return; + } RenderWidgetHostView* rwhv = GetPreviewContents()->tab_contents()->GetRenderWidgetHostView(); @@ -302,9 +315,10 @@ TabContentsWrapper* InstantController::ReleasePreviewContents( ClearBlacklist(); is_active_ = false; - omnibox_bounds_ = gfx::Rect(); + is_displayable_ = false; commit_on_mouse_up_ = false; - loader_manager_.reset(NULL); + omnibox_bounds_ = gfx::Rect(); + loader_manager_.reset(); update_timer_.Stop(); return tab; } @@ -314,24 +328,24 @@ void InstantController::CompleteRelease(TabContents* tab) { } TabContentsWrapper* InstantController::GetPreviewContents() { - return loader_manager_.get() ? + return loader_manager_.get() && loader_manager_->current_loader() ? loader_manager_->current_loader()->preview_contents() : NULL; } bool InstantController::IsShowingInstant() { - return loader_manager_.get() && + return loader_manager_.get() && loader_manager_->current_loader() && loader_manager_->current_loader()->is_showing_instant(); } bool InstantController::MightSupportInstant() { - return loader_manager_.get() && + return loader_manager_.get() && loader_manager_->active_loader() && loader_manager_->active_loader()->is_showing_instant(); } void InstantController::ShowInstantLoader(InstantLoader* loader) { DCHECK(loader_manager_.get()); if (loader_manager_->current_loader() == loader) { - is_active_ = true; + is_displayable_ = true; delegate_->ShowInstant(loader->preview_contents()); } else if (loader_manager_->pending_loader() == loader) { scoped_ptr old_loader; @@ -377,20 +391,20 @@ void InstantController::InstantLoaderDoesntSupportInstant( DCHECK(!loader->ready()); // We better not be showing this loader. DCHECK(loader->template_url_id()); - VLOG(1) << " provider does not support instant"; + 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, shut down instant. - DestroyPreviewContents(); + // The loader is active, hide all. + DestroyAndLeaveActive(); } else { - if (loader_manager_->current_loader() == loader && is_active_) { + if (loader_manager_->current_loader() == loader && is_displayable_) { // 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; + is_displayable_ = false; delegate_->HideInstant(); } loader_manager_->DestroyLoader(loader); @@ -413,16 +427,30 @@ void InstantController::AddToBlacklist(InstantLoader* loader, const GURL& url) { ScheduleDestroy(loader); loader_manager_->ReleaseLoader(loader); - if (is_active_ && + if (is_displayable_ && (!loader_manager_->active_loader() || !loader_manager_->current_loader()->ready())) { // Hide instant. When the pending loader finishes loading we'll go active // again. - is_active_ = false; + is_displayable_ = false; delegate_->HideInstant(); } } +void InstantController::DestroyAndLeaveActive() { + is_displayable_ = false; + commit_on_mouse_up_ = false; + delegate_->HideInstant(); + + loader_manager_.reset(new InstantLoaderManager(this)); + update_timer_.Stop(); +} + +TabContentsWrapper* InstantController::GetPendingPreviewContents() { + return loader_manager_.get() && loader_manager_->pending_loader() ? + loader_manager_->pending_loader()->preview_contents() : NULL; +} + bool InstantController::ShouldUpdateNow(TemplateURLID instant_id, const GURL& url) { DCHECK(loader_manager_.get()); @@ -446,8 +474,10 @@ bool InstantController::ShouldUpdateNow(TemplateURLID instant_id, // WillUpateChangeActiveLoader should return true if no active loader, so // we know there will be an active loader if we get here. DCHECK(active_loader); - // Immediately update if the hosts differ, otherwise we'll delay the update. - return active_loader->url().host() != url.host(); + // Immediately update if the url is the same (which should result in nothing + // happening) or the hosts differ, otherwise we'll delay the update. + return (active_loader->url() == url) || + (active_loader->url().host() != url.host()); } void InstantController::ScheduleUpdate(const GURL& url) { diff --git a/chrome/browser/instant/instant_controller.h b/chrome/browser/instant/instant_controller.h index 6b1345ae3d7ba5..1482481fb3d9a7 100644 --- a/chrome/browser/instant/instant_controller.h +++ b/chrome/browser/instant/instant_controller.h @@ -27,6 +27,7 @@ struct AutocompleteMatch; class InstantDelegate; class InstantLoader; class InstantLoaderManager; +class InstantTest; class PrefService; class Profile; class TabContents; @@ -153,10 +154,14 @@ class InstantController : public InstantLoaderDelegate { // The preview TabContents; may be null. TabContentsWrapper* GetPreviewContents(); - // Returns true if the preview TabContents is active. In some situations this - // may return false yet preview_contents() returns non-NULL. + // Returns true if |Update| has been invoked without a corresponding call to + // |DestroyPreviewContents| or |CommitCurrentPreview|. bool is_active() const { return is_active_; } + // Returns true if the preview TabContents is ready to be displayed. In some + // situations this may return false yet GetPreviewContents() returns non-NULL. + bool is_displayable() const { return is_displayable_; } + // Returns the transition type of the last AutocompleteMatch passed to Update. PageTransition::Type last_transition_type() const { return last_transition_type_; @@ -191,8 +196,17 @@ class InstantController : public InstantLoaderDelegate { private: + friend class InstantTest; + typedef std::set HostBlacklist; + // Destroys the current loaders but remains actives. + void DestroyAndLeaveActive(); + + // Returns the TabContents of the pending loader (or NULL). This is only used + // for testing. + TabContentsWrapper* GetPendingPreviewContents(); + // Returns true if we should update immediately. bool ShouldUpdateNow(TemplateURLID instant_id, const GURL& url); @@ -253,9 +267,12 @@ class InstantController : public InstantLoaderDelegate { // The TabContents last passed to |Update|. TabContentsWrapper* tab_contents_; + // See description above getter for details. + bool is_active_; + // Has notification been sent out that the preview TabContents is ready to be // shown? - bool is_active_; + bool is_displayable_; // See description above setter. gfx::Rect omnibox_bounds_; diff --git a/chrome/browser/instant/instant_delegate.h b/chrome/browser/instant/instant_delegate.h index 32d94e64494c95..a908981641ac8e 100644 --- a/chrome/browser/instant/instant_delegate.h +++ b/chrome/browser/instant/instant_delegate.h @@ -26,7 +26,9 @@ class InstantDelegate { // Invoked when the instant TabContents should be shown. virtual void ShowInstant(TabContentsWrapper* preview_contents) = 0; - // Invoked when the instant TabContents should be hidden. + // Invoked when the instant TabContents should be hidden. Instant still may be + // active at the time this is invoked. Use |is_active()| to determine if + // instant is still active. virtual void HideInstant() = 0; // Invoked when the user does something that should result in the preview diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc index b5bdd630de2e08..67d313acaf84d2 100644 --- a/chrome/browser/ui/browser.cc +++ b/chrome/browser/ui/browser.cc @@ -3380,7 +3380,7 @@ void Browser::ShowInstant(TabContentsWrapper* preview_contents) { } void Browser::HideInstant() { - window_->HideInstant(); + window_->HideInstant(instant_->is_active()); } void Browser::CommitInstant(TabContentsWrapper* preview_contents) { diff --git a/chrome/browser/ui/browser_window.h b/chrome/browser/ui/browser_window.h index 77c88985756e19..11e2c50cf7540f 100644 --- a/chrome/browser/ui/browser_window.h +++ b/chrome/browser/ui/browser_window.h @@ -323,7 +323,8 @@ class BrowserWindow { virtual void ShowInstant(TabContents* preview_contents) = 0; // Invoked when the instant's tab contents should be hidden. - virtual void HideInstant() = 0; + // |instant_is_active| indicates if instant is still active. + virtual void HideInstant(bool instant_is_active) = 0; // Returns the desired bounds for instant in screen coordinates. Note that if // instant isn't currently visible this returns the bounds instant would be diff --git a/chrome/browser/ui/cocoa/browser_window_cocoa.h b/chrome/browser/ui/cocoa/browser_window_cocoa.h index 316d0626c4a272..a7dd79bce32e19 100644 --- a/chrome/browser/ui/cocoa/browser_window_cocoa.h +++ b/chrome/browser/ui/cocoa/browser_window_cocoa.h @@ -110,7 +110,7 @@ class BrowserWindowCocoa : public BrowserWindow, virtual void OpenTabpose(); virtual void PrepareForInstant(); virtual void ShowInstant(TabContents* preview_contents); - virtual void HideInstant(); + virtual void HideInstant(bool instant_is_active); virtual gfx::Rect GetInstantBounds(); // Overridden from NotificationObserver diff --git a/chrome/browser/ui/cocoa/browser_window_cocoa.mm b/chrome/browser/ui/cocoa/browser_window_cocoa.mm index fc8697765aee4a..0ebcafebdb638a 100644 --- a/chrome/browser/ui/cocoa/browser_window_cocoa.mm +++ b/chrome/browser/ui/cocoa/browser_window_cocoa.mm @@ -587,8 +587,10 @@ + (NSMenuItem*)itemForKeyEquivalent:(NSEvent*)key [controller_ showInstant:preview_contents]; } -void BrowserWindowCocoa::HideInstant() { +void BrowserWindowCocoa::HideInstant(bool instant_is_active) { [controller_ hideInstant]; + + // TODO: add support for |instant_is_active|. } gfx::Rect BrowserWindowCocoa::GetInstantBounds() { diff --git a/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm b/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm index 72f3172da472d7..436963d6ad584f 100644 --- a/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm +++ b/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm @@ -233,18 +233,17 @@ new KeywordHintDecoration(AutocompleteEditViewMac::GetFieldFont())), if (!instant) return; - if (!instant->is_active() || !instant->GetPreviewContents()) - return; - // If |IsMouseDownFromActivate()| returns false, the RenderWidgetHostView did // not receive a mouseDown event. Therefore, we should destroy the preview. // Otherwise, the RWHV was clicked, so we commit the preview. - if (!instant->IsMouseDownFromActivate()) + if (!instant->is_displayable() || !instant->GetPreviewContents() || + !instant->IsMouseDownFromActivate()) { instant->DestroyPreviewContents(); - else if (instant->IsShowingInstant()) + } else if (instant->IsShowingInstant()) { instant->SetCommitOnMouseUp(); - else + } else { instant->CommitCurrentPreview(INSTANT_COMMIT_FOCUS_LOST); + } } void LocationBarViewMac::OnAutocompleteWillAccept() { diff --git a/chrome/browser/ui/views/frame/browser_view.cc b/chrome/browser/ui/views/frame/browser_view.cc index b970b16346b745..769b59f1e40668 100644 --- a/chrome/browser/ui/views/frame/browser_view.cc +++ b/chrome/browser/ui/views/frame/browser_view.cc @@ -1369,18 +1369,20 @@ void BrowserView::ShowInstant(TabContents* preview_contents) { contents_->RemoveFade(); } -void BrowserView::HideInstant() { - if (!preview_container_) { +void BrowserView::HideInstant(bool instant_is_active) { + if (instant_is_active) + contents_->ShowFade(); + else contents_->RemoveFade(); + + if (!preview_container_) return; - } // The contents must be changed before SetPreview is invoked. preview_container_->ChangeTabContents(NULL); contents_->SetPreview(NULL, NULL); delete preview_container_; preview_container_ = NULL; - contents_->RemoveFade(); } gfx::Rect BrowserView::GetInstantBounds() { diff --git a/chrome/browser/ui/views/frame/browser_view.h b/chrome/browser/ui/views/frame/browser_view.h index 40364d1753ba27..402f4f28b9f308 100644 --- a/chrome/browser/ui/views/frame/browser_view.h +++ b/chrome/browser/ui/views/frame/browser_view.h @@ -325,7 +325,7 @@ class BrowserView : public BrowserBubbleHost, virtual void ToggleTabStripMode(); virtual void PrepareForInstant(); virtual void ShowInstant(TabContents* preview_contents); - virtual void HideInstant(); + virtual void HideInstant(bool instant_is_active); virtual gfx::Rect GetInstantBounds(); #if defined(OS_CHROMEOS) virtual void ShowKeyboardOverlay(gfx::NativeWindow owning_window); diff --git a/chrome/browser/ui/views/frame/contents_container.cc b/chrome/browser/ui/views/frame/contents_container.cc index 0e24ac7a331557..ccf0abd0597b96 100644 --- a/chrome/browser/ui/views/frame/contents_container.cc +++ b/chrome/browser/ui/views/frame/contents_container.cc @@ -5,6 +5,7 @@ #include "chrome/browser/views/frame/contents_container.h" #include "app/slide_animation.h" +#include "base/logging.h" #include "third_party/skia/include/core/SkColor.h" #include "views/background.h" #include "views/widget/root_view.h" @@ -14,11 +15,35 @@ static const int kMinOpacity = 0; static const int kMaxOpacity = 192; +// View used to track when the overlay widget is destroyed. If the +// ContentsContainer is still valid when the destructor is invoked this invokes +// |OverlayViewDestroyed| on the ContentsContainer. +class ContentsContainer::OverlayContentView : public views::View { + public: + explicit OverlayContentView(ContentsContainer* container) + : container_(container) { + } + ~OverlayContentView() { + if (container_) + container_->OverlayViewDestroyed(); + } + + void Detach() { + container_ = NULL; + } + + private: + ContentsContainer* container_; + + DISALLOW_COPY_AND_ASSIGN(OverlayContentView); +}; + ContentsContainer::ContentsContainer(views::View* active) : active_(active), preview_(NULL), preview_tab_contents_(NULL), active_overlay_(NULL), + overlay_view_(NULL), active_top_margin_(0) { AddChildView(active_); } @@ -26,6 +51,8 @@ ContentsContainer::ContentsContainer(views::View* active) ContentsContainer::~ContentsContainer() { // We don't need to explicitly delete active_overlay_ as it'll be deleted by // virtue of being a child window. + if (overlay_view_) + overlay_view_->Detach(); } void ContentsContainer::MakePreviewContentsActiveContents() { @@ -82,26 +109,21 @@ void ContentsContainer::FadeActiveContents() { overlay_animation_->SetSlideDuration(300); overlay_animation_->Show(); - active_overlay_ = views::Widget::CreatePopupWidget(views::Widget::Transparent, - views::Widget::NotAcceptEvents, - views::Widget::DeleteOnDestroy, - views::Widget::MirrorOriginInRTL); - active_overlay_->SetOpacity(0); - gfx::Point screen_origin; - views::View::ConvertPointToScreen(active_, &screen_origin); - gfx::Rect overlay_bounds(screen_origin, active_->size()); - active_overlay_->Init(active_->GetWidget()->GetNativeView(), overlay_bounds); - views::View* content_view = new views::View(); - content_view->set_background( - views::Background::CreateSolidBackground(SK_ColorWHITE)); - active_overlay_->SetContentsView(content_view); - active_overlay_->Show(); - active_overlay_->MoveAbove(active_->GetWidget()); + CreateOverlay(kMinOpacity); +} + +void ContentsContainer::ShowFade() { + if (active_overlay_ || !Animation::ShouldRenderRichAnimation()) + return; + + CreateOverlay(kMaxOpacity); } void ContentsContainer::RemoveFade() { overlay_animation_.reset(); if (active_overlay_) { + overlay_view_->Detach(); + overlay_view_ = NULL; active_overlay_->Close(); active_overlay_ = NULL; } @@ -126,3 +148,27 @@ void ContentsContainer::Layout() { // still need a layout. views::View::Layout(); } + +void ContentsContainer::CreateOverlay(int initial_opacity) { + DCHECK(!active_overlay_); + active_overlay_ = views::Widget::CreatePopupWidget(views::Widget::Transparent, + views::Widget::NotAcceptEvents, + views::Widget::DeleteOnDestroy, + views::Widget::MirrorOriginInRTL); + active_overlay_->SetOpacity(initial_opacity); + gfx::Point screen_origin; + views::View::ConvertPointToScreen(active_, &screen_origin); + gfx::Rect overlay_bounds(screen_origin, active_->size()); + active_overlay_->Init(active_->GetWidget()->GetNativeView(), overlay_bounds); + overlay_view_ = new OverlayContentView(this); + overlay_view_->set_background( + views::Background::CreateSolidBackground(SK_ColorWHITE)); + active_overlay_->SetContentsView(overlay_view_); + active_overlay_->Show(); + active_overlay_->MoveAbove(active_->GetWidget()); +} + +void ContentsContainer::OverlayViewDestroyed() { + active_overlay_ = NULL; + overlay_view_ = NULL; +} diff --git a/chrome/browser/ui/views/frame/contents_container.h b/chrome/browser/ui/views/frame/contents_container.h index 9b32e2d8d5e922..0429b9f2980ec3 100644 --- a/chrome/browser/ui/views/frame/contents_container.h +++ b/chrome/browser/ui/views/frame/contents_container.h @@ -45,6 +45,9 @@ class ContentsContainer : public views::View, public AnimationDelegate { // Fades out the active contents. void FadeActiveContents(); + // Shows the fade. This is similiar to |FadeActiveContents|, but is immediate. + void ShowFade(); + // Removes the fade. This is done implicitly when the preview is made active. void RemoveFade(); @@ -55,6 +58,14 @@ class ContentsContainer : public views::View, public AnimationDelegate { virtual void AnimationProgressed(const Animation* animation); private: + class OverlayContentView; + + // Creates the overlay widget. The opacity is set at |initial_opacity|. + void CreateOverlay(int initial_opacity); + + // Invoked when the contents view of the overlay is destroyed. + void OverlayViewDestroyed(); + views::View* active_; views::View* preview_; @@ -65,6 +76,9 @@ class ContentsContainer : public views::View, public AnimationDelegate { // make the active view appear faded out. views::Widget* active_overlay_; + // Content view of active_overlay. Used to track when the widget is destroyed. + OverlayContentView* overlay_view_; + // Animation used to vary the opacity of active_overlay. scoped_ptr overlay_animation_; diff --git a/chrome/test/data/instant/403.html b/chrome/test/data/instant/403.html new file mode 100644 index 00000000000000..90531a4b3ed18b --- /dev/null +++ b/chrome/test/data/instant/403.html @@ -0,0 +1,2 @@ + + diff --git a/chrome/test/data/instant/403.html.mock-http-headers b/chrome/test/data/instant/403.html.mock-http-headers new file mode 100644 index 00000000000000..78537b6737be8c --- /dev/null +++ b/chrome/test/data/instant/403.html.mock-http-headers @@ -0,0 +1 @@ +HTTP/1.1 403 Unauthorized diff --git a/chrome/test/test_browser_window.h b/chrome/test/test_browser_window.h index 61a25d0d586f50..e6a1bdfded1879 100644 --- a/chrome/test/test_browser_window.h +++ b/chrome/test/test_browser_window.h @@ -106,7 +106,7 @@ class TestBrowserWindow : public BrowserWindow { virtual void OpenTabpose() {} virtual void PrepareForInstant() {} virtual void ShowInstant(TabContents* preview_contents) {} - virtual void HideInstant() {} + virtual void HideInstant(bool instant_is_active) {} virtual gfx::Rect GetInstantBounds() { return gfx::Rect(); } #if defined(OS_CHROMEOS) virtual void ShowKeyboardOverlay(gfx::NativeWindow owning_window) {}