|
|
Created:
4 years, 10 months ago by johnme Modified:
3 years, 10 months ago CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@crash Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake 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
Patch Set 1 #Patch Set 2 : Fix typo #Patch Set 3 : Fix NPE #Patch Set 4 : Remove is_showing_ #Patch Set 5 : Update web_contents_impl_unittest.cc #
Total comments: 3
Patch Set 6 : Remove misleading EXPECT_TRUE(interstitial->is_showing()); #Patch Set 7 : Rebase #Patch Set 8 : Fix ExtensionContextMenuModelTest.TestPageAccessSubmenu #
Total comments: 5
Patch Set 9 : Tentatively undo render_widget_host_view_base.cc modification #
Dependent Patchsets: Messages
Total messages: 67 (39 generated)
The CQ bit was checked by johnme@chromium.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by johnme@chromium.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by johnme@chromium.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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@chromium.org changed reviewers: + sievers@chromium.org
The CQ bit was checked by johnme@chromium.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by johnme@chromium.org to run a CQ dry run
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 ==========
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
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 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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.
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.
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 ==========
The CQ bit was checked by johnme@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
thanks, lgtm!
The CQ bit was checked by johnme@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by johnme@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
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.
The CQ bit was checked by johnme@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
johnme@chromium.org changed reviewers: + boliu@chromium.org, sky@chromium.org
sky: Please review the overall patch, since sievers is no longer an OWNER. boliu: Please review content/browser/ Thanks :)
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
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?
The CQ bit was checked by johnme@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
The CQ bit was checked by johnme@chromium.org
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 160001, "attempt_start_ts": 1486734593819780, "parent_rev": "7e9684dde3d74662e43ec427afd9ab55c8d6138d", "commit_rev": "1c496374204313ae4233bc19c6495c8c2b0ee660"}
Message was sent while issue was closed.
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... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/1c496374204313ae4233bc19c649...
Message was sent while issue was closed.
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....
Message was sent while issue was closed.
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. |