Make TestRenderWidgetHostView::Show/Hide call through to RWHI

content/browser/web_contents/web_contents_impl_unittest.cc

 // Copyright (c) 2012 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 <stdint.h>
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/macros.h"
@@ skipping 139 matching lines @@
 
   void TestRenderViewTerminated(base::TerminationStatus status,
                                 int error_code) {
     RenderViewTerminated(GetMainFrame()->GetRenderViewHost(), status,
                          error_code);
   }
 
   bool is_showing() const {
     return static_cast<TestRenderWidgetHostView*>(
                GetMainFrame()->GetRenderViewHost()->GetWidget()->GetView())
-        ->is_showing();
+        ->IsShowing();
   }
 
   void ClearStates() {
     state_ = nullptr;
     deleted_ = nullptr;
     delegate_ = nullptr;
   }
 
   void CommandReceived() {
     command_received_count_++;
@@ skipping 1518 matching lines @@
   TestInterstitialPage::InterstitialState state =
       TestInterstitialPage::INVALID;
   bool deleted = false;
   GURL url2("http://interstitial");
   TestInterstitialPage* interstitial =
       new TestInterstitialPage(contents(), true, url2, &state, &deleted);
   TestInterstitialPageStateGuard state_guard(interstitial);
   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.
   interstitial->TestDidNavigate(1, interstitial_entry_id, true, url2);
   EXPECT_TRUE(interstitial->is_showing());
   EXPECT_TRUE(contents()->ShowingInterstitialPage());
   EXPECT_TRUE(contents()->GetInterstitialPage() == interstitial);
   entry = controller().GetVisibleEntry();
   ASSERT_NE(nullptr, entry);
   EXPECT_TRUE(entry->GetURL() == url2);
@@ skipping 27 matching lines @@
   TestInterstitialPage::InterstitialState state =
       TestInterstitialPage::INVALID;
   bool deleted = false;
   GURL url2("http://interstitial");
   TestInterstitialPage* interstitial =
       new TestInterstitialPage(contents(), true, url2, &state, &deleted);
   TestInterstitialPageStateGuard state_guard(interstitial);
   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.
   interstitial->TestDidNavigate(1, interstitial_entry_id, true, url2);
   EXPECT_TRUE(interstitial->is_showing());
   EXPECT_TRUE(contents()->ShowingInterstitialPage());
   EXPECT_TRUE(contents()->GetInterstitialPage() == interstitial);
   NavigationEntry* entry = controller().GetVisibleEntry();
   ASSERT_NE(nullptr, entry);
   EXPECT_TRUE(entry->GetURL() == url2);
@@ skipping 24 matching lines @@
   // Show an interstitial.
   TestInterstitialPage::InterstitialState state =
       TestInterstitialPage::INVALID;
   bool deleted = false;
   GURL url2("http://interstitial");
   TestInterstitialPage* interstitial =
       new TestInterstitialPage(contents(), false, url2, &state, &deleted);
   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.
   interstitial->TestDidNavigate(1, 0, true, url2);
   EXPECT_TRUE(interstitial->is_showing());
   EXPECT_TRUE(contents()->ShowingInterstitialPage());
   EXPECT_TRUE(contents()->GetInterstitialPage() == interstitial);
   NavigationEntry* entry = controller().GetVisibleEntry();
   ASSERT_NE(nullptr, entry);
   // The URL specified to the interstitial should have been ignored.
@@ skipping 31 matching lines @@
   TestInterstitialPage::InterstitialState state =
       TestInterstitialPage::INVALID;
   bool deleted = false;
   GURL url2("http://interstitial");
   TestInterstitialPage* interstitial =
       new TestInterstitialPage(contents(), true, url2, &state, &deleted);
   TestInterstitialPageStateGuard state_guard(interstitial);
   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.
   interstitial->TestDidNavigate(1, interstitial_entry_id, true, url2);
   EXPECT_TRUE(interstitial->is_showing());
   EXPECT_TRUE(contents()->ShowingInterstitialPage());
   EXPECT_TRUE(contents()->GetInterstitialPage() == interstitial);
   NavigationEntry* entry = controller().GetVisibleEntry();
   ASSERT_NE(nullptr, entry);
   EXPECT_TRUE(entry->GetURL() == url2);
@@ skipping 39 matching lines @@
   TestInterstitialPage::InterstitialState state =
       TestInterstitialPage::INVALID;
   bool deleted = false;
   GURL url2("http://interstitial");
   TestInterstitialPage* interstitial =
       new TestInterstitialPage(contents(), true, url2, &state, &deleted);
   TestInterstitialPageStateGuard state_guard(interstitial);
   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.
   interstitial->TestDidNavigate(1, interstitial_entry_id, true, url2);
   EXPECT_TRUE(interstitial->is_showing());
   EXPECT_TRUE(contents()->ShowingInterstitialPage());
   EXPECT_TRUE(contents()->GetInterstitialPage() == interstitial);
   NavigationEntry* entry = controller().GetVisibleEntry();
   ASSERT_NE(nullptr, entry);
   EXPECT_TRUE(entry->GetURL() == url2);
@@ skipping 36 matching lines @@
   // Show an interstitial.
   TestInterstitialPage::InterstitialState state =
       TestInterstitialPage::INVALID;
   bool deleted = false;
   GURL url2("http://interstitial");
   TestInterstitialPage* interstitial =
       new TestInterstitialPage(contents(), false, url2, &state, &deleted);
   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.
   interstitial->TestDidNavigate(1, 0, true, url2);
   EXPECT_TRUE(interstitial->is_showing());
   EXPECT_TRUE(contents()->ShowingInterstitialPage());
   EXPECT_TRUE(contents()->GetInterstitialPage() == interstitial);
   NavigationEntry* entry = controller().GetVisibleEntry();
   ASSERT_NE(nullptr, entry);
   // The URL specified to the interstitial should have been ignored.
@@ skipping 364 matching lines @@
                                &state, &deleted);
   TestInterstitialPageStateGuard state_guard(interstitial);
   interstitial->Show();
   int interstitial_entry_id = controller().GetTransientEntry()->GetUniqueID();
 
   // Let's simulate a navigation initiated from the browser before the
   // interstitial finishes loading.
   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);
 
   // Now let's make the interstitial navigation commit.
   interstitial->TestDidNavigate(1, interstitial_entry_id, true,
                                 interstitial_url);
 
   // After it loaded the interstitial should be gone.
   EXPECT_EQ(TestInterstitialPage::CANCELED, state);
 
@@ skipping 332 matching lines @@
 
 TEST_F(WebContentsImplTest, CapturerPreventsHiding) {
   const gfx::Size original_preferred_size(1024, 768);
   contents()->UpdatePreferredSize(original_preferred_size);
 
   TestRenderWidgetHostView* view = static_cast<TestRenderWidgetHostView*>(
       contents()->GetMainFrame()->GetRenderViewHost()->GetWidget()->GetView());
 
   // With no capturers, setting and un-setting occlusion should change the
   // view's occlusion state.
-  EXPECT_FALSE(view->is_showing());
+  EXPECT_TRUE(view->IsShowing());
+  contents()->WasHidden();
+  EXPECT_FALSE(view->IsShowing());
   contents()->WasShown();
-  EXPECT_TRUE(view->is_showing());
-  contents()->WasHidden();
-  EXPECT_FALSE(view->is_showing());
-  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) {
   const gfx::Size original_preferred_size(1024, 768);
   contents()->UpdatePreferredSize(original_preferred_size);
 
   TestRenderWidgetHostView* view = static_cast<TestRenderWidgetHostView*>(
       contents()->GetMainFrame()->GetRenderViewHost()->GetWidget()->GetView());
 
   // With no capturers, setting and un-setting occlusion should change the
@@ skipping 652 matching lines @@
   backend->AllowCertForHost(*cert, test_url.host(), 1);
   EXPECT_TRUE(backend->HasAllowException(test_url.host()));
 
   contents()->OnDidLoadResourceFromMemoryCache(test_url, "", "GET", "mime type",
                                                RESOURCE_TYPE_MAIN_FRAME);
 
   EXPECT_TRUE(backend->HasAllowException(test_url.host()));
 }
 
 }  // namespace content