Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2828023002:
Set GPU rasterization trigger for OOPIFs (Closed)
DescriptionSet GPU rasterization trigger for OOPIFs
Currently the GPU rasterization trigger is never set on the compositor
for an out-of-process iframe. This patch changes it so that it is always
set.
BUG=712794
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2828023002
Cr-Commit-Position: refs/heads/master@{#467130}
Committed: https://chromium.googlesource.com/chromium/src/+/1931faea0d5ada216fca3c11add153ceaa82ba6d
Patch Set 1 #Patch Set 2 : Update based on comments #Patch Set 3 : Removed bad DCHECK #Patch Set 4 : Back to first fix #Messages
Total messages: 31 (20 generated)
The CQ bit was checked by kenrb@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...
kenrb@chromium.org changed reviewers: + ericrk@chromium.org
ericrk: Does this look reasonable to you? I don't see a good way to test this. Since it is OOPIF-specific behavior, it would need to be tested from Blink, but we are setting a value in cc/. Verifying it would require plumbing a value just for testing from cc/ through content/ and then to Blink via WebLayerTreeView. Any suggestions would be appreciated..
Description was changed from ========== Set GPU rasterization trigger for OOPIFs Currently the GPU rasterization trigger is never set on the compositor for an out-of-process iframe. This patch changes it so that it is always set. BUG=712794 ========== to ========== Set GPU rasterization trigger for OOPIFs Currently the GPU rasterization trigger is never set on the compositor for an out-of-process iframe. This patch changes it so that it is always set. BUG=712794 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/19 17:30:33, kenrb wrote: > ericrk: Does this look reasonable to you? > > I don't see a good way to test this. Since it is OOPIF-specific behavior, it > would need to be tested from Blink, but we are setting a value in cc/. Verifying > it would require plumbing a value just for testing from cc/ through content/ and > then to Blink via WebLayerTreeView. Any suggestions would be appreciated.. Thanks for taking a look. +ojan@ and vmiura@ - Thinking about this more - I wonder if we should be matching the behavior of the top document? This is only a concern on Android, so we might not need to worry about this yet. This might not really matter, as we're rasterizing from two different processes, so having the raster mode match doesn't necessarily allow for any resource sharing, etc... However, it feels like it would make issues easier to debug if this trigger applied to the whole page (including iframes). Also, on desktop content (which doesn't trigger this), we're more likely to pinch zoom, which can cause perf issues with GPU raster - these issues might bubble down to embedded iframes with GPU raster. WDYT? Test wise, I don't have any great insights on how to test this... If blink tests don't mock out CC, then I guess we would have to pipe a value back from CC, which does sound like a lot of plumbing. I wonder if we could just add a DCHECK to CC to make sure this value is always true on Desktop? We could then just write a test which hits this and would DCHECK in debug builds if this was wrong? Doesn't help on Android, but at least gets us some coverage.
Actually cc vmiura@ and ojan@ :D - see question above.
Description was changed from ========== Set GPU rasterization trigger for OOPIFs Currently the GPU rasterization trigger is never set on the compositor for an out-of-process iframe. This patch changes it so that it is always set. BUG=712794 ========== to ========== Set GPU rasterization trigger for OOPIFs Currently the GPU rasterization trigger is never set on the compositor for an out-of-process iframe. This patch changes it so that it is always set. BUG=712794 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by kenrb@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_...)
The CQ bit was checked by kenrb@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...
On 2017/04/19 20:34:49, ericrk wrote: > This might not really matter, as we're rasterizing from two different processes, > so having the raster mode match doesn't necessarily allow for any resource > sharing, etc... However, it feels like it would make issues easier to debug if > this trigger applied to the whole page (including iframes). Also, on desktop > content (which doesn't trigger this), we're more likely to pinch zoom, which can > cause perf issues with GPU raster - these issues might bubble down to embedded > iframes with GPU raster. > > WDYT? I have attempted to add that condition, setting it when the layerTreeView is initialized and again when the widget is resized. I'm not certain that is correct but it might be reasonable based on where HeuristicsForGpuRasterizationUdpated is called in WebViewImpl? > > Test wise, I don't have any great insights on how to test this... If blink tests > don't mock out CC, then I guess we would have to pipe a value back from CC, > which does sound like a lot of plumbing. > > I wonder if we could just add a DCHECK to CC to make sure this value is always > true on Desktop? We could then just write a test which hits this and would > DCHECK in debug builds if this was wrong? Doesn't help on Android, but at least > gets us some coverage. I tried adding a DCHECK to cc/, and hundreds of layout tests broke. Apparently that gets called with trigger 'false' during tests regularly, in any case. The right answer here is to add a new unit test class for WebFrameWidgets (it currently doesn't have any unit tests at all) with a mocked out client, and test it there. I am busy with some higher priority bugs right now so would you be fine with landing with a test to resolve this issue? I filed bug 714710 to create that test class.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Overall LGTM, with follow-up bug/bugs in place. On 2017/04/24 18:21:09, kenrb wrote: > On 2017/04/19 20:34:49, ericrk wrote: > > This might not really matter, as we're rasterizing from two different > processes, > > so having the raster mode match doesn't necessarily allow for any resource > > sharing, etc... However, it feels like it would make issues easier to debug if > > this trigger applied to the whole page (including iframes). Also, on desktop > > content (which doesn't trigger this), we're more likely to pinch zoom, which > can > > cause perf issues with GPU raster - these issues might bubble down to embedded > > iframes with GPU raster. > > > > WDYT? > > I have attempted to add that condition, setting it when the layerTreeView is > initialized and again when the widget is resized. I'm not certain that is > correct but it might be reasonable based on where > HeuristicsForGpuRasterizationUdpated is called in WebViewImpl? hmm, I was thinking we would match the parent (top) document, rather than re-determine this here - I'm not sure this really matches the logic in WebViewImpl (which triggers for no viewport (desktop), or a user defined viewport). Maybe just go back to "true" for now, and add a note to the follow-up bug to make this match top doc? > > > > > Test wise, I don't have any great insights on how to test this... If blink > tests > > don't mock out CC, then I guess we would have to pipe a value back from CC, > > which does sound like a lot of plumbing. > > > > I wonder if we could just add a DCHECK to CC to make sure this value is always > > true on Desktop? We could then just write a test which hits this and would > > DCHECK in debug builds if this was wrong? Doesn't help on Android, but at > least > > gets us some coverage. > > I tried adding a DCHECK to cc/, and hundreds of layout tests broke. Apparently > that gets called with trigger 'false' during tests regularly, in any case. > > The right answer here is to add a new unit test class for WebFrameWidgets (it > currently doesn't have any unit tests at all) with a mocked out client, and test > it there. I am busy with some higher priority bugs right now so would you be > fine with landing with a test to resolve this issue? I filed bug 714710 to > create that test class. sounds good.
On 2017/04/24 22:17:00, ericrk wrote: > hmm, I was thinking we would match the parent (top) document, rather than > re-determine this here - I'm not sure this really matches the logic in > WebViewImpl (which triggers for no viewport (desktop), or a user defined > viewport). > > Maybe just go back to "true" for now, and add a note to the follow-up bug to > make this match top doc? > Okay, I have done this. I will leave this bug open to finish the work. The eventual plan is to remove the WebWidget implementation in WebViewImpl, and have all local frame roots (i.e. every frame that has a widget and compositor) use a WebFrameWidget, so we no longer have to duplicate logic for top-level frames and OOPIFs. In the short term this can be done by moving code from WebViewImpl into WebFrameWidgetBase, but untangling resize logic is difficult.
kenrb@chromium.org changed reviewers: + bokan@chromium.org
bokan@: PTAL for Source/web OWNERS review?
Agree it would make more sense to match the top document but assuming OOPIF and GPU folks are ok with this, code is lgtm
FYI, the reason viewport meta is used is as a signal that the page is "mobile friendly". Subframes would necessarily not have this since the viewport meta is only applicable on the top frame. But this isn't a signal of "mobile friendliness" one way or the other - this could cause us to use GPU raster on iframes we otherwise wouldn't.
The CQ bit was checked by kenrb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ericrk@chromium.org Link to the patchset: https://codereview.chromium.org/2828023002/#ps60001 (title: "Back to first fix")
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": 60001, "attempt_start_ts": 1493149696411220,
"parent_rev": "deda22b114bc8308ad1ae888727a66ceda9189aa", "commit_rev":
"1931faea0d5ada216fca3c11add153ceaa82ba6d"}
Message was sent while issue was closed.
Description was changed from ========== Set GPU rasterization trigger for OOPIFs Currently the GPU rasterization trigger is never set on the compositor for an out-of-process iframe. This patch changes it so that it is always set. BUG=712794 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Set GPU rasterization trigger for OOPIFs Currently the GPU rasterization trigger is never set on the compositor for an out-of-process iframe. This patch changes it so that it is always set. BUG=712794 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2828023002 Cr-Commit-Position: refs/heads/master@{#467130} Committed: https://chromium.googlesource.com/chromium/src/+/1931faea0d5ada216fca3c11add1... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/1931faea0d5ada216fca3c11add1... |
