Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2495373003:
Match html canvas which is transferred to OffscreenCanvas to CSS style (Closed)
|
Created:
4 years, 1 month ago by xlai (Olivia) Modified:
4 years ago CC:
chromium-reviews, dshwang, ajuma+watch-canvas_chromium.org, blink-reviews-html_chromium.org, jam, dglazkov+blink, Rik, darin-cc_chromium.org, blink-reviews, piman+watch_chromium.org, cc-bugs_chromium.org, kinuko+watch, blink-worker-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMatch html canvas which is transferred to OffscreenCanvas to CSS style
This CL aims to make html canvas to match to CSS style restriction regardless
whether it is transferred to OffscreenCanvas or not.
In a usual compositor commit, when a css style is imposed on the html canvas
to change its width and height, its layer will reset a different bounds.
However, in the case when html canvas has transferred control to offscreen,
it is using a SurfaceLayer, which directly use surface_size_ without
incorporating layer bounds to append SurfaceDrawQuad. This CL changes
SurfaceLayerImpl to take into account the special case when layer bounds is
different from surface_size by doing a transformation on the
Quad, especially when the aspect ratio is different.
The CL respects existing non-canvas cases on high-dpi devices and settings
when enable-use-zoom-for-dsf is on or off.
Also, it is also possible that the compositor commit due to style change
happens before the Surface Layer for html canvas is created. To prevent
flakiness, we force a compositor commit at the time when Surface Layer is
created and registered to GraphicsLayers.
TBR=khushalsagar@chromium.org, twellington@chromium.org, pfeldman@chromium.org
BUG=655335, 652931
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Committed: https://crrev.com/5e87e713d6ed8c36fb37b814c6b65e7096d4554e
Cr-Commit-Position: refs/heads/master@{#438986}
Patch Set 1 #
Total comments: 4
Patch Set 2 : unit test #Patch Set 3 : fix #Patch Set 4 : added deterministic gpu tests and fix high dpi issue #Patch Set 5 : spelling error #
Total comments: 13
Patch Set 6 : fix #Patch Set 7 : Separate canvas case from others #Patch Set 8 : fix #Patch Set 9 : fix #
Total comments: 39
Patch Set 10 : rebase #Patch Set 11 : fix basedd on feedback #
Total comments: 7
Patch Set 12 : rebase #Patch Set 13 : final fix #Patch Set 14 : fix compilation error #
Total comments: 2
Messages
Total messages: 94 (60 generated)
Description was changed from ========== Match html canvas which is transferred to OffscreenCanvas to CSS style This CL aims to make html canvas to match to CSS style restriction regardless whether it is transferred to OffscreenCanvas or not. In a usual compositor commit, when a css style is imposed on the html canvas, its layer will reset a different bounds. However, in the case when html canvas has transferred control to offscreen, it is using a SurfaceLayer, which directly use surface_size_ without incorporating layer bounds to append SurfaceDrawQuad. This CL makes SurfaceLayerImpl to take into account the special case when layer bounds is different from surface_size_ in AppendQuads by doing a transformation on the Quad. Also, it is also possible that the compositor commit due to style change happens before the Surface Layer for html canvas is created. To prevent flakiness, we force a compositor commit at the time when Surface Layer is created and registered to GraphicsLayers. BUG=655355 ========== to ========== Match html canvas which is transferred to OffscreenCanvas to CSS style This CL aims to make html canvas to match to CSS style restriction regardless whether it is transferred to OffscreenCanvas or not. In a usual compositor commit, when a css style is imposed on the html canvas, its layer will reset a different bounds. However, in the case when html canvas has transferred control to offscreen, it is using a SurfaceLayer, which directly use surface_size_ without incorporating layer bounds to append SurfaceDrawQuad. This CL makes SurfaceLayerImpl to take into account the special case when layer bounds is different from surface_size_ in AppendQuads by doing a transformation on the Quad. Also, it is also possible that the compositor commit due to style change happens before the Surface Layer for html canvas is created. To prevent flakiness, we force a compositor commit at the time when Surface Layer is created and registered to GraphicsLayers. BUG=655355 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel ==========
Description was changed from ========== Match html canvas which is transferred to OffscreenCanvas to CSS style This CL aims to make html canvas to match to CSS style restriction regardless whether it is transferred to OffscreenCanvas or not. In a usual compositor commit, when a css style is imposed on the html canvas, its layer will reset a different bounds. However, in the case when html canvas has transferred control to offscreen, it is using a SurfaceLayer, which directly use surface_size_ without incorporating layer bounds to append SurfaceDrawQuad. This CL makes SurfaceLayerImpl to take into account the special case when layer bounds is different from surface_size_ in AppendQuads by doing a transformation on the Quad. Also, it is also possible that the compositor commit due to style change happens before the Surface Layer for html canvas is created. To prevent flakiness, we force a compositor commit at the time when Surface Layer is created and registered to GraphicsLayers. BUG=655355 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel ========== to ========== Match html canvas which is transferred to OffscreenCanvas to CSS style This CL aims to make html canvas to match to CSS style restriction regardless whether it is transferred to OffscreenCanvas or not. In a usual compositor commit, when a css style is imposed on the html canvas, its layer will reset a different bounds. However, in the case when html canvas has transferred control to offscreen, it is using a SurfaceLayer, which directly use surface_size_ without incorporating layer bounds to append SurfaceDrawQuad. This CL makes SurfaceLayerImpl to take into account the special case when layer bounds is different from surface_size_ in AppendQuads by doing a transformation on the Quad. Also, it is also possible that the compositor commit due to style change happens before the Surface Layer for html canvas is created. To prevent flakiness, we force a compositor commit at the time when Surface Layer is created and registered to GraphicsLayers. BUG=655355 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== Match html canvas which is transferred to OffscreenCanvas to CSS style This CL aims to make html canvas to match to CSS style restriction regardless whether it is transferred to OffscreenCanvas or not. In a usual compositor commit, when a css style is imposed on the html canvas, its layer will reset a different bounds. However, in the case when html canvas has transferred control to offscreen, it is using a SurfaceLayer, which directly use surface_size_ without incorporating layer bounds to append SurfaceDrawQuad. This CL makes SurfaceLayerImpl to take into account the special case when layer bounds is different from surface_size_ in AppendQuads by doing a transformation on the Quad. Also, it is also possible that the compositor commit due to style change happens before the Surface Layer for html canvas is created. To prevent flakiness, we force a compositor commit at the time when Surface Layer is created and registered to GraphicsLayers. BUG=655355 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Match html canvas which is transferred to OffscreenCanvas to CSS style This CL aims to make html canvas to match to CSS style restriction regardless whether it is transferred to OffscreenCanvas or not. In a usual compositor commit, when a css style is imposed on the html canvas, its layer will reset a different bounds. However, in the case when html canvas has transferred control to offscreen, it is using a SurfaceLayer, which directly use surface_size_ without incorporating layer bounds to append SurfaceDrawQuad. This CL makes SurfaceLayerImpl to take into account the special case when layer bounds is different from surface_size_ in AppendQuads by doing a transformation on the Quad. Also, it is also possible that the compositor commit due to style change happens before the Surface Layer for html canvas is created. To prevent flakiness, we force a compositor commit at the time when Surface Layer is created and registered to GraphicsLayers. BUG=655355 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
xlai@chromium.org changed reviewers: + danakj@chromium.org, junov@chromium.org, kbr@chromium.org
xlai@chromium.org changed reviewers: + chrishtr@chromium.org
Description was changed from ========== Match html canvas which is transferred to OffscreenCanvas to CSS style This CL aims to make html canvas to match to CSS style restriction regardless whether it is transferred to OffscreenCanvas or not. In a usual compositor commit, when a css style is imposed on the html canvas, its layer will reset a different bounds. However, in the case when html canvas has transferred control to offscreen, it is using a SurfaceLayer, which directly use surface_size_ without incorporating layer bounds to append SurfaceDrawQuad. This CL makes SurfaceLayerImpl to take into account the special case when layer bounds is different from surface_size_ in AppendQuads by doing a transformation on the Quad. Also, it is also possible that the compositor commit due to style change happens before the Surface Layer for html canvas is created. To prevent flakiness, we force a compositor commit at the time when Surface Layer is created and registered to GraphicsLayers. BUG=655355 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Match html canvas which is transferred to OffscreenCanvas to CSS style This CL aims to make html canvas to match to CSS style restriction regardless whether it is transferred to OffscreenCanvas or not. In a usual compositor commit, when a css style is imposed on the html canvas to change its width and height, its layer will reset a different bounds. However, in the case when html canvas has transferred control to offscreen, it is using a SurfaceLayer, which directly use surface_size_ without incorporating layer bounds to append SurfaceDrawQuad. This CL makes SurfaceLayerImpl to take into account the special case when layer bounds is different from surface_size_ in AppendQuads by doing a transformation on the Quad. Also, it is also possible that the compositor commit due to style change happens before the Surface Layer for html canvas is created. To prevent flakiness, we force a compositor commit at the time when Surface Layer is created and registered to GraphicsLayers. BUG=655355 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The test changes lgtm - I defer to others' review of the rest.
Where is the code that pushes the CSS size to the compositor when the style changes? For example, if the CSS size of the canvas changes after the SurfaceLayer was created, what happens? https://codereview.chromium.org/2495373003/diff/1/cc/layers/surface_layer_imp... File cc/layers/surface_layer_impl.cc (right): https://codereview.chromium.org/2495373003/diff/1/cc/layers/surface_layer_imp... cc/layers/surface_layer_impl.cc:81: scaled_draw_transform.Scale(SK_MScalar1 / scale_x, SK_MScalar1 / scale_y); How does this affect other uses of surface_layer?
https://codereview.chromium.org/2495373003/diff/1/cc/layers/surface_layer_imp... File cc/layers/surface_layer_impl.cc (right): https://codereview.chromium.org/2495373003/diff/1/cc/layers/surface_layer_imp... cc/layers/surface_layer_impl.cc:71: gfx::Transform scaled_draw_transform = New code requires new unit tests of the class.
Unit test added. On 2016/11/14 20:41:27, Justin Novosad wrote: > Where is the code that pushes the CSS size to the compositor when the style > changes? For example, if the CSS size of the canvas changes after the > SurfaceLayer was created, what happens? I can see that the style change will be detected by CompositedLayerMapping::updateGraphicsLayerGeometry, which will eventually lead to WebLayer::setBounds, and then propagate to LayerImpl::setBounds after PushProperties (this propagation is the compositor commit). If SurfaceLayer is already present at the time when push properties is happening (i.e. CompositedLayerMapping:: updateGraphicsLayerConfiguration() has correctly put the SurfaceLayer as HTMLCanvas layout object's layer), then the above change (LayerImpl::setBounds) would be correctly captured by SurfaceLayerImpl. And when SurfaceLayerImpl is appending quads (an action which I found to be initiated by cc scheduler), it will do the correct calculation I've added. If SurfaceLayer is not present at the time when push properties is happening, then the style change is simply ignored because we don't have SurfaceLayerImpl for that html canvas yet. This is why I was encountering flaky case until I add PaintLayerCompositor::setNeedsCompositingUpdate. I verify this reason from standard output when I was debugging: whenever flakiness happens, the createSurfaceLayer always happens after a long sequence of LayerImpl::setBounds happen; whenever a correct image appears, the createSurfaceLayer always happens before a long sequence of LayerImp::setBounds happen. https://codereview.chromium.org/2495373003/diff/1/cc/layers/surface_layer_imp... File cc/layers/surface_layer_impl.cc (right): https://codereview.chromium.org/2495373003/diff/1/cc/layers/surface_layer_imp... cc/layers/surface_layer_impl.cc:71: gfx::Transform scaled_draw_transform = On 2016/11/14 20:44:06, danakj wrote: > New code requires new unit tests of the class. Done. https://codereview.chromium.org/2495373003/diff/1/cc/layers/surface_layer_imp... cc/layers/surface_layer_impl.cc:81: scaled_draw_transform.Scale(SK_MScalar1 / scale_x, SK_MScalar1 / scale_y); On 2016/11/14 20:41:27, Justin Novosad wrote: > How does this affect other uses of surface_layer? I don't think it will not affect other uses of surface_layer, because the existing usage of surface_layer always assume that bounds() and surface_size are the same. If there are other use cases that don't make such assumption, then they must have already found out the SurfaceLayerImpl does not handle this special case well and fixed this bug before I ever have a chance to create this CL:) If you look carefully, I only modify one component of the quads, which is the scale_draw_transform here. The calculation would be the same for existing cases where bounds() and surfaceSize are the same.
The CQ bit was checked by xlai@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
So, if updating the CSS style after the SurfaceLayer has been created works as it should, we should definitely have a layout test for it.
On 2016/11/15 21:19:34, Justin Novosad wrote: > So, if updating the CSS style after the SurfaceLayer has been created works as > it should, we should definitely have a layout test for it. Unfortunately layout test does not work for OffscreenCanvas.commit(); I mean, the compositorframe sent to Surface will not be processed in layout test. I have already use a gpu pixel test to test it.
On 2016/11/15 21:29:31, xlai (Olivia) wrote: > On 2016/11/15 21:19:34, Justin Novosad wrote: > > So, if updating the CSS style after the SurfaceLayer has been created works as > > it should, we should definitely have a layout test for it. > > Unfortunately layout test does not work for OffscreenCanvas.commit(); I mean, > the > compositorframe sent to Surface will not be processed in layout test. I have > already use a gpu pixel test to test it. Right, but the gpu test in this cl does not cover that use case.
On 2016/11/15 21:31:29, Justin Novosad wrote: > On 2016/11/15 21:29:31, xlai (Olivia) wrote: > > On 2016/11/15 21:19:34, Justin Novosad wrote: > > > So, if updating the CSS style after the SurfaceLayer has been created works > as > > > it should, we should definitely have a layout test for it. > > > > Unfortunately layout test does not work for OffscreenCanvas.commit(); I mean, > > the > > compositorframe sent to Surface will not be processed in layout test. I have > > already use a gpu pixel test to test it. > > Right, but the gpu test in this cl does not cover that use case. It covers. The timing of when surfacelayer is created and the timing of when push properties happen are non-deterministic. So, if one of the use case failed, that gpu pixel test will become flaky. The fact that it is not flaky means both cases are running good.
On 2016/11/15 21:34:16, xlai (Olivia) wrote: > On 2016/11/15 21:31:29, Justin Novosad wrote: > > On 2016/11/15 21:29:31, xlai (Olivia) wrote: > > > On 2016/11/15 21:19:34, Justin Novosad wrote: > > > > So, if updating the CSS style after the SurfaceLayer has been created > works > > as > > > > it should, we should definitely have a layout test for it. > > > > > > Unfortunately layout test does not work for OffscreenCanvas.commit(); I > mean, > > > the > > > compositorframe sent to Surface will not be processed in layout test. I have > > > already use a gpu pixel test to test it. > > > > Right, but the gpu test in this cl does not cover that use case. > > It covers. The timing of when surfacelayer is created and the timing of when > push properties happen are non-deterministic. So, if one of the use case failed, > that gpu pixel test will become flaky. The fact that it is not flaky means both > cases are running good. That is terrible. It would be easy to write non-flaky tests that deterministically cover both scenarios by using testRunner.layoutAndPaintAsyncThen
FYI, the reason I wrote "That is terrible": flake is often dismissed, or even worse, filtered-out by the tools. We should not be counting on flake for detecting problems.
The CQ bit was checked by xlai@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Justin, I think there was something that I didn't realize when I was explaining the two cases, i.e the case A when SurfaceLayer is present when compositor commit is happening, v.s. the case B when SurfaceLayer is absent when compositor commit is happening. I did encounter both cases (and therefore flaky results) before I add PaintLayerCompositor::setNeedsCompositingUpdate. That's where my explanation of "I verify this reason from standard output when I was debugging" comes from. However, after I add PaintLayerCompositor::setNeedsCompositingUpdate and force a compositor commit after SurfaceLayer is created (which is the current status of this CL), Case B is no longer happening because even if SurfaceLayer is absent when compositor commit is happening I will force another compositor commit to happen after SurfaceLayer creation. Therefore, there is only 1 use case and 1 deterministic result in this CL, which is case A. Nevertheless, this CL still have some problems with HighDPI machines and HighDPI tests, which I'm still investigating. > On 2016/11/14 20:41:27, Justin Novosad wrote: > > Where is the code that pushes the CSS size to the compositor when the style > > changes? For example, if the CSS size of the canvas changes after the > > SurfaceLayer was created, what happens? > > I can see that the style change will be detected by > CompositedLayerMapping::updateGraphicsLayerGeometry, > which will eventually lead to WebLayer::setBounds, and > then propagate to LayerImpl::setBounds after PushProperties > (this propagation is the compositor commit). > > If SurfaceLayer is already present at the time when push > properties is happening (i.e. CompositedLayerMapping:: > updateGraphicsLayerConfiguration() has correctly put the > SurfaceLayer as HTMLCanvas layout object's layer), then > the above change (LayerImpl::setBounds) would be correctly > captured by SurfaceLayerImpl. And when SurfaceLayerImpl > is appending quads (an action which I found to be initiated > by cc scheduler), it will do the correct calculation I've > added. > > If SurfaceLayer is not present at the time when push properties > is happening, then the style change is simply ignored because > we don't have SurfaceLayerImpl for that html canvas yet. This > is why I was encountering flaky case until I add > PaintLayerCompositor::setNeedsCompositingUpdate. > > I verify this reason from standard output when I was debugging: > whenever flakiness happens, the > createSurfaceLayer always happens after a long sequence of > LayerImpl::setBounds happen; whenever a correct image appears, > the createSurfaceLayer always happens before a long sequence of > LayerImp::setBounds happen.
On 2016/11/16 18:14:22, xlai (Olivia) wrote: > Justin, I think there was something that I didn't realize when I was explaining > the > two cases, i.e the case A when SurfaceLayer is present when compositor commit > is happening, v.s. the case B when SurfaceLayer is absent when compositor commit > is happening. > > I did encounter both cases (and therefore flaky results) before I add > PaintLayerCompositor::setNeedsCompositingUpdate. That's where my explanation > of "I verify this reason from standard output when I was debugging" comes from. > However, after I add PaintLayerCompositor::setNeedsCompositingUpdate and force > a compositor commit after SurfaceLayer is created (which is the current status > of this > CL), Case B is no longer happening because even if SurfaceLayer is absent when > compositor commit is happening I will force another compositor commit to happen > after SurfaceLayer creation. Therefore, there is only 1 use case and 1 > deterministic > result in this CL, which is case A. > > Nevertheless, this CL still have some problems with HighDPI machines and HighDPI > tests, which I'm still investigating. > Okay, that is all good, but you are still missing test coverage for changing the style after the initial SurfaceLayer creation.
The CQ bit was checked by xlai@chromium.org to run a CQ dry run
The CQ bit was unchecked by xlai@chromium.org
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by xlai@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_...)
junov@: I've replaced the modified gpu pixel tests with two new dedicated gpu pixel tests that put a different order btw SurfaceLayer creation and Style resize change. danakj@: High DPI issue is now fixed; all the existing high DPI tests passed in my previous dry run. The correct formula is to simply apply a transformation of surface_size/layer_bound_size without taking into account of DSF, PDSF or surface_scale, because surface_size or layer_bound_size (chrome and blink respectively) in high DPI cases already gets inflated accordingly.
lgtm with nits. https://codereview.chromium.org/2495373003/diff/100001/content/test/data/gpu/... File content/test/data/gpu/pixel_offscreenCanvas_transfer_after_style_resize.html (right): https://codereview.chromium.org/2495373003/diff/100001/content/test/data/gpu/... content/test/data/gpu/pixel_offscreenCanvas_transfer_after_style_resize.html:88: window.webkitRequestAnimationFrame(waitForFinish); the "webkit" prefix is obsolete please don't use it in new tests. https://codereview.chromium.org/2495373003/diff/100001/content/test/data/gpu/... File content/test/data/gpu/pixel_offscreenCanvas_transfer_before_style_resize.html (right): https://codereview.chromium.org/2495373003/diff/100001/content/test/data/gpu/... content/test/data/gpu/pixel_offscreenCanvas_transfer_before_style_resize.html:88: window.webkitRequestAnimationFrame(waitForFinish); remove "webkit" prefix.
The CQ bit was checked by xlai@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: This issue passed the CQ dry run.
Description was changed from ========== Match html canvas which is transferred to OffscreenCanvas to CSS style This CL aims to make html canvas to match to CSS style restriction regardless whether it is transferred to OffscreenCanvas or not. In a usual compositor commit, when a css style is imposed on the html canvas to change its width and height, its layer will reset a different bounds. However, in the case when html canvas has transferred control to offscreen, it is using a SurfaceLayer, which directly use surface_size_ without incorporating layer bounds to append SurfaceDrawQuad. This CL makes SurfaceLayerImpl to take into account the special case when layer bounds is different from surface_size_ in AppendQuads by doing a transformation on the Quad. Also, it is also possible that the compositor commit due to style change happens before the Surface Layer for html canvas is created. To prevent flakiness, we force a compositor commit at the time when Surface Layer is created and registered to GraphicsLayers. BUG=655355 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Match html canvas which is transferred to OffscreenCanvas to CSS style This CL aims to make html canvas to match to CSS style restriction regardless whether it is transferred to OffscreenCanvas or not. In a usual compositor commit, when a css style is imposed on the html canvas to change its width and height, its layer will reset a different bounds. However, in the case when html canvas has transferred control to offscreen, it is using a SurfaceLayer, which directly use surface_size_ without incorporating layer bounds to append SurfaceDrawQuad. This CL makes SurfaceLayerImpl to take into account the special case when layer bounds is different from surface_size_ in AppendQuads by doing a transformation on the Quad. Also, it is also possible that the compositor commit due to style change happens before the Surface Layer for html canvas is created. To prevent flakiness, we force a compositor commit at the time when Surface Layer is created and registered to GraphicsLayers. BUG=655335 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
gentle ping to danakj@.
Description was changed from ========== Match html canvas which is transferred to OffscreenCanvas to CSS style This CL aims to make html canvas to match to CSS style restriction regardless whether it is transferred to OffscreenCanvas or not. In a usual compositor commit, when a css style is imposed on the html canvas to change its width and height, its layer will reset a different bounds. However, in the case when html canvas has transferred control to offscreen, it is using a SurfaceLayer, which directly use surface_size_ without incorporating layer bounds to append SurfaceDrawQuad. This CL makes SurfaceLayerImpl to take into account the special case when layer bounds is different from surface_size_ in AppendQuads by doing a transformation on the Quad. Also, it is also possible that the compositor commit due to style change happens before the Surface Layer for html canvas is created. To prevent flakiness, we force a compositor commit at the time when Surface Layer is created and registered to GraphicsLayers. BUG=655335 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Match html canvas which is transferred to OffscreenCanvas to CSS style This CL aims to make html canvas to match to CSS style restriction regardless whether it is transferred to OffscreenCanvas or not. In a usual compositor commit, when a css style is imposed on the html canvas to change its width and height, its layer will reset a different bounds. However, in the case when html canvas has transferred control to offscreen, it is using a SurfaceLayer, which directly use surface_size_ without incorporating layer bounds to append SurfaceDrawQuad. This CL makes SurfaceLayerImpl to take into account the special case when layer bounds is different from surface_size_ in AppendQuads by doing a transformation on the Quad. Also, it is also possible that the compositor commit due to style change happens before the Surface Layer for html canvas is created. To prevent flakiness, we force a compositor commit at the time when Surface Layer is created and registered to GraphicsLayers. BUG=655335, 655311 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
https://codereview.chromium.org/2495373003/diff/100001/cc/layers/surface_laye... File cc/layers/surface_layer_impl.cc (right): https://codereview.chromium.org/2495373003/diff/100001/cc/layers/surface_laye... cc/layers/surface_layer_impl.cc:74: float boundsWidth = bounds().width(); chromium style, though I'd drop these 4 temp vars they aren't adding any context with their names and they are only used once? https://codereview.chromium.org/2495373003/diff/100001/cc/layers/surface_laye... cc/layers/surface_layer_impl.cc:78: float scale_x = surfaceSizeWidth / boundsWidth; why not invert these, and then you don't have to re-invert them below in the call to Scale()? Can you leave a comment explaining why this scale is being used? AIUI this is the transform from the space inside the surface to the space of the surface layer? https://codereview.chromium.org/2495373003/diff/100001/cc/layers/surface_laye... cc/layers/surface_layer_impl.cc:82: gfx::Size scaled_bounds = gfx::ScaleToCeiledSize(bounds(), surface_scale_); I'm very confused what the function of this surface_scale_ is at this point, and having this not match the transform is very odd. Can you explain why it is not matching? The position/bounds of the layer say what space it should occupy in the embedder of the SurfaceLayer. The transform creates a difference space where the content is coming from, in this case based on the size of the surface that was being produced. This scaled bounds should be in that space I would expect but they are not here? https://codereview.chromium.org/2495373003/diff/100001/cc/layers/surface_laye... cc/layers/surface_layer_impl.cc:87: shared_quad_state->SetAll( If you can't just add params toPopulateScaledSharedQuadState(), I'd prefer this move to a 2nd method as a peer of it, so the duplicated code is easier to find.
+cc weiliangc@ https://codereview.chromium.org/2495373003/diff/100001/cc/layers/surface_laye... File cc/layers/surface_layer_impl.cc (right): https://codereview.chromium.org/2495373003/diff/100001/cc/layers/surface_laye... cc/layers/surface_layer_impl.cc:74: float boundsWidth = bounds().width(); On 2016/11/21 23:37:22, danakj wrote: > chromium style, though I'd drop these 4 temp vars they aren't adding any context > with their names and they are only used once? Done. I was trying to force them (integer) to float to prevent their fractional part being discarded. https://codereview.chromium.org/2495373003/diff/100001/cc/layers/surface_laye... cc/layers/surface_layer_impl.cc:78: float scale_x = surfaceSizeWidth / boundsWidth; On 2016/11/21 23:37:22, danakj wrote: > why not invert these, and then you don't have to re-invert them below in the > call to Scale()? > > Can you leave a comment explaining why this scale is being used? AIUI this is > the transform from the space inside the surface to the space of the surface > layer? Done. https://codereview.chromium.org/2495373003/diff/100001/cc/layers/surface_laye... cc/layers/surface_layer_impl.cc:82: gfx::Size scaled_bounds = gfx::ScaleToCeiledSize(bounds(), surface_scale_); On 2016/11/21 23:37:22, danakj wrote: > I'm very confused what the function of this surface_scale_ is at this point, and > having this not match the transform is very odd. Can you explain why it is not > matching? > > The position/bounds of the layer say what space it should occupy in the embedder > of the SurfaceLayer. The transform creates a difference space where the content > is coming from, in this case based on the size of the surface that was being > produced. This scaled bounds should be in that space I would expect but they are > not here? Actually, this part of code is exactly same as the original code. It'll be more clearer after I put this part of code as a peer function of the existing PopulateScaledSharedQuadState(). The whole CL only changes the quad_to_target_transform in the SharedQuadState and not any other attributes. It is a bit complicated to explain why. Let me list out the standard printout on bounds and surface_size on device-scale-factor=2 case, coupled with the css style change. Let's say css forces canvas to be 400X100, intrinsic canvas size is 300X300. (1) layer bounds() for canvas's layer is 800X200, which means it already takes account the style change and is already double based on device_scale_factor. (2) surface_size remains as 300X300 and surface_scale_ remains as 1. But! For the SurfaceLayer that already exists for a global webpage (before I introduce canvas's SurfaceLayer): (1) layer bounds() remains almost the same as the layer bounds() when device_scale_factor=1. (2) surface_size is double as the the surface_size in DSF=1 (3) surface_scale is 2. To accommodate both cases (as they share the same part of code here), scaled_bounds computation here is doing things right because: (1) In canvas's case, bounds() is double, surface_scale_ is same, so the scaling make the overall scaled_bounds double, which is right for DSF=2. (2) In existing SurfaceLayer's case, bounds() is same, surface_scale_ is double, so the scaling makes the overall scaled_bounds double, which is again right for DSF=2. You might ask why bounds(), surface_size_ and DSF, PDSF, surface_scale all look different (in fact, opposite) in both cases. weiliangc@ and I are making a guess that Chromium and Blink handle all these differently but exactly where are the part handling this is far more complex than we thought and after doing an intensive code search we still cannot see a full picture clearly. I have to admit that I figure out my computation based on the standard output printing different variables (PDSF, DSF, bounds, etc) on different test cases on high DPI and low DPI screens, couple with a lot of trial-and-error efforts to make all of them correct. https://codereview.chromium.org/2495373003/diff/100001/cc/layers/surface_laye... cc/layers/surface_layer_impl.cc:87: shared_quad_state->SetAll( On 2016/11/21 23:37:22, danakj wrote: > If you can't just add params toPopulateScaledSharedQuadState(), I'd prefer this > move to a 2nd method as a peer of it, so the duplicated code is easier to find. Done. https://codereview.chromium.org/2495373003/diff/100001/content/test/data/gpu/... File content/test/data/gpu/pixel_offscreenCanvas_transfer_after_style_resize.html (right): https://codereview.chromium.org/2495373003/diff/100001/content/test/data/gpu/... content/test/data/gpu/pixel_offscreenCanvas_transfer_after_style_resize.html:88: window.webkitRequestAnimationFrame(waitForFinish); On 2016/11/18 15:07:27, Justin Novosad wrote: > the "webkit" prefix is obsolete please don't use it in new tests. Done.
Hope this helps, look carefully cuz I might have made mistakes but hopefully all the different coordinate spaces and how to think about mapping between spaces helps. https://codereview.chromium.org/2495373003/diff/100001/cc/layers/surface_laye... File cc/layers/surface_layer_impl.cc (right): https://codereview.chromium.org/2495373003/diff/100001/cc/layers/surface_laye... cc/layers/surface_layer_impl.cc:74: float boundsWidth = bounds().width(); On 2016/11/22 16:55:59, xlai (Olivia) wrote: > On 2016/11/21 23:37:22, danakj wrote: > > chromium style, though I'd drop these 4 temp vars they aren't adding any > context > > with their names and they are only used once? > > Done. I was trying to force them (integer) to float to prevent their fractional > part being discarded. Ah, you can use a static cast for that, or use temps and a comment explaining. You only need to make one of them (surface_size_ for instance) promoted to a float for that to work. https://codereview.chromium.org/2495373003/diff/100001/cc/layers/surface_laye... cc/layers/surface_layer_impl.cc:82: gfx::Size scaled_bounds = gfx::ScaleToCeiledSize(bounds(), surface_scale_); On 2016/11/22 16:55:59, xlai (Olivia) wrote: > On 2016/11/21 23:37:22, danakj wrote: > > I'm very confused what the function of this surface_scale_ is at this point, > and > > having this not match the transform is very odd. Can you explain why it is not > > matching? > > > > The position/bounds of the layer say what space it should occupy in the > embedder > > of the SurfaceLayer. The transform creates a difference space where the > content > > is coming from, in this case based on the size of the surface that was being > > produced. This scaled bounds should be in that space I would expect but they > are > > not here? > > Actually, this part of code is exactly same as the original code. It'll be more > clearer after I put this part of code as a peer function of the existing > PopulateScaledSharedQuadState(). The whole CL only changes the > quad_to_target_transform > in the SharedQuadState and not any other attributes. My feeling is that it doesn't make sense to change one part and not the other, as they are tied together. > It is a bit complicated to explain why. Let me list out the standard printout on > bounds and > surface_size on device-scale-factor=2 case, coupled with the css style change. > Let's say css forces canvas to be 400X100, intrinsic canvas size is 300X300. > > (1) layer bounds() for canvas's layer is 800X200, which means it already takes > account the style change and is already double based on device_scale_factor. This is true when UseZoomForDSF is true, but when it is false, the layer bounds will stay 400x100. > (2) surface_size remains as 300X300 and surface_scale_ remains as 1. So intrinsic canvas size does not change if your device-scale-factor changes, is my take away here? Let's start with canvas and css match and both are 100x100, with DSF=2. In that case the canvas should cover 200x200 pixels in its target when it is drawn. With UseZoomForDSF for a canvas: - CSS size=100x100 - Bounds=200x200 - Surface size=100x100 - scale = surfacesize / bounds = 100/200 = 1/2 - transform = 1/scale = 2 This says that when going from the layer to its target everything grows by 2x so the 100x100 pixels in the canvas will cover 200x200 pixels in the target, giving what we want under DSF=2. Without UseZoomForDSF for a canvas (did you try this mode is it true? this is how you can change it https://cs.chromium.org/chromium/src/content/common/content_switches_internal... to use the mode): - CSS size=100x100 - Bounds=100x100 - Surface size=100x100 - scale = surfacesize / bounds = 100/100 = 1 - transform = 1/scale = 1 Here we'll take the 100x100 pixels from the canvas and map them to 100x100 pixels in the target. This seems wrong we want it to cover 200x200 pixels in the target. With UseZoomForDSF for web/compositor content: - CSS size=100x100 - Bounds=200x200 - Surface size=200x200 - scale = surfacesize / bounds = 200/200 = 1 - transform = 1/scale = 1 Here we have already DSF-scaled content of 200x200 and we transform it by identity so we get 200x200 pixels in the target, great. Without UseZoomForDSF for web/compositor content: - CSS size=100x100 - Bounds=100x100 - Surface size=200x200 - scale = surfacesize / bounds = 200/100 = 2 - transform = 1/scale = 1/2 I think that things go awry here again. Web content is scaled so you have a 200x200 surface, but since we're not considering the DSF here we end scaling the content by 1/2 to fill 100x100 pixels in the target which is incorrect. > But! For the SurfaceLayer that already exists for a global webpage (before I > introduce canvas's SurfaceLayer): > > (1) layer bounds() remains almost the same as the layer bounds() when > device_scale_factor=1. This would be true for canvas also if UseZoomForDSF was disabled (which it is for some platforms). > (2) surface_size is double as the the surface_size in DSF=1 This is no longer canvas, which is why you're seeing this behave differently. For compositor outputs, their intrinsic sizes double when the device-scale-factor doubles. Intrinsic canvas size seems to differ here? > (3) surface_scale is 2. Same, this is because the intrinsic size of the content itself doubles for web content when the device-scale-factor changes. > To accommodate both cases (as they share the same part of code here), > scaled_bounds computation here is doing things right because: > (1) In canvas's case, bounds() is double, surface_scale_ is same, so the scaling > make the overall scaled_bounds double, which is right for DSF=2. I claim above that it is doing something incorrect when UseZoomForDSF is false, so would like to understand if I'm wrong about that or we need to handle this. > (2) In existing SurfaceLayer's case, bounds() is same, surface_scale_ is double, > so the scaling makes the overall scaled_bounds double, which is again right for > DSF=2. The documentation for quad_layer_bounds https://cs.chromium.org/chromium/src/cc/quads/shared_quad_state.h?rcl=0&l=47 says that the rect is in the same space as the quad rects. The quad rects https://cs.chromium.org/chromium/src/cc/quads/draw_quad.h?rcl=0&l=55 are in a space such that when you apply the transform you get the quad's output space in its target. I think that we need to change the rect computation here because you changed the transform, which changes the space that the quad rects are defined in. But we did not change the bounds/visible rect. If we change the space they are in, but don't change them, then multiplying them by the transform will give a different result. We can enumerate the cases above again for these rects. With UseZoomForDSF for a canvas: - CSS size=100x100 - Bounds=200x200 - Surface size=100x100 - scale = surfacesize / bounds = 100/200 = 1/2 - transform = 1/scale = 2 - Surface scale=1 - DSF=1 - PaintedDSF=2 The quad rect and layer bounds are being scaled by 2 to find them in the target space. However the layer rect we're giving is bounds*surfacescale = 200x200 * 1 = 200x200. When you apply the transform to that you get 400x400 in the target which is not right. I think this must be the inverse of the transform to get the right result. 200x200 * 1/2 = 100x100 which represents the intrinsic size of the canvas correctly, and which when transformed to the target gives us the correct 200x200 pixels there. Another way to think of this is that the "layer bounds" and "visible rect" here should be in the space of the intrinsic canvas size. That is because the transform is mapping pixels from the intrinsic canvas size to the area of the target we want them to cover. Without UseZoomForDSF for a canvas (did you try this mode is it true? this is how you can change it https://cs.chromium.org/chromium/src/content/common/content_switches_internal... to use the mode): - CSS size=100x100 - Bounds=100x100 - Surface size=100x100 - scale = surfacesize / bounds = 100/100 = 1 - transform = 1/scale = 1 - Surface scale=1 - DSF=2 - PaintedDSF=1 Here we were mapping the pixels incorrectly already I think, so the origin space from the intrinsic canvas space was wrong. We have to fix that before we can talk about the bounds/visible rect. The correct transform here would be 2 again for the pixels to be drawn correctly. Since the canvas intrinsic size is 100x100 but we want it to cover 200x200 in the target. Since the bounds are 100x100 they are already in the origin space/intrinsic canvas space. We actually don't need to scale them at all. With UseZoomForDSF for web/compositor content: - CSS size=100x100 - Bounds=200x200 - Surface size=200x200 - scale = surfacesize / bounds = 200/200 = 1 - transform = 1/scale = 1 - Surface scale=2 - DSF=1 - PaintedDSF=2 Here the intrinsic size of the content is 200x200 (note the surface scale=2). So the bounds actually do not need to be scaled. Worth noting here that the old code *did* scale the bounds here, which would make them 400x400 which is wrong. I'm certain that has been pretty confusing here. The transform scale of 1 here is right since we don't need to scale the pixels from the intrinsic size. The bounds also don't need to be scaled. Without UseZoomForDSF for web/compositor content: - CSS size=100x100 - Bounds=100x100 - Surface size=200x200 - scale = surfacesize / bounds = 200/100 = 2 - transform = 1/scale = 1/2 - Surface scale=2 - DSF=2 - PaintedDSF=1 Here the bounds do not match the intrinsic size of the content (surface scale=2). We can get the bounds to match the intrinsic size of the content by scaling by the surface scale which is what the old code did. This is the case that the old code was written for. The correct transform scale here would be 1 as the intrinsic size is already scaled, to get it drawn over 200x200 pixels in the target. However, since the bounds do not match the intrinsic size, we can't just use them directly. That's why the old code scaled bounds by the surface scale. In all four cases, from what I can see, if you multiply the bounds by "Surface scale" / PaintedDSF, you get bounds in the space of the content / origin space / intrinsic space. And if you take 1 / surface size / bounds / DSF, you get a correct scale from the pixels of the intrinsic space to the target space. Does this hold for the case where the surface size (intrinsic size) is different from the layer bounds? UseZoomForDSF for canvas: - CSS size=300x300 - Bounds=600x600 - Surface size=400x100 - Surface scale=1 - DSF=1 - PaintedDSF=2 Bounds * (Surface scale / PaintedDSF) = 300x300 <- Bounds in Origin space / Intrinsic space. The intrinsic size is relative to these. 1 / (Surface size / bounds / DSF) = Scale[6/4,6/1] <- Transform from Origin space to target space. If you multiply 400x100 by this you get 600x600 which is the right number of pixels in the target. Not-UseZoomForDSF for canvas: - CSS size=300x300 - Bounds=300x300 - Surface size=400x100 - Surface scale=1 - DSF=2 - PaintedDSF=1 Bounds * (Surface scale / PaintedDSF) = 300x300 <- Note same result yay 1 / (Surface size / bounds / DSF) = Scale[6/4,6/1] <- Same result again UseZoomForDSF for web/compositor: - CSS size=300x300 - Bounds=600x600 - Surface size=800x200 - Surface scale=2 - DSF=1 - PaintedDSF=2 Bounds * (Surface scale / PaintedDSF) = 600x600 <- The intrinsic size of web content is doubled compared to canvas since it's scaled by the DSF. 1 / (Surface size / bounds / DSF) = Scale[6/8,6/2] <- The transform scales by 1/2 of the above to account for the source pixels being twice as many, to cover the space space in the target. 800x200 scaled by this gives 600x600 which is what we want still. Not-UseZoomForDSF for web/compositor: - CSS size=300x300 - Bounds=300x300 - Surface size=800x200 - Surface scale=2 - DSF=2 - PaintedDSF=1 Bounds * (Surface scale / PaintedDSF) = 600x600 <- Same result, good. 1 / (Surface size / bounds / DSF) = Scale[6/8,6/2] <- Same again, yay. > You might ask why bounds(), surface_size_ and DSF, PDSF, surface_scale all look > different (in fact, opposite) in both cases. weiliangc@ and I are making a guess > that Chromium and Blink handle all these differently but exactly where are the > part handling this is far more complex than we thought and after doing an > intensive code search we still cannot see a full picture clearly. > I have to admit that I figure out my computation based on the standard output > printing different variables (PDSF, DSF, bounds, etc) on different test cases on > high DPI and low DPI screens, couple with a lot of trial-and-error efforts to > make all of them correct. I think we had 2 cases to consider before, and now we have 4. Before: UseZoomForDSF + web/compositor output !UseZoomForDSF + web/compositor output Now also: UseZoomForDSF + canvas output !UseZoomForDSF + canvas output The difference between these from what you wrote above appears to be that the web/compositor output intrinsic size is scaled by the DSF, but canvas' is not. Also the code being wrong for UseZoomForDSF is a thing. When you get the layer rect wrong what happens is we won't use AA on the edges correctly when the surface is rotated. This doesn't happen much for existing use cases so I can see that not being noticed. When you get the visible rect wrong I'm not sure what happens for surface quads right now. Other quads use this in GL renderer to avoid overdrawing, but I'm not sure if it's used (right now) for surface quads.
Haven't read through but I'll just try reply to first part first. On 2016/11/22 at 19:19:13, danakj wrote: > > It is a bit complicated to explain why. Let me list out the standard printout on > > bounds and > > surface_size on device-scale-factor=2 case, coupled with the css style change. > > Let's say css forces canvas to be 400X100, intrinsic canvas size is 300X300. > > > > (1) layer bounds() for canvas's layer is 800X200, which means it already takes > > account the style change and is already double based on device_scale_factor. > > This is true when UseZoomForDSF is true, but when it is false, the layer bounds will stay 400x100. > > > (2) surface_size remains as 300X300 and surface_scale_ remains as 1. > > So intrinsic canvas size does not change if your device-scale-factor changes, is my take away here? > > > Let's start with canvas and css match and both are 100x100, with DSF=2. In that case the canvas should cover 200x200 pixels in its target when it is drawn. So I think one thing missing here is when DSF=2, cc puts that inside every target_draw_transform. > > With UseZoomForDSF for a canvas: > - CSS size=100x100 > - Bounds=200x200 > - Surface size=100x100 > - scale = surfacesize / bounds = 100/200 = 1/2 > - transform = 1/scale = 2 > > This says that when going from the layer to its target everything grows by 2x so the 100x100 pixels in the canvas will cover 200x200 pixels in the target, giving what we want under DSF=2. > > > Without UseZoomForDSF for a canvas (did you try this mode is it true? this is how you can change it https://cs.chromium.org/chromium/src/content/common/content_switches_internal... to use the mode): > - CSS size=100x100 > - Bounds=100x100 > - Surface size=100x100 > - scale = surfacesize / bounds = 100/100 = 1 > - transform = 1/scale = 1 ^ transform = draw_transform / scale = 2/scale = 2 > > Here we'll take the 100x100 pixels from the canvas and map them to 100x100 pixels in the target. This seems wrong we want it to cover 200x200 pixels in the target. > > > With UseZoomForDSF for web/compositor content: > - CSS size=100x100 > - Bounds=200x200 > - Surface size=200x200 > - scale = surfacesize / bounds = 200/200 = 1 > - transform = 1/scale = 1 > > Here we have already DSF-scaled content of 200x200 and we transform it by identity so we get 200x200 pixels in the target, great. > > Without UseZoomForDSF for web/compositor content: > - CSS size=100x100 > - Bounds=100x100 > - Surface size=200x200 > - scale = surfacesize / bounds = 200/100 = 2 > - transform = 1/scale = 1/2 ^ transform = draw_transform / scale = 2/scale = 2/2 =1 > > I think that things go awry here again. Web content is scaled so you have a 200x200 surface, but since we're not considering the DSF here we end scaling the content by 1/2 to fill 100x100 pixels in the target which is incorrect. yay?
On 2016/11/22 20:28:40, weiliangc wrote: > Haven't read through but I'll just try reply to first part first. > Another comment I want to add on top of weiliangc@'s comment is that I've just obtained the experimental data for all four cases: Case 1: Enable UseZoomForDSF + web/compositor output bounds: same surface_size: double surface_scale: 2 DSF: 2 PDSF: 1 scale_x: 1/2 scale_y: 1/2 Case 2: Enable UseZoomForDSF + canvas output bounds: double (800X200) surface_size: same (300X300) surface_scale: 1 DSF: 1 PDSF: 2 scale_x: 8/3 scale_y: 2/3 Case 3: Disable UseZoomForDSF + web/compositor output bounds: same surface_size: double surface_scale: 2 DSF: 2 PDSF: 1 scale_x: 1/2 scale_y: 1/2 Case 4: Disable UseZoomForDSF + canvas output bounds: same (400X100) surface_size: same (300X300) surface_scale: 1 DSF: 2 PDSF: 1 scale_x: 4/3 scale_y: 1/3 If you pay attention to the difference between Case 2 and Case 4, you may find that after combining all these differences, if I replace the gfx::Size scaled_bounds = gfx::ScaleToCeiledSize(bounds(), layer_tree_impl()->device_scale_factor()); inside PopulateScaledSharedQuadState Then the scaled_bounds is going to be always twice in all four cases. My observation is that Both DisableUseZoomForDSF and EnableUseZoomForDSF display the correct canvas image enlarged twice for dsf=2. WDYT?
On 2016/11/22 at 20:28:40, weiliangc wrote: > Haven't read through but I'll just try reply to first part first. > > On 2016/11/22 at 19:19:13, danakj wrote: > > > > It is a bit complicated to explain why. Let me list out the standard printout on > > > bounds and > > > surface_size on device-scale-factor=2 case, coupled with the css style change. > > > Let's say css forces canvas to be 400X100, intrinsic canvas size is 300X300. > > > > > > (1) layer bounds() for canvas's layer is 800X200, which means it already takes > > > account the style change and is already double based on device_scale_factor. > > > > This is true when UseZoomForDSF is true, but when it is false, the layer bounds will stay 400x100. > > > > > (2) surface_size remains as 300X300 and surface_scale_ remains as 1. > > > > So intrinsic canvas size does not change if your device-scale-factor changes, is my take away here? > > > > > > Let's start with canvas and css match and both are 100x100, with DSF=2. In that case the canvas should cover 200x200 pixels in its target when it is drawn. > > So I think one thing missing here is when DSF=2, cc puts that inside every target_draw_transform. > > > > > With UseZoomForDSF for a canvas: > > - CSS size=100x100 > > - Bounds=200x200 > > - Surface size=100x100 > > - scale = surfacesize / bounds = 100/200 = 1/2 > > - transform = 1/scale = 2 > > > > This says that when going from the layer to its target everything grows by 2x so the 100x100 pixels in the canvas will cover 200x200 pixels in the target, giving what we want under DSF=2. > > > > > > Without UseZoomForDSF for a canvas (did you try this mode is it true? this is how you can change it https://cs.chromium.org/chromium/src/content/common/content_switches_internal... to use the mode): > > - CSS size=100x100 > > - Bounds=100x100 > > - Surface size=100x100 > > - scale = surfacesize / bounds = 100/100 = 1 > > - transform = 1/scale = 1 > > ^ transform = draw_transform / scale = 2/scale = 2 > > > > > Here we'll take the 100x100 pixels from the canvas and map them to 100x100 pixels in the target. This seems wrong we want it to cover 200x200 pixels in the target. > > > > > > With UseZoomForDSF for web/compositor content: > > - CSS size=100x100 > > - Bounds=200x200 > > - Surface size=200x200 > > - scale = surfacesize / bounds = 200/200 = 1 > > - transform = 1/scale = 1 > > > > Here we have already DSF-scaled content of 200x200 and we transform it by identity so we get 200x200 pixels in the target, great. > > > > Without UseZoomForDSF for web/compositor content: > > - CSS size=100x100 > > - Bounds=100x100 > > - Surface size=200x200 > > - scale = surfacesize / bounds = 200/100 = 2 > > - transform = 1/scale = 1/2 > ^ transform = draw_transform / scale = 2/scale = 2/2 =1 > > > > I think that things go awry here again. Web content is scaled so you have a 200x200 surface, but since we're not considering the DSF here we end scaling the content by 1/2 to fill 100x100 pixels in the target which is incorrect. > > yay? Also for the quad_layer_bounds, I think scaling it by surface_scale is OK, we just need to make sure Canvas also set surface_scale matching device_scale_factor, which is the assumption for surface_scale.
After discussion with danakj@ and weiliangc@, I give up trying a universal approach that works for all four cases because of risks in breaking existing Surface in compositing. Instead, I introduce a boolean in SurfaceLayerImpl that is going to be false at default, and true only for HTML canvas placeholder. When it is true, the SurfaceLayerImpl::AppendQuads will scale quad_to_target_transform with x and y, and bounds() (which already has x and y scaled) with DSF. Now the code for everyone else except canvas remains the same. I just double check the canvas case and for both nonZoom case and Zoom case, for both DSF=2 and DSF=1, the SQS->quad_layer_bounds is always having the same size as DrawQuad->rect after applying SQS->quad_to_target_transform. So now it is correct.
The CQ bit was checked by xlai@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 checked by xlai@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...
Patchset #8 (id:160001) has been deleted
Dry run: Try jobs failed on following builders: linux_chromium_clobber_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 xlai@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...
After talking with weiliangc@, agree to add surface_scale_ into the computation to make it more consistent with existing code and also to prevent future refactoring on surface_scale_ to break in canvas case. In a summary, the following equation is going to always correct for non-UseZoomForDSF and UseZoomForDSF cases for canvas (note that we now keep the same code for non-canvas cases so we only consider the canvas cases her): quad_size X quad_to_target_transformation = bounds X DSF where quad_size = surface_size quad_to_target_transformation = draw_properties.target_space_transform X (scale_x, scale_y) / surface_scale (scale_x, scale_y) = bounds / surface_size Now we look at why this equation is always correct in non-Zoom and Zoom cases: In the case when canvas intrinsic size is 300X300, style size is 400X100, and in a high DPI (scale 2) settings, surface_size is still 300X300, surface_scale is still 1, non-Zoom case: 1. draw_properties.target_space_transform is scaled by 2, 2. bounds is still 400X100 3, DSF=1 LHS = (300,300)*2*(400,100)/(300, 300)/1 = (800,200) RHS = (400,100)*2 = (800, 200) Zoom case: 1. draw_properties.target_space_transform is scaled by 1, 2. bounds is now 800X200 3. DSF=2 LHS = (300,300)*1*(800,200)/(300,300)/1 = (800, 200) RHS = (800, 200)*1 = (800, 200) In conclusion, the formula is correct for both non-Zoom and Zoom case.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/11/23 20:38:12, xlai (Olivia) wrote: > After talking with weiliangc@, agree to add surface_scale_ into the computation > to make it more consistent with existing code and also to prevent future > refactoring on surface_scale_ to break in canvas case. > > In a summary, the following equation is going to always correct for > non-UseZoomForDSF and UseZoomForDSF cases for canvas (note that > we now keep the same code for non-canvas cases so we only consider the canvas > cases her): > > quad_size X quad_to_target_transformation = bounds X DSF This should be true, but only if the surface layer's transform doesn't include any scale other than the DSF. Otherwise they will be clearly different by that scale factor (or other transformation such as skew or whatever), because the right hand side doesn't include it but the left hand side does. > > where > > quad_size = surface_size > quad_to_target_transformation = draw_properties.target_space_transform X > (scale_x, scale_y) / surface_scale scale_x and scale_y should not be relevant here, as we're stretching the content to exactly fit the layer bounds, and the scale of the surface is already a component of the surface's size, so this would represent it in the equation twice. And if the canvas was 2x it would shrink the result relative to the layer bounds. So surface_scale doesn't belong here. What's missing in these descriptions is a definition of each coordinate space. Each transform defines a way to move between spaces, which should be well defined. To start with, you have the following spaces: - The layer space is the space relative to the position/bounds of the layer. In this space the layer's position is always 0,0 and it's size is exactly Layer::bounds(). - The target space for the layer is the space where it will be drawn by the compositor. - You apply the draw_properties.target_space_transform to move from the layer space to the target space. So for example, Layer::bounds() X draw_properties.target_space_transform will give you the bounds of the layer inside the target space. Or (0,0)-position X draw_properties.target_space_transform will give you the top left corner of the layer inside the target space. So the spaces we have in this code also includes the *content space* of the surface. For canvas you've referred to this in terms like the intrinsic canvas size. This is the coordinate space in which the canvas draws its content. Its origin (0,0) is the top left of the canvas' bitmap, and the canvas' size is exactly the intrinsic size in this space - so it's an identity space for the canvas' content if that makes sense? The quad rect is defined in this content space. We can tell this because the rect is Rect(surface_size_). It's not transforming the size at all, so it's making a rect the size of the canvas's backing in the content space. The draw_properties.target_space_transform moves from layer space to target space. What we want is to define a transform from the content space to the target space. To do that we need a transform from content space to layer space. Then concat them and that is what we should put in the SQS. For us, the mapping from content space to layer space is exactly the scale_x/scale_y you mention, as that maps each pixel in the content to a position in the layer bounds. That is, surface_size_ / layer_bounds. Note this is exactly defining a relationship between these two spaces as it is a ratio of rects defined in each of these two spaces, the content space and the layer space. TARGET SPACE LAYER SPACE +-------------+ target space xform +-------+ | | <------------------- |0,0 | | +-----+ | | | | |LAYER| | +-------+ | +-----+ | bounds^ | | +-------------+ LAYER SPACE CONTENT SPACE +-------+ content space xform +-----------+ |0,0 | <-------------------- |0,0 | | | +-----------+ +-------+ surface_size^ bounds^ The transform in the SQS needs to be used to map the quad_rect (which is defined in CONTENT SPACE) to the TARGET SPACE. So it is just the |target space xform| X |content space xform|. Our job in this CL is to define the content space xform. Phrasing the code in this way to clearly identify coordinate spaces will make it much more understandable. Secondly. We have a rect (layer bounds) in the LAYER SPACE, and we need to move it to the same space as the quad_rect, which is in CONTENT SPACE. We do this by applying the inverse of the content space xform. The code in LayerImpl::PopulateSQS was doing this with just a simple scale before. But since our content space xform will be more than just a single-dimentional scale, we will need the inverse function to also be more than a single-dimensional scale, it should be scaled in x and y dimensions by the inverse of how we map from CONTENT SPACE to LAYER SPACE. The analogous example from existing code is for PictureLayerImpl. In that case it has a CONTENT SPACE that is larger than the LAYER SPACE by a single scale factor which we called |scale| here: https://cs.chromium.org/chromium/src/cc/layers/layer_impl.cc?rcl=0&l=184 The transform from CONTENT to LAYER SPACE then is 1/scale, which is what we append to the draw transform on that line. The next line does the inverse, to map the layer's bounds back into the CONTENT SPACE from their LAYER SPACE. The code here for canvas should look something similar, except that there are 2 scales, one for x and one for y. This was much more complicated earlier because we were trying to somehow incorporate the non-canvas case which is *very* different in how it deals with DSF, but thankfully we're not now - we simply want the content to fill the bounds of the layer, independent of device scales. The result should look just like the existing PopulateSQS I think, but it just needs to take 2 scale factors instead of 1 (for x and y), and existing callers could call it with the same scale for x and y, eliminating the need to fork that function. Let me know if this makes sense. > (scale_x, scale_y) = bounds / surface_size > > > Now we look at why this equation is always correct in non-Zoom and Zoom cases: > > In the case when canvas intrinsic size is 300X300, style size is 400X100, and in > a high DPI (scale 2) settings, > surface_size is still 300X300, surface_scale is still 1, > > non-Zoom case: > 1. draw_properties.target_space_transform is scaled by 2, > 2. bounds is still 400X100 > 3, DSF=1 > > LHS = (300,300)*2*(400,100)/(300, 300)/1 = (800,200) > RHS = (400,100)*2 = (800, 200) > > Zoom case: > 1. draw_properties.target_space_transform is scaled by 1, > 2. bounds is now 800X200 > 3. DSF=2 > > LHS = (300,300)*1*(800,200)/(300,300)/1 = (800, 200) > RHS = (800, 200)*1 = (800, 200) > > > In conclusion, the formula is correct for both non-Zoom and Zoom case.
On 2016/11/24 at 02:09:47, danakj wrote: > On 2016/11/23 20:38:12, xlai (Olivia) wrote: > > After talking with weiliangc@, agree to add surface_scale_ into the computation > > to make it more consistent with existing code and also to prevent future > > refactoring on surface_scale_ to break in canvas case. > > > > In a summary, the following equation is going to always correct for > > non-UseZoomForDSF and UseZoomForDSF cases for canvas (note that > > we now keep the same code for non-canvas cases so we only consider the canvas > > cases her): > > > > quad_size X quad_to_target_transformation = bounds X DSF > > This should be true, but only if the surface layer's transform doesn't include any scale other than the DSF. Otherwise they will be clearly different by that scale factor (or other transformation such as skew or whatever), because the right hand side doesn't include it but the left hand side does. > > > > > where > > > > quad_size = surface_size > > quad_to_target_transformation = draw_properties.target_space_transform X > > (scale_x, scale_y) / surface_scale > > scale_x and scale_y should not be relevant here, as we're stretching the content to exactly fit the layer bounds, and the scale of the surface is already a component of the surface's size, so this would represent it in the equation twice. And if the canvas was 2x it would shrink the result relative to the layer bounds. So surface_scale doesn't belong here. > > What's missing in these descriptions is a definition of each coordinate space. Each transform defines a way to move between spaces, which should be well defined. To start with, you have the following spaces: > > - The layer space is the space relative to the position/bounds of the layer. In this space the layer's position is always 0,0 and it's size is exactly Layer::bounds(). > - The target space for the layer is the space where it will be drawn by the compositor. > - You apply the draw_properties.target_space_transform to move from the layer space to the target space. > > So for example, Layer::bounds() X draw_properties.target_space_transform will give you the bounds of the layer inside the target space. Or (0,0)-position X draw_properties.target_space_transform will give you the top left corner of the layer inside the target space. > > So the spaces we have in this code also includes the *content space* of the surface. For canvas you've referred to this in terms like the intrinsic canvas size. This is the coordinate space in which the canvas draws its content. Its origin (0,0) is the top left of the canvas' bitmap, and the canvas' size is exactly the intrinsic size in this space - so it's an identity space for the canvas' content if that makes sense? > > The quad rect is defined in this content space. We can tell this because the rect is Rect(surface_size_). It's not transforming the size at all, so it's making a rect the size of the canvas's backing in the content space. > > The draw_properties.target_space_transform moves from layer space to target space. What we want is to define a transform from the content space to the target space. To do that we need a transform from content space to layer space. Then concat them and that is what we should put in the SQS. > > For us, the mapping from content space to layer space is exactly the scale_x/scale_y you mention, as that maps each pixel in the content to a position in the layer bounds. That is, surface_size_ / layer_bounds. Note this is exactly defining a relationship between these two spaces as it is a ratio of rects defined in each of these two spaces, the content space and the layer space. > > TARGET SPACE LAYER SPACE > +-------------+ target space xform +-------+ > | | <------------------- |0,0 | > | +-----+ | | | > | |LAYER| | +-------+ > | +-----+ | bounds^ > | | > +-------------+ > > LAYER SPACE CONTENT SPACE > +-------+ content space xform +-----------+ > |0,0 | <-------------------- |0,0 | > | | +-----------+ > +-------+ surface_size^ > bounds^ > > The transform in the SQS needs to be used to map the quad_rect (which is defined in CONTENT SPACE) to the TARGET SPACE. So it is just the |target space xform| X |content space xform|. Our job in this CL is to define the content space xform. Phrasing the code in this way to clearly identify coordinate spaces will make it much more understandable. > > Secondly. We have a rect (layer bounds) in the LAYER SPACE, and we need to move it to the same space as the quad_rect, which is in CONTENT SPACE. We do this by applying the inverse of the content space xform. > > The code in LayerImpl::PopulateSQS was doing this with just a simple scale before. But since our content space xform will be more than just a single-dimentional scale, we will need the inverse function to also be more than a single-dimensional scale, it should be scaled in x and y dimensions by the inverse of how we map from CONTENT SPACE to LAYER SPACE. > > The analogous example from existing code is for PictureLayerImpl. In that case it has a CONTENT SPACE that is larger than the LAYER SPACE by a single scale factor which we called |scale| here: https://cs.chromium.org/chromium/src/cc/layers/layer_impl.cc?rcl=0&l=184 > > The transform from CONTENT to LAYER SPACE then is 1/scale, which is what we append to the draw transform on that line. The next line does the inverse, to map the layer's bounds back into the CONTENT SPACE from their LAYER SPACE. > > The code here for canvas should look something similar, except that there are 2 scales, one for x and one for y. This was much more complicated earlier because we were trying to somehow incorporate the non-canvas case which is *very* different in how it deals with DSF, but thankfully we're not now - we simply want the content to fill the bounds of the layer, independent of device scales. The result should look just like the existing PopulateSQS I think, but it just needs to take 2 scale factors instead of 1 (for x and y), and existing callers could call it with the same scale for x and y, eliminating the need to fork that function. > > Let me know if this makes sense. Oops my fault I was a bit confused again, but totally get it now and explained on whiteboard so should be good to go now. :D > > > (scale_x, scale_y) = bounds / surface_size > > > > > > Now we look at why this equation is always correct in non-Zoom and Zoom cases: > > > > In the case when canvas intrinsic size is 300X300, style size is 400X100, and in > > a high DPI (scale 2) settings, > > surface_size is still 300X300, surface_scale is still 1, > > > > non-Zoom case: > > 1. draw_properties.target_space_transform is scaled by 2, > > 2. bounds is still 400X100 > > 3, DSF=1 > > > > LHS = (300,300)*2*(400,100)/(300, 300)/1 = (800,200) > > RHS = (400,100)*2 = (800, 200) > > > > Zoom case: > > 1. draw_properties.target_space_transform is scaled by 1, > > 2. bounds is now 800X200 > > 3. DSF=2 > > > > LHS = (300,300)*1*(800,200)/(300,300)/1 = (800, 200) > > RHS = (800, 200)*1 = (800, 200) > > > > > > In conclusion, the formula is correct for both non-Zoom and Zoom case.
The CQ bit was checked by xlai@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Thanks a lot for the super detailed explanation:-) Now I can clearly
differentiate the difference
between content space, layer space and target space. I fixed my latest patch and
here I can
explain why it is right this time.
The correct equation I should aim to achieve is a matching between quad and
layer, both in
target space:
LHS= quad in target space = quad in content space * content_to_layer_transform *
layer_to_target_transform
RHS= layer in target space = layer in layer space * layer_to_target_transform
where
layer_to_target_transform = draw_properties.target_space_transform
content_to_layer_transform = the transformation that I'm changing in this CL
which I set it to be layer_bounds / surface_size
quad in content space = surface_size
layer in layer space = layer_bounds
So to expand the equation it is actually:
LHS=surface_size * layer_bounds / surface_size *
draw_properties.target_space_transform
= layer_bounds * draw_properties.target_space_transform
RHS= layer_bounds * draw_properties.target_space_transform
= LHS
So this equation would be always right if I makes sure that
content_to_layer_transform is layer_bounds/surface_size.
Also, the SQS->quad_to_target_transform is actually content_layer_transform *
layer_to_target_transform.
Previously I misunderstood SQS->quad_layer_bounds and put it into the equation.
Now I know
that it is layer bounds in content space, and that's why a reverse of scale is
applied to bounds()
to get the SQS->quad_layer_bounds.
In my latest patch, I put scale_x and scale_y to be surface_size / layer_bounds
and then pass it
back to PopulateSharedQuadState(state, scale_x, scale_y) where:
1. content_to_layer_transform is set to be an inverse of (scale_x, scale_y)
==> As a result, the equation that I describe above would be balanced.
2. SQS->quad_layer_bounds is set to be layer_bounds scaled by (scale_x, scale_y)
==> As a result, it will be back to surface_size, which is exactly the
layer bounds in content space
After I fix this, I find that it is exactly matching the original code in
PopulateSharedQuadState(state, scale) and
so now I can unify two function by letting PopulateSharedQuadState(state, scale)
to call
PopulateSharedQuadState(state, scale, scale).
LG thanks, some smaller comments https://codereview.chromium.org/2495373003/diff/200001/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/2495373003/diff/200001/cc/layers/layer_impl.c... cc/layers/layer_impl.cc:180: void LayerImpl::PopulateScaledSharedQuadState(SharedQuadState* state, I don't think we need 2 methods really. How about just having existing callers call the one below, passing the same value twice? https://codereview.chromium.org/2495373003/diff/200001/cc/layers/layer_impl.c... cc/layers/layer_impl.cc:186: float scale_x, can you rename these input vars to all be like "layer_to_content_scale" instead of just "scale"? https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... File cc/layers/surface_layer.cc (right): https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer.cc:54: : surface_scale_(1.f), can you put this in the header? the new var is uninit, and it's easier to see that when you expect it to be init in the header. we should move this there too to have them all together. https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_layer.h File cc/layers/surface_layer.h (right): https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer.h:38: bool scale_layer_bounds_with_surface_size = false); i'd prefer no default behaviour here, just update callsites. https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer.h:38: bool scale_layer_bounds_with_surface_size = false); also, I think "stretch_content_to_fill_bounds" might be a more clear name to express intent. a comment that the |scale| is ununsed when this is true would be helpful too. https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer.h:65: bool scale_layer_bounds_with_surface_size_; = false https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... File cc/layers/surface_layer_impl.cc (right): https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer_impl.cc:20: surface_scale_(0.f), can you move both of these to the .h https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer_impl.cc:63: scale_layer_bounds_with_surface_size_ = is_scaling_needed; You should NoteLayerPropertyChanged() when something changes that affects how the layer's contents are drawn. Also otherwise we might not pushproperties on the layer, so you'd have to notify that push props is needed otherwise. https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer_impl.cc:85: // When a DOM element's size in CSS style is changed and it is using DOM and CSS are foreign concepts to cc/ so can you rewrite this without using blink terminology? Just speak in terms of layers and surface content. https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer_impl.cc:89: float scale_x = ((float)surface_size_.width()) / bounds().width(); static_cast, not c-style cast https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... File cc/layers/surface_layer_impl.h (right): https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer_impl.h:51: // This sets whether SurfaceLayerImpl needs to compute the SharedQuadState's This says what the variable does but a better comment explains why/intent. Maybe my name suggestion helps and you don't need a comment then, but the point of this should explain that when this is true the contents in the surface are stretched to fit exactly within the bounds of the layer. https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... File cc/layers/surface_layer_impl_unittest.cc (right): https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer_impl_unittest.cc:77: // Given condition what is interesting about the given condition? here it's that the layer and surface have different sizes and different aspect ratios https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer_impl_unittest.cc:84: // Surface_layer_impl will be set after PushProperties. Does this mean to say the below is mimicing the behaviour of PushPropertiesTo from the pending to active tree? https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer_impl_unittest.cc:93: // Calling test function AppendQuads. This comment isn't adding value https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer_impl_unittest.cc:101: A comment explaining the math below is helpful in a test. Like // We expect that the transform for the quad stretches the quad to cover the entire bounds of the layer. or something? https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer_impl_unittest.cc:102: gfx::Transform transform(target_space_transform); expected_transform is a helpful name https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer_impl_unittest.cc:103: float scale_x = ((float)surface_size.width()) / layer_size.width(); static_cast not c-style https://google.github.io/styleguide/cppguide.html#Casting https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer_impl_unittest.cc:111: transform.TransformRect(&quad_rect); Don't use gfx::Transform::TransformRect in cc/. Instead use cc::MathUtil::MapClippedRect. https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer_impl_unittest.cc:115: gfx::RectF layer_rect((float)layer_size.width(), (float)layer_size.height()); no c-style casts. you shouldn't need any cast here, you already specified RectF as the type? https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer_impl_unittest.cc:116: target_space_transform.TransformRect(&layer_rect); Don't use gfx::Transform::TransformRect in cc/. Instead use cc::MathUtil::MapClippedRect.
Patchset #11 (id:240001) has been deleted
Patchset #11 (id:260001) has been deleted
Patchset #11 (id:280001) has been deleted
I just come back from a long vacation and continue to fix this patch. danakj@: I have addressed all your comments now. PTAL. Thanks! https://codereview.chromium.org/2495373003/diff/200001/cc/layers/layer_impl.cc File cc/layers/layer_impl.cc (right): https://codereview.chromium.org/2495373003/diff/200001/cc/layers/layer_impl.c... cc/layers/layer_impl.cc:180: void LayerImpl::PopulateScaledSharedQuadState(SharedQuadState* state, On 2016/11/28 22:26:22, danakj_OOO_and_slow wrote: > I don't think we need 2 methods really. How about just having existing callers > call the one below, passing the same value twice? Done. https://codereview.chromium.org/2495373003/diff/200001/cc/layers/layer_impl.c... cc/layers/layer_impl.cc:186: float scale_x, On 2016/11/28 22:26:22, danakj_OOO_and_slow wrote: > can you rename these input vars to all be like "layer_to_content_scale" instead > of just "scale"? Done. https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... File cc/layers/surface_layer.cc (right): https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer.cc:54: : surface_scale_(1.f), On 2016/11/28 22:26:22, danakj_OOO_and_slow wrote: > can you put this in the header? the new var is uninit, and it's easier to see > that when you expect it to be init in the header. we should move this there too > to have them all together. Done. https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_layer.h File cc/layers/surface_layer.h (right): https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer.h:38: bool scale_layer_bounds_with_surface_size = false); On 2016/11/28 22:26:22, danakj_OOO_and_slow wrote: > also, I think "stretch_content_to_fill_bounds" might be a more clear name to > express intent. a comment that the |scale| is ununsed when this is true would be > helpful too. Done. https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer.h:38: bool scale_layer_bounds_with_surface_size = false); On 2016/11/28 22:26:22, danakj_OOO_and_slow wrote: > i'd prefer no default behaviour here, just update callsites. Done. https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer.h:65: bool scale_layer_bounds_with_surface_size_; On 2016/11/28 22:26:22, danakj_OOO_and_slow wrote: > = false Done https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... File cc/layers/surface_layer_impl.cc (right): https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer_impl.cc:20: surface_scale_(0.f), On 2016/11/28 22:26:22, danakj_OOO_and_slow wrote: > can you move both of these to the .h Done. https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer_impl.cc:63: scale_layer_bounds_with_surface_size_ = is_scaling_needed; On 2016/11/28 22:26:22, danakj_OOO_and_slow wrote: > You should NoteLayerPropertyChanged() when something changes that affects how > the layer's contents are drawn. Also otherwise we might not pushproperties on > the layer, so you'd have to notify that push props is needed otherwise. Done. https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer_impl.cc:85: // When a DOM element's size in CSS style is changed and it is using On 2016/11/28 22:26:22, danakj_OOO_and_slow wrote: > DOM and CSS are foreign concepts to cc/ so can you rewrite this without using > blink terminology? Just speak in terms of layers and surface content. Done. https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer_impl.cc:89: float scale_x = ((float)surface_size_.width()) / bounds().width(); On 2016/11/28 22:26:22, danakj_OOO_and_slow wrote: > static_cast, not c-style cast Done. https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... File cc/layers/surface_layer_impl.h (right): https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer_impl.h:51: // This sets whether SurfaceLayerImpl needs to compute the SharedQuadState's On 2016/11/28 22:26:22, danakj_OOO_and_slow wrote: > This says what the variable does but a better comment explains why/intent. Maybe > my name suggestion helps and you don't need a comment then, but the point of > this should explain that when this is true the contents in the surface are > stretched to fit exactly within the bounds of the layer. Done. New name is quite self-explanatory. https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... File cc/layers/surface_layer_impl_unittest.cc (right): https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer_impl_unittest.cc:77: // Given condition On 2016/11/28 22:26:23, danakj_OOO_and_slow wrote: > what is interesting about the given condition? here it's that the layer and > surface have different sizes and different aspect ratios Done. https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer_impl_unittest.cc:84: // Surface_layer_impl will be set after PushProperties. On 2016/11/28 22:26:22, danakj_OOO_and_slow wrote: > Does this mean to say the below is mimicing the behaviour of PushPropertiesTo > from the pending to active tree? Done. https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer_impl_unittest.cc:93: // Calling test function AppendQuads. On 2016/11/28 22:26:23, danakj_OOO_and_slow wrote: > This comment isn't adding value Done. https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer_impl_unittest.cc:101: On 2016/11/28 22:26:23, danakj_OOO_and_slow wrote: > A comment explaining the math below is helpful in a test. Like > > // We expect that the transform for the quad stretches the quad to cover the > entire bounds of the layer. > > or something? Done. https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer_impl_unittest.cc:102: gfx::Transform transform(target_space_transform); On 2016/11/28 22:26:23, danakj_OOO_and_slow wrote: > expected_transform is a helpful name Done. https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer_impl_unittest.cc:103: float scale_x = ((float)surface_size.width()) / layer_size.width(); On 2016/11/28 22:26:23, danakj_OOO_and_slow wrote: > static_cast not c-style > https://google.github.io/styleguide/cppguide.html#Casting Done. https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer_impl_unittest.cc:115: gfx::RectF layer_rect((float)layer_size.width(), (float)layer_size.height()); On 2016/11/28 22:26:23, danakj_OOO_and_slow wrote: > no c-style casts. you shouldn't need any cast here, you already specified RectF > as the type? Done. https://codereview.chromium.org/2495373003/diff/200001/cc/layers/surface_laye... cc/layers/surface_layer_impl_unittest.cc:116: target_space_transform.TransformRect(&layer_rect); On 2016/11/28 22:26:22, danakj_OOO_and_slow wrote: > Don't use gfx::Transform::TransformRect in cc/. Instead use > cc::MathUtil::MapClippedRect. Done.
LGTM w/ a few last things https://codereview.chromium.org/2495373003/diff/300001/cc/layers/surface_laye... File cc/layers/surface_layer_impl.cc (right): https://codereview.chromium.org/2495373003/diff/300001/cc/layers/surface_laye... cc/layers/surface_layer_impl.cc:56: void SurfaceLayerImpl::SetStretchContentToFillBounds(bool is_scaling_needed) { nitto https://codereview.chromium.org/2495373003/diff/300001/cc/layers/surface_laye... cc/layers/surface_layer_impl.cc:82: // It is possible that the surface size and its layer bounds are changed I think this could be more clear on its intent. While width and height can change.. this is about them being different. How about this? // Stretches the surface contents to exactly fill the layer bounds, regardless of scale or aspect ratio differences. https://codereview.chromium.org/2495373003/diff/300001/cc/layers/surface_laye... File cc/layers/surface_layer_impl.h (right): https://codereview.chromium.org/2495373003/diff/300001/cc/layers/surface_laye... cc/layers/surface_layer_impl.h:29: void SetStretchContentToFillBounds(bool is_scaling_needed); nit: name the var stretch_content? https://codereview.chromium.org/2495373003/diff/300001/cc/layers/surface_laye... File cc/layers/surface_layer_impl_unittest.cc (right): https://codereview.chromium.org/2495373003/diff/300001/cc/layers/surface_laye... cc/layers/surface_layer_impl_unittest.cc:70: TEST(SurfaceLayerImplTest, LayerBoundsDiffFromSurfaceLayerSize) { nit: test name SurfaceStretchedToLayerBounds ? https://codereview.chromium.org/2495373003/diff/300001/content/renderer/child... File content/renderer/child_frame_compositing_helper.cc (right): https://codereview.chromium.org/2495373003/diff/300001/content/renderer/child... content/renderer/child_frame_compositing_helper.cc:208: surface_layer->SetSurfaceId(surface_id, scale_factor, frame_size, false); use a temp var or a comment to say the name of the |false| https://codereview.chromium.org/2495373003/diff/300001/ui/compositor/layer.cc File ui/compositor/layer.cc (right): https://codereview.chromium.org/2495373003/diff/300001/ui/compositor/layer.cc... ui/compositor/layer.cc:671: new_layer->SetSurfaceId(surface_id, scale, surface_size, false); use a temp var or a comment to say the name of the |false|
Description was changed from ========== Match html canvas which is transferred to OffscreenCanvas to CSS style This CL aims to make html canvas to match to CSS style restriction regardless whether it is transferred to OffscreenCanvas or not. In a usual compositor commit, when a css style is imposed on the html canvas to change its width and height, its layer will reset a different bounds. However, in the case when html canvas has transferred control to offscreen, it is using a SurfaceLayer, which directly use surface_size_ without incorporating layer bounds to append SurfaceDrawQuad. This CL makes SurfaceLayerImpl to take into account the special case when layer bounds is different from surface_size_ in AppendQuads by doing a transformation on the Quad. Also, it is also possible that the compositor commit due to style change happens before the Surface Layer for html canvas is created. To prevent flakiness, we force a compositor commit at the time when Surface Layer is created and registered to GraphicsLayers. BUG=655335, 655311 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Match html canvas which is transferred to OffscreenCanvas to CSS style This CL aims to make html canvas to match to CSS style restriction regardless whether it is transferred to OffscreenCanvas or not. In a usual compositor commit, when a css style is imposed on the html canvas to change its width and height, its layer will reset a different bounds. However, in the case when html canvas has transferred control to offscreen, it is using a SurfaceLayer, which directly use surface_size_ without incorporating layer bounds to append SurfaceDrawQuad. This CL makes SurfaceLayerImpl to take into account the special case when layer bounds is different from surface_size_ in AppendQuads by doing a transformation on the Quad. Also, it is also possible that the compositor commit due to style change happens before the Surface Layer for html canvas is created. To prevent flakiness, we force a compositor commit at the time when Surface Layer is created and registered to GraphicsLayers. BUG=655335, 652931 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== Match html canvas which is transferred to OffscreenCanvas to CSS style This CL aims to make html canvas to match to CSS style restriction regardless whether it is transferred to OffscreenCanvas or not. In a usual compositor commit, when a css style is imposed on the html canvas to change its width and height, its layer will reset a different bounds. However, in the case when html canvas has transferred control to offscreen, it is using a SurfaceLayer, which directly use surface_size_ without incorporating layer bounds to append SurfaceDrawQuad. This CL makes SurfaceLayerImpl to take into account the special case when layer bounds is different from surface_size_ in AppendQuads by doing a transformation on the Quad. Also, it is also possible that the compositor commit due to style change happens before the Surface Layer for html canvas is created. To prevent flakiness, we force a compositor commit at the time when Surface Layer is created and registered to GraphicsLayers. BUG=655335, 652931 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Match html canvas which is transferred to OffscreenCanvas to CSS style This CL aims to make html canvas to match to CSS style restriction regardless whether it is transferred to OffscreenCanvas or not. In a usual compositor commit, when a css style is imposed on the html canvas to change its width and height, its layer will reset a different bounds. However, in the case when html canvas has transferred control to offscreen, it is using a SurfaceLayer, which directly use surface_size_ without incorporating layer bounds to append SurfaceDrawQuad. This CL changes SurfaceLayerImpl to take into account the special case when layer bounds is different from surface_size by doing a transformation on the Quad, especially when the aspect ratio is different. The CL respects existing non-canvas cases on high-dpi devices and settings when enable-use-zoom-for-dsf is on or off. Also, it is also possible that the compositor commit due to style change happens before the Surface Layer for html canvas is created. To prevent flakiness, we force a compositor commit at the time when Surface Layer is created and registered to GraphicsLayers. BUG=655335, 652931 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by xlai@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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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 xlai@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 ========== Match html canvas which is transferred to OffscreenCanvas to CSS style This CL aims to make html canvas to match to CSS style restriction regardless whether it is transferred to OffscreenCanvas or not. In a usual compositor commit, when a css style is imposed on the html canvas to change its width and height, its layer will reset a different bounds. However, in the case when html canvas has transferred control to offscreen, it is using a SurfaceLayer, which directly use surface_size_ without incorporating layer bounds to append SurfaceDrawQuad. This CL changes SurfaceLayerImpl to take into account the special case when layer bounds is different from surface_size by doing a transformation on the Quad, especially when the aspect ratio is different. The CL respects existing non-canvas cases on high-dpi devices and settings when enable-use-zoom-for-dsf is on or off. Also, it is also possible that the compositor commit due to style change happens before the Surface Layer for html canvas is created. To prevent flakiness, we force a compositor commit at the time when Surface Layer is created and registered to GraphicsLayers. BUG=655335, 652931 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Match html canvas which is transferred to OffscreenCanvas to CSS style This CL aims to make html canvas to match to CSS style restriction regardless whether it is transferred to OffscreenCanvas or not. In a usual compositor commit, when a css style is imposed on the html canvas to change its width and height, its layer will reset a different bounds. However, in the case when html canvas has transferred control to offscreen, it is using a SurfaceLayer, which directly use surface_size_ without incorporating layer bounds to append SurfaceDrawQuad. This CL changes SurfaceLayerImpl to take into account the special case when layer bounds is different from surface_size by doing a transformation on the Quad, especially when the aspect ratio is different. The CL respects existing non-canvas cases on high-dpi devices and settings when enable-use-zoom-for-dsf is on or off. Also, it is also possible that the compositor commit due to style change happens before the Surface Layer for html canvas is created. To prevent flakiness, we force a compositor commit at the time when Surface Layer is created and registered to GraphicsLayers. TBR=khushalsagar@chromium.org, twellington@chromium.org, pfeldman@chromium.org BUG=655335, 652931 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xlai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org, junov@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2495373003/#ps360001 (title: "fix compilation error")
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": 360001, "attempt_start_ts": 1481851703498050,
"parent_rev": "2fe680f9d9db9b0b7e2ccf30ab4fee96d8a197bb", "commit_rev":
"7afab552b9d7d1710b8a82e977eeb6c4ebbde429"}
Message was sent while issue was closed.
Description was changed from ========== Match html canvas which is transferred to OffscreenCanvas to CSS style This CL aims to make html canvas to match to CSS style restriction regardless whether it is transferred to OffscreenCanvas or not. In a usual compositor commit, when a css style is imposed on the html canvas to change its width and height, its layer will reset a different bounds. However, in the case when html canvas has transferred control to offscreen, it is using a SurfaceLayer, which directly use surface_size_ without incorporating layer bounds to append SurfaceDrawQuad. This CL changes SurfaceLayerImpl to take into account the special case when layer bounds is different from surface_size by doing a transformation on the Quad, especially when the aspect ratio is different. The CL respects existing non-canvas cases on high-dpi devices and settings when enable-use-zoom-for-dsf is on or off. Also, it is also possible that the compositor commit due to style change happens before the Surface Layer for html canvas is created. To prevent flakiness, we force a compositor commit at the time when Surface Layer is created and registered to GraphicsLayers. TBR=khushalsagar@chromium.org, twellington@chromium.org, pfeldman@chromium.org BUG=655335, 652931 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Match html canvas which is transferred to OffscreenCanvas to CSS style This CL aims to make html canvas to match to CSS style restriction regardless whether it is transferred to OffscreenCanvas or not. In a usual compositor commit, when a css style is imposed on the html canvas to change its width and height, its layer will reset a different bounds. However, in the case when html canvas has transferred control to offscreen, it is using a SurfaceLayer, which directly use surface_size_ without incorporating layer bounds to append SurfaceDrawQuad. This CL changes SurfaceLayerImpl to take into account the special case when layer bounds is different from surface_size by doing a transformation on the Quad, especially when the aspect ratio is different. The CL respects existing non-canvas cases on high-dpi devices and settings when enable-use-zoom-for-dsf is on or off. Also, it is also possible that the compositor commit due to style change happens before the Surface Layer for html canvas is created. To prevent flakiness, we force a compositor commit at the time when Surface Layer is created and registered to GraphicsLayers. TBR=khushalsagar@chromium.org, twellington@chromium.org, pfeldman@chromium.org BUG=655335, 652931 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2495373003 ==========
Message was sent while issue was closed.
Committed patchset #14 (id:360001)
Message was sent while issue was closed.
Description was changed from ========== Match html canvas which is transferred to OffscreenCanvas to CSS style This CL aims to make html canvas to match to CSS style restriction regardless whether it is transferred to OffscreenCanvas or not. In a usual compositor commit, when a css style is imposed on the html canvas to change its width and height, its layer will reset a different bounds. However, in the case when html canvas has transferred control to offscreen, it is using a SurfaceLayer, which directly use surface_size_ without incorporating layer bounds to append SurfaceDrawQuad. This CL changes SurfaceLayerImpl to take into account the special case when layer bounds is different from surface_size by doing a transformation on the Quad, especially when the aspect ratio is different. The CL respects existing non-canvas cases on high-dpi devices and settings when enable-use-zoom-for-dsf is on or off. Also, it is also possible that the compositor commit due to style change happens before the Surface Layer for html canvas is created. To prevent flakiness, we force a compositor commit at the time when Surface Layer is created and registered to GraphicsLayers. TBR=khushalsagar@chromium.org, twellington@chromium.org, pfeldman@chromium.org BUG=655335, 652931 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2495373003 ========== to ========== Match html canvas which is transferred to OffscreenCanvas to CSS style This CL aims to make html canvas to match to CSS style restriction regardless whether it is transferred to OffscreenCanvas or not. In a usual compositor commit, when a css style is imposed on the html canvas to change its width and height, its layer will reset a different bounds. However, in the case when html canvas has transferred control to offscreen, it is using a SurfaceLayer, which directly use surface_size_ without incorporating layer bounds to append SurfaceDrawQuad. This CL changes SurfaceLayerImpl to take into account the special case when layer bounds is different from surface_size by doing a transformation on the Quad, especially when the aspect ratio is different. The CL respects existing non-canvas cases on high-dpi devices and settings when enable-use-zoom-for-dsf is on or off. Also, it is also possible that the compositor commit due to style change happens before the Surface Layer for html canvas is created. To prevent flakiness, we force a compositor commit at the time when Surface Layer is created and registered to GraphicsLayers. TBR=khushalsagar@chromium.org, twellington@chromium.org, pfeldman@chromium.org BUG=655335, 652931 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Committed: https://crrev.com/5e87e713d6ed8c36fb37b814c6b65e7096d4554e Cr-Commit-Position: refs/heads/master@{#438986} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/5e87e713d6ed8c36fb37b814c6b65e7096d4554e Cr-Commit-Position: refs/heads/master@{#438986}
Message was sent while issue was closed.
Couple typoes to fix in followup https://codereview.chromium.org/2495373003/diff/300001/cc/layers/surface_laye... File cc/layers/surface_layer_impl.cc (right): https://codereview.chromium.org/2495373003/diff/300001/cc/layers/surface_laye... cc/layers/surface_layer_impl.cc:82: // It is possible that the surface size and its layer bounds are changed "Strecthes" => "Stretches" https://codereview.chromium.org/2495373003/diff/360001/blimp/client/core/comp... File blimp/client/core/compositor/blimp_compositor.cc (right): https://codereview.chromium.org/2495373003/diff/360001/blimp/client/core/comp... blimp/client/core/compositor/blimp_compositor.cc:337: surface_size, false /* strecth_content_to_fill_bounds */); "strecth" => stretch https://codereview.chromium.org/2495373003/diff/360001/ui/android/delegated_f... File ui/android/delegated_frame_host_android.cc (right): https://codereview.chromium.org/2495373003/diff/360001/ui/android/delegated_f... ui/android/delegated_frame_host_android.cc:56: false /* strecth_content_to_fill_bounds */); "strecth" => stretch |
