From c3c0d91ae658c660f01705cd691b783e7f10d3f4 Mon Sep 17 00:00:00 2001 From: ananta Date: Tue, 16 Sep 2014 14:49:00 -0700 Subject: [PATCH] Relanding https://codereview.chromium.org/564553002/ with fixes for the unit test failures which required addition of some more plumbing in the form of dummy IO and file blocking threads. The IO thread is needed to ensure that the RenderWidgetHelper instances get freed corectly. When we switch tabs in chrome, the tab being switched away from gets hidden/shown/hidden. This occurs in the NativeViewHostAura::NativeViewDetaching code path where we first remove the clipping window which is the intermediate parent of the web contents view. The clipping window is hidden which causes the RWHVA::Hide function to get called which initiates the hiding sequence. Then the web contents view is reparented to the main view which is still visible. Now the RWHVA::Show function is called which initiates the show sequence. Eventually the main view is hidden, which then initiates the hide sequence. Addressed this with the following changes. 1. WebView::AttachWebContents and WebView::DetachWebContents now show and hide the webcontents native view. The WebContents is shown and hidden as before in WebContentsNativeViewAura::OnWindowVisibilityChanged. 2. Removed the WebContentsNativeViewAura::OnWindowParentChanged function. This function was present to show and hide the webcontents if the window was visible. This should not be needed with the change in #1 above. 3. Added a new file webview_unittest.cc. This contains the unittest WebViewUnitTest.TestWebViewAttachDetachWebContents This is run as part of unit_tests.exe. BUG=412989 R=sky Review URL: https://codereview.chromium.org/569153005 Cr-Commit-Position: refs/heads/master@{#295151} --- chrome/chrome_tests_unit.gypi | 4 + .../browser/web_contents/web_contents_view_aura.cc | 13 -- .../browser/web_contents/web_contents_view_aura.h | 2 - ui/views/controls/webview/DEPS | 2 + ui/views/controls/webview/webview.cc | 12 ++ ui/views/controls/webview/webview.gyp | 1 + ui/views/controls/webview/webview_unittest.cc | 177 +++++++++++++++++++++ 7 files changed, 196 insertions(+), 15 deletions(-) create mode 100644 ui/views/controls/webview/webview_unittest.cc diff --git a/chrome/chrome_tests_unit.gypi b/chrome/chrome_tests_unit.gypi index 8f577e85f8dd..a9c5f8f14805 100644 --- a/chrome/chrome_tests_unit.gypi +++ b/chrome/chrome_tests_unit.gypi @@ -2127,10 +2127,14 @@ 'dependencies': [ '../ui/wm/wm.gyp:wm', '../ui/aura/aura.gyp:aura_test_support', + '../ui/views/views.gyp:views_test_support', ], 'sources/': [ ['exclude', '^browser/ui/views/extensions/browser_action_drag_data_unittest.cc'], ], + 'sources': [ + '../ui/views/controls/webview/webview_unittest.cc', + ], }], ['use_aura==1 and use_ash==0 and use_ozone==0 and OS=="linux"', { 'dependencies': [ diff --git a/content/browser/web_contents/web_contents_view_aura.cc b/content/browser/web_contents/web_contents_view_aura.cc index ecbe02ca04e5..f242e7f149a8 100644 --- a/content/browser/web_contents/web_contents_view_aura.cc +++ b/content/browser/web_contents/web_contents_view_aura.cc @@ -1578,19 +1578,6 @@ int WebContentsViewAura::OnPerformDrop(const ui::DropTargetEvent& event) { return ConvertFromWeb(current_drag_op_); } -void WebContentsViewAura::OnWindowParentChanged(aura::Window* window, - aura::Window* parent) { - // Ignore any visibility changes in the hierarchy below. - if (window != window_.get() && window_->Contains(window)) - return; - - // On Windows we will get called with a parent of NULL as part of the shut - // down process. As such we do only change the visibility when a parent gets - // set. - if (parent) - UpdateWebContentsVisibility(window->IsVisible()); -} - void WebContentsViewAura::OnWindowVisibilityChanged(aura::Window* window, bool visible) { // Ignore any visibility changes in the hierarchy below. diff --git a/content/browser/web_contents/web_contents_view_aura.h b/content/browser/web_contents/web_contents_view_aura.h index 2d7df30a122e..98736047ea2d 100644 --- a/content/browser/web_contents/web_contents_view_aura.h +++ b/content/browser/web_contents/web_contents_view_aura.h @@ -178,8 +178,6 @@ class WebContentsViewAura virtual int OnPerformDrop(const ui::DropTargetEvent& event) OVERRIDE; // Overridden from aura::WindowObserver: - virtual void OnWindowParentChanged(aura::Window* window, - aura::Window* parent) OVERRIDE; virtual void OnWindowVisibilityChanged(aura::Window* window, bool visible) OVERRIDE; diff --git a/ui/views/controls/webview/DEPS b/ui/views/controls/webview/DEPS index 0fad725a16b4..70d04585a522 100644 --- a/ui/views/controls/webview/DEPS +++ b/ui/views/controls/webview/DEPS @@ -2,4 +2,6 @@ include_rules = [ "+content/public", "+ui/views", "+ui/web_dialogs", + "+content/browser/web_contents/web_contents_impl.h", + "+content/test/test_content_browser_client.h", ] diff --git a/ui/views/controls/webview/webview.cc b/ui/views/controls/webview/webview.cc index 3e07f6f42322..e7885005272b 100644 --- a/ui/views/controls/webview/webview.cc +++ b/ui/views/controls/webview/webview.cc @@ -13,6 +13,7 @@ #include "ipc/ipc_message.h" #include "ui/accessibility/ax_enums.h" #include "ui/accessibility/ax_view_state.h" +#include "ui/aura/window.h" #include "ui/base/ui_base_switches_util.h" #include "ui/events/event.h" #include "ui/views/accessibility/native_view_accessibility.h" @@ -301,6 +302,12 @@ void WebView::AttachWebContents() { OnBoundsChanged(bounds()); if (holder_->native_view() == view_to_attach) return; + + // Fullscreen widgets are not parented by a WebContentsView. Their visibility + // is controlled by content i.e. (RenderWidgetHost) + if (!is_embedding_fullscreen_widget_) + view_to_attach->Show(); + holder_->Attach(view_to_attach); // The view will not be focused automatically when it is attached, so we need @@ -320,6 +327,11 @@ void WebView::AttachWebContents() { void WebView::DetachWebContents() { if (web_contents()) { + // Fullscreen widgets are not parented by a WebContentsView. Their + // visibility is controlled by content i.e. (RenderWidgetHost). + if (!is_embedding_fullscreen_widget_) + web_contents()->GetNativeView()->Hide(); + holder_->Detach(); #if defined(OS_WIN) if (!is_embedding_fullscreen_widget_) diff --git a/ui/views/controls/webview/webview.gyp b/ui/views/controls/webview/webview.gyp index af284b7c3d41..afd1ae2053bd 100644 --- a/ui/views/controls/webview/webview.gyp +++ b/ui/views/controls/webview/webview.gyp @@ -12,6 +12,7 @@ 'target_name': 'webview', 'type': '<(component)', 'dependencies': [ + '../../../aura/aura.gyp:aura', '../../../../base/base.gyp:base', '../../../../base/base.gyp:base_i18n', '../../../../base/third_party/dynamic_annotations/dynamic_annotations.gyp:dynamic_annotations', diff --git a/ui/views/controls/webview/webview_unittest.cc b/ui/views/controls/webview/webview_unittest.cc new file mode 100644 index 000000000000..12012ca13e02 --- /dev/null +++ b/ui/views/controls/webview/webview_unittest.cc @@ -0,0 +1,177 @@ +// Copyright 2014 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "ui/views/controls/webview/webview.h" + +#include "base/memory/scoped_ptr.h" +#include "content/browser/web_contents/web_contents_impl.h" +#include "content/public/browser/web_contents_observer.h" +#include "content/public/test/test_browser_context.h" +#include "content/public/test/test_browser_thread.h" +#include "content/public/test/web_contents_tester.h" +#include "content/test/test_content_browser_client.h" +#include "ui/aura/window.h" +#include "ui/views/test/test_views_delegate.h" +#include "ui/views/test/widget_test.h" + +namespace { + +// Provides functionality to create a test WebContents. +class WebViewTestViewsDelegate : public views::TestViewsDelegate { + public: + WebViewTestViewsDelegate() {} + virtual ~WebViewTestViewsDelegate() {} + + // Overriden from TestViewsDelegate. + virtual content::WebContents* CreateWebContents( + content::BrowserContext* browser_context, + content::SiteInstance* site_instance) OVERRIDE { + return content::WebContentsTester::CreateTestWebContents(browser_context, + site_instance); + } + + private: + DISALLOW_COPY_AND_ASSIGN(WebViewTestViewsDelegate); +}; + +// Provides functionality to test a WebView. +class WebViewUnitTest : public views::test::WidgetTest { + public: + WebViewUnitTest() + : ui_thread_(content::BrowserThread::UI, base::MessageLoop::current()), + file_blocking_thread_(content::BrowserThread::FILE_USER_BLOCKING, + base::MessageLoop::current()), + io_thread_(content::BrowserThread::IO, base::MessageLoop::current()) {} + + virtual ~WebViewUnitTest() {} + + virtual void SetUp() OVERRIDE { + // The ViewsDelegate is deleted when the ViewsTestBase class is torn down. + WidgetTest::set_views_delegate(new WebViewTestViewsDelegate); + browser_context_.reset(new content::TestBrowserContext); + WidgetTest::SetUp(); + // Set the test content browser client to avoid pulling in needless + // dependencies from content. + SetBrowserClientForTesting(&test_browser_client_); + } + + virtual void TearDown() OVERRIDE { + browser_context_.reset(NULL); + // Flush the message loop to execute pending relase tasks as this would + // upset ASAN and Valgrind. + RunPendingMessages(); + WidgetTest::TearDown(); + } + + protected: + content::BrowserContext* browser_context() { return browser_context_.get(); } + + private: + content::TestBrowserThread ui_thread_; + content::TestBrowserThread file_blocking_thread_; + content::TestBrowserThread io_thread_; + scoped_ptr browser_context_; + scoped_ptr views_delegate_; + content::TestContentBrowserClient test_browser_client_; + + DISALLOW_COPY_AND_ASSIGN(WebViewUnitTest); +}; + +// Provides functionaity to observe events on a WebContents like WasShown/ +// WasHidden/WebContentsDestroyed. +class WebViewTestWebContentsObserver : public content::WebContentsObserver { + public: + WebViewTestWebContentsObserver(content::WebContents* web_contents) + : web_contents_(static_cast(web_contents)), + was_shown_(false), + shown_count_(0), + hidden_count_(0) { + content::WebContentsObserver::Observe(web_contents); + } + + virtual ~WebViewTestWebContentsObserver() { + if (web_contents_) + content::WebContentsObserver::Observe(NULL); + } + + virtual void WebContentsDestroyed() OVERRIDE { + DCHECK(web_contents_); + content::WebContentsObserver::Observe(NULL); + web_contents_ = NULL; + } + + virtual void WasShown() OVERRIDE { + was_shown_ = true; + ++shown_count_; + } + + virtual void WasHidden() OVERRIDE { + was_shown_ = false; + ++hidden_count_; + } + + bool was_shown() const { return was_shown_; } + + int shown_count() const { return shown_count_; } + + int hidden_count() const { return hidden_count_; } + + private: + content::WebContentsImpl* web_contents_; + bool was_shown_; + int32 shown_count_; + int32 hidden_count_; + + DISALLOW_COPY_AND_ASSIGN(WebViewTestWebContentsObserver); +}; + +// Tests that attaching and detaching a WebContents to a WebView makes the +// WebContents visible and hidden respectively. +TEST_F(WebViewUnitTest, TestWebViewAttachDetachWebContents) { + // Create a top level widget and a webview as its content. + views::Widget* widget = CreateTopLevelFramelessPlatformWidget(); + widget->SetBounds(gfx::Rect(0, 10, 100, 100)); + views::WebView* webview = new views::WebView(browser_context()); + widget->SetContentsView(webview); + widget->Show(); + + // Case 1: Create a new WebContents and set it in the webview via + // SetWebContents. This should make the WebContents visible. + content::WebContents::CreateParams params(browser_context()); + scoped_ptr web_contents1( + content::WebContents::Create(params)); + WebViewTestWebContentsObserver observer1(web_contents1.get()); + EXPECT_FALSE(observer1.was_shown()); + + webview->SetWebContents(web_contents1.get()); + EXPECT_TRUE(observer1.was_shown()); + EXPECT_TRUE(web_contents1->GetNativeView()->IsVisible()); + EXPECT_EQ(observer1.shown_count(), 1); + EXPECT_EQ(observer1.hidden_count(), 0); + + // Case 2: Create another WebContents and replace the current WebContents + // via SetWebContents(). This should hide the current WebContents and show + // the new one. + content::WebContents::CreateParams params2(browser_context()); + scoped_ptr web_contents2( + content::WebContents::Create(params2)); + WebViewTestWebContentsObserver observer2(web_contents2.get()); + EXPECT_FALSE(observer2.was_shown()); + + // Setting the new WebContents should hide the existing one. + webview->SetWebContents(web_contents2.get()); + EXPECT_FALSE(observer1.was_shown()); + EXPECT_TRUE(observer2.was_shown()); + + // WebContents1 should not get stray show calls when WebContents2 is set. + EXPECT_EQ(observer1.shown_count(), 1); + EXPECT_EQ(observer1.hidden_count(), 1); + EXPECT_EQ(observer2.shown_count(), 1); + EXPECT_EQ(observer2.hidden_count(), 0); + + widget->Close(); + RunPendingMessages(); +} + +} // namespace -- 2.11.4.GIT