Make TestRenderWidgetHostView::Show/Hide call through to RWHI

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

[johnme, 2016-02-18 10:34:06]

The CQ bit was checked by johnme@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-18 10:35:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1713473002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1713473002/1

[commit-bot: I haz the power, 2016-02-18 10:46:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-18 10:46:47]

Dry run: Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[johnme, 2016-02-18 10:48:04]

The CQ bit was checked by johnme@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-18 10:48:56]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1713473002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1713473002/20001

[commit-bot: I haz the power, 2016-02-18 11:18:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-18 11:18:33]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[johnme, 2016-02-18 11:57:57]

The CQ bit was checked by johnme@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-18 11:58:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1713473002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1713473002/40001

[commit-bot: I haz the power, 2016-02-18 13:01:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-18 13:01:54]

Dry run: This issue passed the CQ dry run.

[johnme, 2016-02-18 14:12:45]

Description was changed from

==========
Make TestRenderWidgetHostView::Show/Hide call through to RWHI

BUG=
==========

to

==========
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.

BUG=577336
==========

[johnme, 2016-02-18 14:14:10]

johnme@chromium.org changed reviewers:
+ sievers@chromium.org

[johnme, 2016-02-18 14:14:21]

The CQ bit was checked by johnme@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-18 14:14:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1713473002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1713473002/60001

[commit-bot: I haz the power, 2016-02-18 14:45:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-18 14:45:08]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[johnme, 2016-02-18 17:30:05]

The CQ bit was checked by johnme@chromium.org to run a CQ dry run

[johnme, 2016-02-18 17:33:51]

Description was changed from

==========
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.

BUG=577336
==========

to

==========
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.

BUG=577336
==========

[commit-bot: I haz the power, 2016-02-18 17:33:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1713473002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1713473002/80001

[johnme, 2016-02-18 17:34:56]

Description was changed from

==========
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.

BUG=577336
==========

to

==========
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.

BUG=577336
==========

[commit-bot: I haz the power, 2016-02-18 18:45:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-18 18:45:48]

Dry run: This issue passed the CQ dry run.

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

https://codereview.chromium.org/1713473002/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl_unittest.cc (right):

https://codereview.chromium.org/1713473002/diff/80001/content/browser/web_con...
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.

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

https://codereview.chromium.org/1713473002/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl_unittest.cc (right):

https://codereview.chromium.org/1713473002/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_unittest.cc:1699:
EXPECT_TRUE(interstitial->is_showing());
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:07:14]

Description was changed from

==========
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.

BUG=577336
==========

to

==========
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.

BUG=577336,636953
==========

[johnme, 2016-08-11 14:07:43]

The CQ bit was checked by johnme@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-11 14:08:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

Sorry, I put this patch on hold as it seemed like I was going down a
rabbit-hole, but let's try to resurrect it.

https://codereview.chromium.org/1713473002/diff/80001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl_unittest.cc (right):

https://codereview.chromium.org/1713473002/diff/80001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_unittest.cc:1699:
EXPECT_TRUE(interstitial->is_showing());
On 2016/02/18 21:29:43, sievers wrote:
> 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.

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.

[commit-bot: I haz the power, 2016-08-11 14:58:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-11 14:58:58]

Dry run: This issue passed the CQ dry run.

[no sievers, 2016-08-16 21:51:41]

thanks, lgtm!

[johnme, 2016-08-17 00:54:37]

The CQ bit was checked by johnme@chromium.org

[commit-bot: I haz the power, 2016-08-17 00:54:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-17 01:27:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-17 01:27:31]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[johnme, 2017-02-08 16:34:00]

The CQ bit was checked by johnme@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 16:34:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-08 17:11:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 17:11:50]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[johnme, 2017-02-08 18:32:54]

Added a TestScreen to ExtensionContextMenuModelTest.TestPageAccessSubmenu since
otherwise it tries to dereference a null screen at the following stacktrace:

content::(anonymous namespace)::GetScreenInfoForWindow()
content::RenderWidgetHostImpl::GetScreenInfo()
content::RenderWidgetHostImpl::GetResizeParams()
content::RenderWidgetHostImpl::WasResized()
content::RenderWidgetHostImpl::WasShown()
content::TestRenderWidgetHostView::Show()
content::RenderFrameHostManager::CommitPending()
content::RenderFrameHostManager::CommitPendingIfNecessary()
content::RenderFrameHostManager::DidNavigateFrame()
content::NavigatorImpl::DidNavigate()
content::RenderFrameHostImpl::OnDidCommitProvisionalLoad()
content::TestRenderFrameHost::SendNavigateWithParams()
content::TestRenderFrameHost::SendNavigateWithParameters()
content::TestRenderFrameHost::SendNavigateWithTransition()
content::TestWebContents::CommitPendingNavigation()
content::TestWebContents::NavigateAndCommit()
extensions::ExtensionContextMenuModelTest_TestPageAccessSubmenu_Test::TestBody()

It feels a bit ugly that navigating now causes this test to require a
TestScreen, but the fact that it only affects this one test is rather
reassuring.

[johnme, 2017-02-08 18:35:55]

The CQ bit was checked by johnme@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 18:36:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[johnme, 2017-02-08 19:52:15]

Description was changed from

==========
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.

BUG=577336,636953
==========

to

==========
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
==========

[commit-bot: I haz the power, 2017-02-08 19:56:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-08 19:56:38]

Dry run: This issue passed the CQ dry run.

[johnme, 2017-02-08 19:56:38]

johnme@chromium.org changed reviewers:
+ boliu@chromium.org, sky@chromium.org

[johnme, 2017-02-08 19:56:39]

sky: Please review the overall patch, since sievers is no longer an OWNER.

boliu: Please review content/browser/

Thanks :)

[boliu, 2017-02-08 20:13:34]

https://codereview.chromium.org/1713473002/diff/140001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.cc (right):

https://codereview.chromium.org/1713473002/diff/140001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.cc:107: if (!screen)
I'm curious which tests and which platform had had this problem?

Maybe should fix test harness rather than doing this.

If we don't end up doing that, definitely add a comment that the check is for
tests

[sky, 2017-02-08 23:21:36]

https://codereview.chromium.org/1713473002/diff/140001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.cc (right):

https://codereview.chromium.org/1713473002/diff/140001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.cc:107: if (!screen)
On 2017/02/08 20:13:34, boliu wrote:
> I'm curious which tests and which platform had had this problem?
> 
> Maybe should fix test harness rather than doing this.
> 
> If we don't end up doing that, definitely add a comment that the check is for
> tests

+1 to this. I'm mildly worried this is going to cause problems down the line and
it would be better to fix it now.

https://codereview.chromium.org/1713473002/diff/140001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl_unittest.cc (left):

https://codereview.chromium.org/1713473002/diff/140001/content/browser/web_co...
content/browser/web_contents/web_contents_impl_unittest.cc:1687:
EXPECT_FALSE(interstitial->is_showing());
Why are the interstitial assertions no longer interesting?

[johnme, 2017-02-09 17:55:32]

The CQ bit was checked by johnme@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 17:55:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[johnme, 2017-02-09 18:59:29]

https://codereview.chromium.org/1713473002/diff/140001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_base.cc (right):

https://codereview.chromium.org/1713473002/diff/140001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_base.cc:107: if (!screen)
On 2017/02/08 23:21:36, sky wrote:
> On 2017/02/08 20:13:34, boliu wrote:
> > I'm curious which tests and which platform had had this problem?
> > 
> > Maybe should fix test harness rather than doing this.
> > 
> > If we don't end up doing that, definitely add a comment that the check is
for
> > tests
> 
> +1 to this. I'm mildly worried this is going to cause problems down the line
and
> it would be better to fix it now.

Good news - it turns out this change to GetPhysicalBackingSize is no longer
necessary! It seems some refactoring has happened since I originally wrote this
patch that makes this more robust.

(I can't remember exactly which tests this was causing to fail, but you can see
which platforms were failing by looking at the try results for patch set 2:
Windows, Linux, and Chrome OS).

https://codereview.chromium.org/1713473002/diff/140001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl_unittest.cc (left):

https://codereview.chromium.org/1713473002/diff/140001/content/browser/web_co...
content/browser/web_contents/web_contents_impl_unittest.cc:1687:
EXPECT_FALSE(interstitial->is_showing());
On 2017/02/08 23:21:36, sky wrote:
> Why are the interstitial assertions no longer interesting?

They were misleading. Outside of tests, is_showing() is actually true for
interstitials in this state, and once this patch lands it'll be true here too.
It was only false here due to the issue that this patch is intended to resolve,
i.e. that TestRenderWidgetHostView::Show/Hide failed to propagate through to
RWHI. I filed https://crbug.com/636953 to follow up on this in case it points to
a bug in interstitials.

[boliu, 2017-02-09 19:50:51]

lgtm

[commit-bot: I haz the power, 2017-02-09 20:25:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-09 20:25:17]

Dry run: This issue passed the CQ dry run.

[sky, 2017-02-09 21:01:37]

LGTM

[johnme, 2017-02-10 13:49:53]

The CQ bit was checked by johnme@chromium.org

[johnme, 2017-02-10 13:49:54]

The patchset sent to the CQ was uploaded after l-g-t-m from sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1713473002/#ps160001
(title: "Tentatively undo render_widget_host_view_base.cc modification")

[commit-bot: I haz the power, 2017-02-10 13:50:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-10 13:54:15]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1486734593819780,
"parent_rev": "7e9684dde3d74662e43ec427afd9ab55c8d6138d", "commit_rev":
"1c496374204313ae4233bc19c6495c8c2b0ee660"}

[commit-bot: I haz the power, 2017-02-10 13:54:51]

Description was changed from

==========
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
==========

to

==========
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/+/1c496374204313ae4233bc19c649...
==========

[commit-bot: I haz the power, 2017-02-10 13:54:52]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/1c496374204313ae4233bc19c649...

[Bret, 2017-02-11 00:05:25]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/2684993011/ by bsep@chromium.org.

The reason for reverting is: 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%20C....

[Bret, 2017-02-11 02:22:26]

On 2017/02/11 00:05:25, Bret Sepulveda wrote:
> A revert of this CL (patchset #9 id:160001) has been created in
> https://codereview.chromium.org/2684993011/ by mailto:bsep@chromium.org.
> 
> The reason for reverting is: 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%20C....

Confirmed that this CL was the issue.