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}

4 files changed, 192 insertions, 0 deletions

diff --git a/ui/views/controls/webview/DEPS b/ui/views/controls/webview/DEPS
index 0fad725..70d0458 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 3e07f6f..e788500 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 af284b7..afd1ae2 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 0000000..12012ca
--- /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<content::TestBrowserContext> browser_context_;
+  scoped_ptr<WebViewTestViewsDelegate> 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<content::WebContentsImpl*>(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<content::WebContents> 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<content::WebContents> 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