Make TestRenderWidgetHostView::Show/Hide call through to RWHI

content/browser/web_contents/web_contents_impl_unittest.cc

Index: content/browser/web_contents/web_contents_impl_unittest.cc
diff --git a/content/browser/web_contents/web_contents_impl_unittest.cc b/content/browser/web_contents/web_contents_impl_unittest.cc
index 5464042094821e5d93ca97ed05469898b28d9013..dc8c29b1486143fc873cd8a423eceaea2854037a 100644
--- a/content/browser/web_contents/web_contents_impl_unittest.cc
+++ b/content/browser/web_contents/web_contents_impl_unittest.cc
@@ -157,7 +157,7 @@ class TestInterstitialPage : public InterstitialPageImpl {
   bool is_showing() const {
     return static_cast<TestRenderWidgetHostView*>(
                GetMainFrame()->GetRenderViewHost()->GetWidget()->GetView())
-        ->is_showing();
+        ->IsShowing();
   }
 
   void ClearStates() {
@@ -1696,7 +1696,7 @@ TEST_F(WebContentsImplTest,
   interstitial->Show();
   int interstitial_entry_id = controller().GetTransientEntry()->GetUniqueID();
   // The interstitial should not show until its navigation has committed.
-  EXPECT_FALSE(interstitial->is_showing());
+  EXPECT_TRUE(interstitial->is_showing());

[no sievers, 2016/02/18 21:22:09]

Agreed that this was hardcoding the wrong expectations.
The interstitial's RWHV will be created with the current visibility of the
WebContents here (see RenderFrameHostManager::Init).

There is some confusion here about the notion of 'showing'. It mainly seems to
want to test whether the interstitial RWHV is the active one (calling Show()
above and then checking !is_showing() is also a bit nonsensical in that regard).

So maybe we should also check EXPECT_FALSE(contents()->ShowingInterstitialPage)
here instead. That would also still match the comment then.

But if you look at InterstitialPageImpl::DidNavigate() I wonder if your patch is
also uncovering a glitch:

 // The RenderViewHost has loaded its contents, we can show it now.
  if (!controller_->delegate()->IsHidden())
    render_view_host_->GetWidget()->GetView()->Show();
  controller_->delegate()->AttachInterstitialPage(this);

So in a way we do actually expect the RWHV itself to not be visible either until
the commit. But that would need to somehow explicitly create the RWHV with
initial state being hidden from InterstitialPageImpl::CreateWebContentsView(),
which I don't see happen. But it's still a bit nonsensical that
InterstitialPageImpl::Show() doesn't show anything but creates a hidden view
instead.

I'm a bit surprised that this actually works on all platforms, because
RWHV::Show() might just create a window that shows on top (before the navigation
is committed). So it's possible that we think we are delaying showing it
(because it's not current in the RFHost), but the platform window is actually
showing before the commit. I mean, if you call Show() on the RWHV, then you
should expect it to be showing :)

Also note the ugly logic in WebContentsViewAura::CreateViewForWidget() that
overrides behavior in tests to do something very different. I hope it doesn't
further mess with what we're testing here.

[no sievers, 2016/02/18 21:29:43]

Btw feel free to file a separate bug for someone very knowledgeable of the
interstitial code to investigate this, but I suggest keeping the expectation as
is by changing it to contents()->ShowingInterstitialPage() here and below.

[johnme, 2016/08/11 14:11:44]

I've filed https://crbug.com/636953 to ask someone knowledgeable to look into
this.

The tests are actually already checking contents()->ShowingInterstitialPage()
before and after, so I've just removed the confusing
EXPECT_FALSE(interstitial->is_showing()) lines for now (I left in the
EXPECT_TRUE ones afterwards, since those aren't confusing, though the value of
that hasn't actually changed.

   EXPECT_FALSE(contents()->ShowingInterstitialPage());
   EXPECT_EQ(nullptr, contents()->GetInterstitialPage());
   // Let's commit the interstitial navigation.
@@ -1744,7 +1744,7 @@ TEST_F(WebContentsImplTest,
   interstitial->Show();
   int interstitial_entry_id = controller().GetTransientEntry()->GetUniqueID();
   // The interstitial should not show until its navigation has committed.
-  EXPECT_FALSE(interstitial->is_showing());
+  EXPECT_TRUE(interstitial->is_showing());
   EXPECT_FALSE(contents()->ShowingInterstitialPage());
   EXPECT_EQ(nullptr, contents()->GetInterstitialPage());
   // Let's commit the interstitial navigation.
@@ -1789,7 +1789,7 @@ TEST_F(WebContentsImplTest, ShowInterstitialNoNewNavigationDontProceed) {
   TestInterstitialPageStateGuard state_guard(interstitial);
   interstitial->Show();
   // The interstitial should not show until its navigation has committed.
-  EXPECT_FALSE(interstitial->is_showing());
+  EXPECT_TRUE(interstitial->is_showing());
   EXPECT_FALSE(contents()->ShowingInterstitialPage());
   EXPECT_EQ(nullptr, contents()->GetInterstitialPage());
   // Let's commit the interstitial navigation.
@@ -1841,7 +1841,7 @@ TEST_F(WebContentsImplTest,
   interstitial->Show();
   int interstitial_entry_id = controller().GetTransientEntry()->GetUniqueID();
   // The interstitial should not show until its navigation has committed.
-  EXPECT_FALSE(interstitial->is_showing());
+  EXPECT_TRUE(interstitial->is_showing());
   EXPECT_FALSE(contents()->ShowingInterstitialPage());
   EXPECT_EQ(nullptr, contents()->GetInterstitialPage());
   // Let's commit the interstitial navigation.
@@ -1901,7 +1901,7 @@ TEST_F(WebContentsImplTest,
   interstitial->Show();
   int interstitial_entry_id = controller().GetTransientEntry()->GetUniqueID();
   // The interstitial should not show until its navigation has committed.
-  EXPECT_FALSE(interstitial->is_showing());
+  EXPECT_TRUE(interstitial->is_showing());
   EXPECT_FALSE(contents()->ShowingInterstitialPage());
   EXPECT_EQ(nullptr, contents()->GetInterstitialPage());
   // Let's commit the interstitial navigation.
@@ -1958,7 +1958,7 @@ TEST_F(WebContentsImplTest, ShowInterstitialNoNewNavigationProceed) {
   TestInterstitialPageStateGuard state_guard(interstitial);
   interstitial->Show();
   // The interstitial should not show until its navigation has committed.
-  EXPECT_FALSE(interstitial->is_showing());
+  EXPECT_TRUE(interstitial->is_showing());
   EXPECT_FALSE(contents()->ShowingInterstitialPage());
   EXPECT_EQ(nullptr, contents()->GetInterstitialPage());
   // Let's commit the interstitial navigation.
@@ -2343,7 +2343,7 @@ TEST_F(WebContentsImplTest, NavigateBeforeInterstitialShows) {
   const GURL url("http://www.google.com");
   controller().LoadURL(
       url, Referrer(), ui::PAGE_TRANSITION_TYPED, std::string());
-  EXPECT_FALSE(interstitial->is_showing());
+  EXPECT_FALSE(contents()->ShowingInterstitialPage());
   RunAllPendingInMessageLoop();
   ASSERT_FALSE(deleted);
 
@@ -2696,22 +2696,20 @@ TEST_F(WebContentsImplTest, CapturerPreventsHiding) {
 
   // With no capturers, setting and un-setting occlusion should change the
   // view's occlusion state.
-  EXPECT_FALSE(view->is_showing());
-  contents()->WasShown();
-  EXPECT_TRUE(view->is_showing());
+  EXPECT_TRUE(view->IsShowing());
   contents()->WasHidden();
-  EXPECT_FALSE(view->is_showing());
+  EXPECT_FALSE(view->IsShowing());
   contents()->WasShown();
-  EXPECT_TRUE(view->is_showing());
+  EXPECT_TRUE(view->IsShowing());
 
   // Add a capturer and try to hide the contents. The view will remain visible.
   contents()->IncrementCapturerCount(gfx::Size());
   contents()->WasHidden();
-  EXPECT_TRUE(view->is_showing());
+  EXPECT_TRUE(view->IsShowing());
 
   // Remove the capturer, and the WasHidden should take effect.
   contents()->DecrementCapturerCount();
-  EXPECT_FALSE(view->is_showing());
+  EXPECT_FALSE(view->IsShowing());
 }
 
 TEST_F(WebContentsImplTest, CapturerPreventsOcclusion) {