Issue metadata
Sign in to add a comment
|
Interstitials confused about whether or not they are showing |
||||||||||||||||||||||
Issue descriptionIn https://codereview.chromium.org/1713473002#ps80001 I'm making TestRenderWidgetHostView correctly propagate show/hide state. It revealed that many interstitial tests in WebContentsImplTest were incorrectly doing EXPECT_FALSE(interstitial->is_showing()); when actually the interstitial's underlying RenderWidgetHostImpl does think it's showing (is_hidden() is false). sievers@'s reaction was: > > https://codereview.chromium.org/1713473002/diff/80001/content/browser/web_contents/web_contents_impl_unittest.cc#newcode1699 > > content/browser/web_contents/web_contents_impl_unittest.cc:1699: > > EXPECT_TRUE(interstitial->is_showing()); > 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. > > 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. So I've filed this bug for someone knowledgeable to ponder whether this has revealed any important underlying bugs about interstitial visibility, or is actually fine. nasko@: tentatively assigning to you since you seem to have touched the interstitial code most recently, but please reassign if there's someone more appropriate. Thanks!
,
Aug 12 2016
,
Aug 13 2016
oh blerg. nasko, can you take this on or should one of us interstitial-users pick it up?
,
Aug 16 2016
I won't have time to look at this until early September, so if anyone can help chime in, that will be awesome.
,
Sep 6 2016
,
Nov 22 2016
,
Dec 7 2016
,
Feb 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1c496374204313ae4233bc19c6495c8c2b0ee660 commit 1c496374204313ae4233bc19c6495c8c2b0ee660 Author: johnme <johnme@chromium.org> Date: Fri Feb 10 13:54:11 2017 Make TestRenderWidgetHostView::Show/Hide call through to RWHI Before this patch, after calling test_web_contents->WasHidden() on a TestWebContents, test_web_contents->GetMainFrame()->GetVisibilityState() would still return blink::WebPageVisibilityStateVisible, because TestRenderWidgetHostView::Hide didn't propagate the WasHidden to RenderWidgetHostImpl (unlike all the non-test implementations of RenderWidgetHostView). This patch fixes that. There don't seem to be significant downsides to propagating the WasHidden to RWHI; it tries to send a few IPC messages, but since Send is stubbed out, those are simply discarded. I did have to add a null check in RenderWidgetHostViewBase::GetPhysicalBackingSize (called from RWHI::GetResizeParams, called from RWHI::WasResized, called from RWHI::WasShown), since gfx::Screen::GetScreen() sometimes returns null in unit tests. And I had to update some of the web_contents_impl_unittest.cc tests, which were expecting interstitials etc to be hidden, even though RWHI generally defaults to visible (it would be possible to instead keep the is_showing_ bool in TestRenderWidgetHostView, but it seems weird to allow TRWHV and RWHI to disagree about whether they are visible. Also there's one test that manages to hit a null screen on navigation, so I added a TestScreen to ExtensionContextMenuModelTest.TestPageAccessSubmenu. BUG=577336,636953 Review-Url: https://codereview.chromium.org/1713473002 Cr-Commit-Position: refs/heads/master@{#449606} [modify] https://crrev.com/1c496374204313ae4233bc19c6495c8c2b0ee660/chrome/browser/extensions/extension_context_menu_model_unittest.cc [modify] https://crrev.com/1c496374204313ae4233bc19c6495c8c2b0ee660/chrome/browser/push_messaging/push_messaging_notification_manager_unittest.cc [modify] https://crrev.com/1c496374204313ae4233bc19c6495c8c2b0ee660/components/visitedlink/test/visitedlink_unittest.cc [modify] https://crrev.com/1c496374204313ae4233bc19c6495c8c2b0ee660/content/browser/web_contents/web_contents_impl_unittest.cc [modify] https://crrev.com/1c496374204313ae4233bc19c6495c8c2b0ee660/content/public/test/test_renderer_host.h [modify] https://crrev.com/1c496374204313ae4233bc19c6495c8c2b0ee660/content/test/test_render_view_host.cc [modify] https://crrev.com/1c496374204313ae4233bc19c6495c8c2b0ee660/content/test/test_render_view_host.h
,
Feb 11 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f3b00fa5e74ebdcdf63419a8c8b1e2cfe7d7024f commit f3b00fa5e74ebdcdf63419a8c8b1e2cfe7d7024f Author: bsep <bsep@chromium.org> Date: Sat Feb 11 00:06:38 2017 Revert of Make TestRenderWidgetHostView::Show/Hide call through to RWHI (patchset #9 id:160001 of https://codereview.chromium.org/1713473002/ ) Reason for revert: Speculative revert. Many tests are crashing on CrOS, yours is the only plausible CL. Apologizes in advance if it turns out to be the wrong CL. Example failure: https://uberchromegw.corp.google.com/i/chromium.chromiumos/builders/Linux%20ChromiumOS%20Tests%20%28dbg%29%281%29/builds/22356 Original issue's description: > Make TestRenderWidgetHostView::Show/Hide call through to RWHI > > Before this patch, after calling test_web_contents->WasHidden() on a > TestWebContents, test_web_contents->GetMainFrame()->GetVisibilityState() > would still return blink::WebPageVisibilityStateVisible, because > TestRenderWidgetHostView::Hide didn't propagate the WasHidden to > RenderWidgetHostImpl (unlike all the non-test implementations of > RenderWidgetHostView). > > This patch fixes that. There don't seem to be significant downsides to > propagating the WasHidden to RWHI; it tries to send a few IPC messages, > but since Send is stubbed out, those are simply discarded. > > I did have to add a null check in > RenderWidgetHostViewBase::GetPhysicalBackingSize (called from > RWHI::GetResizeParams, called from RWHI::WasResized, called from > RWHI::WasShown), since gfx::Screen::GetScreen() sometimes returns null > in unit tests. > > And I had to update some of the web_contents_impl_unittest.cc tests, > which were expecting interstitials etc to be hidden, even though RWHI > generally defaults to visible (it would be possible to instead keep the > is_showing_ bool in TestRenderWidgetHostView, but it seems weird to > allow TRWHV and RWHI to disagree about whether they are visible. > > Also there's one test that manages to hit a null screen on navigation, > so I added a TestScreen to > ExtensionContextMenuModelTest.TestPageAccessSubmenu. > > BUG=577336,636953 > > Review-Url: https://codereview.chromium.org/1713473002 > Cr-Commit-Position: refs/heads/master@{#449606} > Committed: https://chromium.googlesource.com/chromium/src/+/1c496374204313ae4233bc19c6495c8c2b0ee660 TBR=sievers@chromium.org,boliu@chromium.org,sky@chromium.org,johnme@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=577336,636953 Review-Url: https://codereview.chromium.org/2684993011 Cr-Commit-Position: refs/heads/master@{#449797} [modify] https://crrev.com/f3b00fa5e74ebdcdf63419a8c8b1e2cfe7d7024f/chrome/browser/extensions/extension_context_menu_model_unittest.cc [modify] https://crrev.com/f3b00fa5e74ebdcdf63419a8c8b1e2cfe7d7024f/chrome/browser/push_messaging/push_messaging_notification_manager_unittest.cc [modify] https://crrev.com/f3b00fa5e74ebdcdf63419a8c8b1e2cfe7d7024f/components/visitedlink/test/visitedlink_unittest.cc [modify] https://crrev.com/f3b00fa5e74ebdcdf63419a8c8b1e2cfe7d7024f/content/browser/web_contents/web_contents_impl_unittest.cc [modify] https://crrev.com/f3b00fa5e74ebdcdf63419a8c8b1e2cfe7d7024f/content/public/test/test_renderer_host.h [modify] https://crrev.com/f3b00fa5e74ebdcdf63419a8c8b1e2cfe7d7024f/content/test/test_render_view_host.cc [modify] https://crrev.com/f3b00fa5e74ebdcdf63419a8c8b1e2cfe7d7024f/content/test/test_render_view_host.h
,
Nov 10 2017
,
Feb 18 2018
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by benwells@chromium.org
, Aug 12 2016Status: Assigned (was: Untriaged)