Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1412663005:
Introduce painted-device-scale-factor and use it when --enable-use-zoom-for-dsf is specified. (Closed)
|
Created:
5 years, 2 months ago by oshima Modified:
5 years, 1 month ago CC:
achuith+watch_chromium.org, blink-reviews, blink-reviews-api_chromium.org, cc-bugs_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, davemoore+watch_chromium.org, dglazkov+blink, dzhioev+watch_chromium.org, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, oshima+watch_chromium.org, piman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce painted-device-scale-factor and use it when --enable-use-zoom-for-dsf is specified.
This only handles the main content. Other things (Inputs/popup/plugins etc) will be handled in separate CLs.
Note to reviewer: the name painted_device_scale_factor is proposed by danakj@ in https://docs.google.com/a/chromium.org/document/d/1_40BGUBIGCmII3FaXdh-daOoxcCar7TnVpH0hldYgO8/edit?disco=AAAAAS8YQhA
BUG=485650
R=danakj@chromium.org, piman@chromium.org, aelias@chromium.org, rbyers@chromium.org
TEST=LayerTreeHostPaintedDeviceScaleFactor
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/750cb4349069151c8ad1f446f92ecf66a2f9023f
Cr-Commit-Position: refs/heads/master@{#357245}
Patch Set 1 #
Total comments: 34
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #Patch Set 4 : #
Total comments: 13
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : remove mode check from RWC #Patch Set 8 : export switch, remove WebWidgetClient::deviceScaleFactor #
Total comments: 3
Patch Set 9 : updated comment #
Total comments: 4
Patch Set 10 : #Messages
Total messages: 60 (22 generated)
Description was changed from ========== Introduce painted-device-scale-factor and use it when --enable-use-zoom-for-dsf is specified. This only handles the main content. Inputs/popup/plugins will be handled in separate CLs. BUG=485650 R=danakj@chromium.org, piman@chromium.org, aelias@chromium.org, rbyers@chromium.org TEST=LayerTreeHostPaintedDeviceScaleFactor patch from issue 1412893002 at patchset 80001 (http://crrev.com/1412893002#ps80001) CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Introduce painted-device-scale-factor and use it when --enable-use-zoom-for-dsf is specified. This only handles the main content. Inputs/popup/plugins will be handled in separate CLs. BUG=485650 R=danakj@chromium.org, piman@chromium.org, aelias@chromium.org, rbyers@chromium.org TEST=LayerTreeHostPaintedDeviceScaleFactor CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
The CQ bit was checked by oshima@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412663005/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412663005/1
Description was changed from ========== Introduce painted-device-scale-factor and use it when --enable-use-zoom-for-dsf is specified. This only handles the main content. Inputs/popup/plugins will be handled in separate CLs. BUG=485650 R=danakj@chromium.org, piman@chromium.org, aelias@chromium.org, rbyers@chromium.org TEST=LayerTreeHostPaintedDeviceScaleFactor CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Introduce painted-device-scale-factor and use it when --enable-use-zoom-for-dsf is specified. This only handles the main content. Other things (Inputs/popup/plugins etc) will be handled in separate CLs. BUG=485650 R=danakj@chromium.org, piman@chromium.org, aelias@chromium.org, rbyers@chromium.org TEST=LayerTreeHostPaintedDeviceScaleFactor CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
oshima@chromium.org changed reviewers: - rbyers@chromium.org
The size of change is actually small, so please review all files. https://codereview.chromium.org/1412663005/diff/1/cc/trees/layer_tree_host_un... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/1412663005/diff/1/cc/trees/layer_tree_host_un... cc/trees/layer_tree_host_unittest.cc:6381: class LayerTreeHostTestPaintedDeviceScaleFactor : public LayerTreeHostTest { danakj@, I'm not sure if this is right place, way , enough to test. Let me know if you have any advice.
Description was changed from ========== Introduce painted-device-scale-factor and use it when --enable-use-zoom-for-dsf is specified. This only handles the main content. Other things (Inputs/popup/plugins etc) will be handled in separate CLs. BUG=485650 R=danakj@chromium.org, piman@chromium.org, aelias@chromium.org, rbyers@chromium.org TEST=LayerTreeHostPaintedDeviceScaleFactor CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Introduce painted-device-scale-factor and use it when --enable-use-zoom-for-dsf is specified. This only handles the main content. Other things (Inputs/popup/plugins etc) will be handled in separate CLs. Note to reviewer: the name painted_device_scale_factor is proposed by danakj@ in https://docs.google.com/document/d/1_40BGUBIGCmII3FaXdh-daOoxcCar7TnVpH0hldYg... BUG=485650 R=danakj@chromium.org, piman@chromium.org, aelias@chromium.org, rbyers@chromium.org TEST=LayerTreeHostPaintedDeviceScaleFactor CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Description was changed from ========== Introduce painted-device-scale-factor and use it when --enable-use-zoom-for-dsf is specified. This only handles the main content. Other things (Inputs/popup/plugins etc) will be handled in separate CLs. Note to reviewer: the name painted_device_scale_factor is proposed by danakj@ in https://docs.google.com/document/d/1_40BGUBIGCmII3FaXdh-daOoxcCar7TnVpH0hldYg... BUG=485650 R=danakj@chromium.org, piman@chromium.org, aelias@chromium.org, rbyers@chromium.org TEST=LayerTreeHostPaintedDeviceScaleFactor CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Introduce painted-device-scale-factor and use it when --enable-use-zoom-for-dsf is specified. This only handles the main content. Other things (Inputs/popup/plugins etc) will be handled in separate CLs. Note to reviewer: the name painted_device_scale_factor is proposed by danakj@ in https://docs.google.com/a/chromium.org/document/d/1_40BGUBIGCmII3FaXdh-daOoxc... BUG=485650 R=danakj@chromium.org, piman@chromium.org, aelias@chromium.org, rbyers@chromium.org TEST=LayerTreeHostPaintedDeviceScaleFactor CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
danakj@chromium.org changed reviewers: + enne@chromium.org
The cc parts look good, couple comments. Pretty hazy on the content parts and I left a few comments there, +enne to look at that too. https://codereview.chromium.org/1412663005/diff/1/cc/trees/layer_tree_host.cc File cc/trees/layer_tree_host.cc (left): https://codereview.chromium.org/1412663005/diff/1/cc/trees/layer_tree_host.cc... cc/trees/layer_tree_host.cc:591: unrelated change https://codereview.chromium.org/1412663005/diff/1/cc/trees/layer_tree_host.cc File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/1412663005/diff/1/cc/trees/layer_tree_host.cc... cc/trees/layer_tree_host.cc:102: painted_device_scale_factor_(0.f), default to 1? https://codereview.chromium.org/1412663005/diff/1/cc/trees/layer_tree_host.cc... cc/trees/layer_tree_host.cc:881: property_trees_.needs_rebuild = true; i don't think this is true, it's not used in property trees https://codereview.chromium.org/1412663005/diff/1/cc/trees/layer_tree_host_im... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/1412663005/diff/1/cc/trees/layer_tree_host_im... cc/trees/layer_tree_host_impl.cc:1511: active_tree_->painted_device_scale_factor() > 0.0f if it defaults to one you dont need an if here. you can just multiply them together? https://codereview.chromium.org/1412663005/diff/1/cc/trees/layer_tree_host_un... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/1412663005/diff/1/cc/trees/layer_tree_host_un... cc/trees/layer_tree_host_unittest.cc:6397: ASSERT_EQ(1.0f, layer_tree_host()->device_scale_factor()); EXPECT_EQ https://codereview.chromium.org/1412663005/diff/1/cc/trees/layer_tree_host_un... cc/trees/layer_tree_host_unittest.cc:6398: PostSetNextCommitForcesRedrawToMainThread(); not needed, we'll draw the first frame https://codereview.chromium.org/1412663005/diff/1/cc/trees/layer_tree_host_un... cc/trees/layer_tree_host_unittest.cc:6410: void SwapBuffersOnThread(LayerTreeHostImpl* host_impl, bool result) override { Can you just use https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/fake_outpu... to check this instead of a swap promise? https://codereview.chromium.org/1412663005/diff/1/cc/trees/layer_tree_impl.cc File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/1412663005/diff/1/cc/trees/layer_tree_impl.cc... cc/trees/layer_tree_impl.cc:537: layer_tree_host_impl_->SetFullRootLayerDamage(); i don't think this is needed, we'll get invalidations from blink if its scale factor changed and it repainted things so you could make this simply set_painted_de..sc..factor() in the header file? https://codereview.chromium.org/1412663005/diff/1/cc/trees/layer_tree_impl.h File cc/trees/layer_tree_impl.h (right): https://codereview.chromium.org/1412663005/diff/1/cc/trees/layer_tree_impl.h#... cc/trees/layer_tree_impl.h:200: void SetPaintedDeviceScaleFactor(float painted_device_scale_factor); nit: normally keep the set/get pair together for each var https://codereview.chromium.org/1412663005/diff/1/content/renderer/gpu/render... File content/renderer/gpu/render_widget_compositor.cc (right): https://codereview.chromium.org/1412663005/diff/1/content/renderer/gpu/render... content/renderer/gpu/render_widget_compositor.cc:625: layer_tree_host_->SetPaintedDeviceScaleFactor(device_scale); I think maybe you want a separate function in RWC and blink will call one or the other https://codereview.chromium.org/1412663005/diff/1/content/renderer/render_wid... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1412663005/diff/1/content/renderer/render_wid... content/renderer/render_widget.cc:783: gfx::ScaleSize(gfx::SizeF(visible_viewport_size), device_scale_factor_); It seems odd that you're scaling things here. And I'm not sure about the WebWidget having a different size depending on the mode, but I think +enne should have a look at this part. https://codereview.chromium.org/1412663005/diff/1/content/renderer/render_wid... content/renderer/render_widget.cc:2387: compositor_->setViewportSize(size_, physical_backing_size_); The function that takes 2 args here should probably not exist anymore, and it always ignores the first argument entirely. So this is all equivalent to always just passing the physical_backing_size_ https://codereview.chromium.org/1412663005/diff/1/content/renderer/render_wid... File content/renderer/render_widget.h (right): https://codereview.chromium.org/1412663005/diff/1/content/renderer/render_wid... content/renderer/render_widget.h:575: // extra lines?
I'm looking into dana's suggestion for the test, but other comments are addressed. https://codereview.chromium.org/1412663005/diff/1/cc/trees/layer_tree_host.cc File cc/trees/layer_tree_host.cc (left): https://codereview.chromium.org/1412663005/diff/1/cc/trees/layer_tree_host.cc... cc/trees/layer_tree_host.cc:591: On 2015/10/22 23:15:29, danakj wrote: > unrelated change Done. https://codereview.chromium.org/1412663005/diff/1/cc/trees/layer_tree_host.cc File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/1412663005/diff/1/cc/trees/layer_tree_host.cc... cc/trees/layer_tree_host.cc:102: painted_device_scale_factor_(0.f), On 2015/10/22 23:15:29, danakj wrote: > default to 1? I was using 0.f as a way to tell if which should be used. I changed the way you suggested. https://codereview.chromium.org/1412663005/diff/1/cc/trees/layer_tree_host.cc... cc/trees/layer_tree_host.cc:881: property_trees_.needs_rebuild = true; On 2015/10/22 23:15:29, danakj wrote: > i don't think this is true, it's not used in property trees Done. https://codereview.chromium.org/1412663005/diff/1/cc/trees/layer_tree_host_im... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/1412663005/diff/1/cc/trees/layer_tree_host_im... cc/trees/layer_tree_host_impl.cc:1511: active_tree_->painted_device_scale_factor() > 0.0f On 2015/10/22 23:15:29, danakj wrote: > if it defaults to one you dont need an if here. you can just multiply them > together? Done. https://codereview.chromium.org/1412663005/diff/1/cc/trees/layer_tree_host_un... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/1412663005/diff/1/cc/trees/layer_tree_host_un... cc/trees/layer_tree_host_unittest.cc:6397: ASSERT_EQ(1.0f, layer_tree_host()->device_scale_factor()); On 2015/10/22 23:15:29, danakj wrote: > EXPECT_EQ Done. https://codereview.chromium.org/1412663005/diff/1/cc/trees/layer_tree_host_un... cc/trees/layer_tree_host_unittest.cc:6398: PostSetNextCommitForcesRedrawToMainThread(); On 2015/10/22 23:15:29, danakj wrote: > not needed, we'll draw the first frame Done. https://codereview.chromium.org/1412663005/diff/1/cc/trees/layer_tree_host_un... cc/trees/layer_tree_host_unittest.cc:6410: void SwapBuffersOnThread(LayerTreeHostImpl* host_impl, bool result) override { On 2015/10/22 23:15:29, danakj wrote: > Can you just use > https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/fake_outpu... > to check this instead of a swap promise? Thanks, I'll look into it. https://codereview.chromium.org/1412663005/diff/1/cc/trees/layer_tree_impl.cc File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/1412663005/diff/1/cc/trees/layer_tree_impl.cc... cc/trees/layer_tree_impl.cc:537: layer_tree_host_impl_->SetFullRootLayerDamage(); On 2015/10/22 23:15:29, danakj wrote: > i don't think this is needed, we'll get invalidations from blink if its scale > factor changed and it repainted things > > so you could make this simply set_painted_de..sc..factor() in the header file? Done. https://codereview.chromium.org/1412663005/diff/1/cc/trees/layer_tree_impl.h File cc/trees/layer_tree_impl.h (right): https://codereview.chromium.org/1412663005/diff/1/cc/trees/layer_tree_impl.h#... cc/trees/layer_tree_impl.h:200: void SetPaintedDeviceScaleFactor(float painted_device_scale_factor); On 2015/10/22 23:15:29, danakj wrote: > nit: normally keep the set/get pair together for each var Done. https://codereview.chromium.org/1412663005/diff/1/content/renderer/gpu/render... File content/renderer/gpu/render_widget_compositor.cc (right): https://codereview.chromium.org/1412663005/diff/1/content/renderer/gpu/render... content/renderer/gpu/render_widget_compositor.cc:625: layer_tree_host_->SetPaintedDeviceScaleFactor(device_scale); On 2015/10/22 23:15:29, danakj wrote: > I think maybe you want a separate function in RWC and blink will call one or the > other This is called from mulitple places, RenderWidgetFullscreenPepper WebFrameWidgetImpl WeebPagePopupImpl WebViewImpl and I felt this is a bit cleaner, especially the old path will be removed once migration is done. I'll wait for the owners' suggestion. https://codereview.chromium.org/1412663005/diff/1/content/renderer/render_wid... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1412663005/diff/1/content/renderer/render_wid... content/renderer/render_widget.cc:783: gfx::ScaleSize(gfx::SizeF(visible_viewport_size), device_scale_factor_); On 2015/10/22 23:15:29, danakj wrote: > It seems odd that you're scaling things here. And I'm not sure about the > WebWidget having a different size depending on the mode, but I think +enne > should have a look at this part. visible_viewport_size is in DIP so we have to multiple by dsf for this mode (otherwise, I get 1/4 visible contents) https://codereview.chromium.org/1412663005/diff/1/content/renderer/render_wid... content/renderer/render_widget.cc:2387: compositor_->setViewportSize(size_, physical_backing_size_); On 2015/10/22 23:15:29, danakj wrote: > The function that takes 2 args here should probably not exist anymore, and it > always ignores the first argument entirely. So this is all equivalent to always > just passing the physical_backing_size_ Ah, I didn't know about that. Changed to use single arg and removed two arg method. https://codereview.chromium.org/1412663005/diff/1/content/renderer/render_wid... File content/renderer/render_widget.h (right): https://codereview.chromium.org/1412663005/diff/1/content/renderer/render_wid... content/renderer/render_widget.h:575: // On 2015/10/22 23:15:29, danakj wrote: > extra lines? Done.
The CQ bit was checked by oshima@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412663005/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412663005/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/1412663005/diff/1/content/renderer/gpu/render... File content/renderer/gpu/render_widget_compositor.cc (right): https://codereview.chromium.org/1412663005/diff/1/content/renderer/gpu/render... content/renderer/gpu/render_widget_compositor.cc:625: layer_tree_host_->SetPaintedDeviceScaleFactor(device_scale); On 2015/10/23 18:05:36, oshima wrote: > On 2015/10/22 23:15:29, danakj wrote: > > I think maybe you want a separate function in RWC and blink will call one or > the > > other > This is called from mulitple places, > > RenderWidgetFullscreenPepper > WebFrameWidgetImpl > WeebPagePopupImpl > WebViewImpl > > and I felt this is a bit cleaner, especially the old path will be removed once > migration is done. I'll wait for the owners' suggestion. I kinda agree with Dana. WebViewImpl (and others) know what they are doing and should tell RWC how to render / pass metadata, instead of RWC second-guessing what they intend to do based on the flag (seems like a bit of a layering violation). https://codereview.chromium.org/1412663005/diff/1/content/renderer/render_wid... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1412663005/diff/1/content/renderer/render_wid... content/renderer/render_widget.cc:783: gfx::ScaleSize(gfx::SizeF(visible_viewport_size), device_scale_factor_); On 2015/10/23 18:05:36, oshima wrote: > On 2015/10/22 23:15:29, danakj wrote: > > It seems odd that you're scaling things here. And I'm not sure about the > > WebWidget having a different size depending on the mode, but I think +enne > > should have a look at this part. > > visible_viewport_size is in DIP so we have to multiple by dsf for this mode > (otherwise, I get 1/4 visible contents) If you look at RenderViewImpl::OnDisableAutoResize, it will call Resize with visible_viewport_size_, which means we would scale it again every time it's called. That seems wrong. It also seems confusing that the space (DIP vs pixels) of that variable depends on the flag. Shouldn't resizePinchViewport be the one doing the scaling?
https://codereview.chromium.org/1412663005/diff/1/content/renderer/gpu/render... File content/renderer/gpu/render_widget_compositor.cc (right): https://codereview.chromium.org/1412663005/diff/1/content/renderer/gpu/render... content/renderer/gpu/render_widget_compositor.cc:625: layer_tree_host_->SetPaintedDeviceScaleFactor(device_scale); On 2015/10/23 at 19:30:33, piman (slow to review) wrote: > I kinda agree with Dana. WebViewImpl (and others) know what they are doing and should tell RWC how to render / pass metadata, instead of RWC second-guessing what they intend to do based on the flag (seems like a bit of a layering violation). Agreed here as well. https://codereview.chromium.org/1412663005/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/1412663005/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:6388: EXPECT_EQ(2.0f, metadata->device_scale_factor); Maybe include a device scale factor on the compositor here too, just for completeness?
Patchset #4 (id:60001) has been deleted
https://codereview.chromium.org/1412663005/diff/1/cc/trees/layer_tree_host_un... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/1412663005/diff/1/cc/trees/layer_tree_host_un... cc/trees/layer_tree_host_unittest.cc:6410: void SwapBuffersOnThread(LayerTreeHostImpl* host_impl, bool result) override { On 2015/10/22 23:15:29, danakj wrote: > Can you just use > https://code.google.com/p/chromium/codesearch#chromium/src/cc/test/fake_outpu... > to check this instead of a swap promise? Actually I could just check the last frame here. Let me know if this is ok. https://codereview.chromium.org/1412663005/diff/1/content/renderer/gpu/render... File content/renderer/gpu/render_widget_compositor.cc (right): https://codereview.chromium.org/1412663005/diff/1/content/renderer/gpu/render... content/renderer/gpu/render_widget_compositor.cc:625: layer_tree_host_->SetPaintedDeviceScaleFactor(device_scale); On 2015/10/23 21:21:25, enne wrote: > On 2015/10/23 at 19:30:33, piman (slow to review) wrote: > > > I kinda agree with Dana. WebViewImpl (and others) know what they are doing and > should tell RWC how to render / pass metadata, instead of RWC second-guessing > what they intend to do based on the flag (seems like a bit of a layering > violation). > > Agreed here as well. How about letting WebWidgetClient set the device scale instead? I wanted Blink not to know the mode. I think the way I changed WebViewImpl was mistake. I should instead have had a method like WebViewImpl::setZoomFactorForDSF, and remove enableUseZoomXxxx (I updated this part). This way, we can restrict the mode knowledge in RenderWidget/RenderViewImpl. What do you think? https://codereview.chromium.org/1412663005/diff/20001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/1412663005/diff/20001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:6388: EXPECT_EQ(2.0f, metadata->device_scale_factor); On 2015/10/23 21:21:25, enne wrote: > Maybe include a device scale factor on the compositor here too, just for > completeness? I added additional check in SwapBuffersOnThread.
The CQ bit was checked by oshima@chromium.org to run a CQ dry run
https://codereview.chromium.org/1412663005/diff/1/content/renderer/render_wid... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1412663005/diff/1/content/renderer/render_wid... content/renderer/render_widget.cc:783: gfx::ScaleSize(gfx::SizeF(visible_viewport_size), device_scale_factor_); On 2015/10/23 19:30:33, piman (slow to review) wrote: > On 2015/10/23 18:05:36, oshima wrote: > > On 2015/10/22 23:15:29, danakj wrote: > > > It seems odd that you're scaling things here. And I'm not sure about the > > > WebWidget having a different size depending on the mode, but I think +enne > > > should have a look at this part. > > > > visible_viewport_size is in DIP so we have to multiple by dsf for this mode > > (otherwise, I get 1/4 visible contents) > > If you look at RenderViewImpl::OnDisableAutoResize, it will call Resize with > visible_viewport_size_, which means we would scale it again every time it's > called. That seems wrong. It also seems confusing that the space (DIP vs pixels) > of that variable depends on the flag. > > Shouldn't resizePinchViewport be the one doing the scaling? You're right. It takes blink's coord (WebRect). I changed to convert it only for resizePinchViewport.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412663005/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412663005/80001
https://codereview.chromium.org/1412663005/diff/80001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/1412663005/diff/80001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:6393: EXPECT_EQ(2.0f, Great, thanks. The cc parts LGTM
Oh few small things: https://codereview.chromium.org/1412663005/diff/80001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/1412663005/diff/80001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:38: #include "cc/test/fake_output_surface_client.h" not needed? https://codereview.chromium.org/1412663005/diff/80001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:6403: MULTI_THREAD_TEST_F(LayerTreeHostTestPaintedDeviceScaleFactor); Why is it MULTI only and not SINGLE_AND_MULTI?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
My main comment is that I'm unhappy with the changes to RenderWidget and would like them to all be moved into the browser. RenderWidget is not a coordinate conversion boundary today and I don't want to turn it into one. I think the appropriate place for those coordinate space changes is RenderWidgetHostViewAura or its environs. As for Android/Mac, let's not worry about supporting them. I smoke tested this patch quickly on an Android device and observed that although it basically works, Android has a lot of extra problems with top controls, tap disambiguation, overscroll glow, selection handles, WebView WRAP_CONTENTS, WebView viewport quirks, etc. Also, the fact that Android uses physical pixels in the browser will make various things different. Especially as this approach doesn't fix pinch zoom, I don't really want to spend time on that transition. For Android, I'd be more comfortable sticking with the old devicescalefactor approach indefinitely, and wait either for the true fix to this issue (display-list annotations for borders/etc), or for Android to move over entirely to Aura anyway. https://codereview.chromium.org/1412663005/diff/80001/cc/trees/layer_tree_imp... File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/1412663005/diff/80001/cc/trees/layer_tree_imp... cc/trees/layer_tree_impl.cc:61: painted_device_scale_factor_(0.f), This one should default to 1 as well. https://codereview.chromium.org/1412663005/diff/80001/content/renderer/render... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1412663005/diff/80001/content/renderer/render... content/renderer/render_widget.cc:778: size_ = new_size; size_ should match WebView::size(). https://codereview.chromium.org/1412663005/diff/80001/content/renderer/render... content/renderer/render_widget.cc:796: IsUseZoomForDSFEnabled() ? physical_backing_size_ : size_; Please do this in/near RenderWidgetHostViewAura instead. https://codereview.chromium.org/1412663005/diff/80001/content/renderer/render... content/renderer/render_widget.cc:806: gfx::ScaleSize(gfx::SizeF(visible_viewport_size), device_scale_factor_); Please set this in/near RenderWidgetHostViewAura (and please avoid the lossy back-conversion from DIPs). https://codereview.chromium.org/1412663005/diff/80001/content/renderer/render... content/renderer/render_widget.cc:1961: if (IsUseZoomForDSFEnabled()) { This is pretty ugly and looks like it will spread. It's really nonobvious from the standpoint of RenderWidget why a conversion to DIP is appropriate here. The consumer of DIP is Aura so I think the conversion should be near Aura in the browser-side.
On 2015/10/25 00:07:11, aelias wrote: > My main comment is that I'm unhappy with the changes to RenderWidget and would > like them to all be moved into the browser. RenderWidget is not a coordinate > conversion boundary today and I don't want to turn it into one. I think the > appropriate place for those coordinate space changes is RenderWidgetHostViewAura > or its environs. There is no clear boundary today. Blink viewpor <-> screen coordinate conversion happens in several places, some in blink (for popups), some in renderwidget, but conversion usually happens on renderer side, not browser side, because render widget needs to know the screen coordinates (like window_rect). Another example, RenderWidgetHostImpl::OnIMECompositionRangeChanged takes the screen coordinates from renderer. I was planning to consolidate these conversions into more narrower place, RenderWidget/ViewImpl. That's being said, I'm happy to move it to browser side if that's the consensus, but that'd require a lot more changes (above examples) because we have to remove screen coordinates from renderer. Let's discuss this in the design doc first. > As for Android/Mac, let's not worry about supporting them. I smoke tested this > patch quickly on an Android device and observed that although it basically > works, Android has a lot of extra problems with top controls, tap > disambiguation, overscroll glow, selection handles, WebView WRAP_CONTENTS, > WebView viewport quirks, etc. This test is just one of many CLs that we need, and things like inputs, scrollbar etc will be handled separately. Original plan is to dela with Android/Mac after aura is migrated. ChromeOS also supports touch, and has similar issue like selection handles, pinch-zoom. > Also, the fact that Android uses physical pixels > in the browser will make various things different. Especially as this approach > doesn't fix pinch zoom, I don't really want to spend time on that transition. > For Android, I'd be more comfortable sticking with the old devicescalefactor > approach indefinitely, and wait either for the true fix to this issue > (display-list annotations for borders/etc), or for Android to move over entirely > to Aura anyway. Sorry, I didn't fully understand, but let's discuss this in design doc too. Note that there are high DPI related bugs reported on Android as well, (e.g. crbug.com/236371, crbug.com/444517), but if we can wait for aura migration, i'm fine with that too. > > https://codereview.chromium.org/1412663005/diff/80001/cc/trees/layer_tree_imp... > File cc/trees/layer_tree_impl.cc (right): > > https://codereview.chromium.org/1412663005/diff/80001/cc/trees/layer_tree_imp... > cc/trees/layer_tree_impl.cc:61: painted_device_scale_factor_(0.f), > This one should default to 1 as well. > > https://codereview.chromium.org/1412663005/diff/80001/content/renderer/render... > File content/renderer/render_widget.cc (right): > > https://codereview.chromium.org/1412663005/diff/80001/content/renderer/render... > content/renderer/render_widget.cc:778: size_ = new_size; > size_ should match WebView::size(). > > https://codereview.chromium.org/1412663005/diff/80001/content/renderer/render... > content/renderer/render_widget.cc:796: IsUseZoomForDSFEnabled() ? > physical_backing_size_ : size_; > Please do this in/near RenderWidgetHostViewAura instead. > > https://codereview.chromium.org/1412663005/diff/80001/content/renderer/render... > content/renderer/render_widget.cc:806: > gfx::ScaleSize(gfx::SizeF(visible_viewport_size), device_scale_factor_); > Please set this in/near RenderWidgetHostViewAura (and please avoid the lossy > back-conversion from DIPs). > > https://codereview.chromium.org/1412663005/diff/80001/content/renderer/render... > content/renderer/render_widget.cc:1961: if (IsUseZoomForDSFEnabled()) { > This is pretty ugly and looks like it will spread. It's really nonobvious from > the standpoint of RenderWidget why a conversion to DIP is appropriate here. The > consumer of DIP is Aura so I think the conversion should be near Aura in the > browser-side.
As the system is designed today the css pixel to device pixel conversion needs to happen in Blink as that is where all rounding and snapping decisions are made. This is needed during layout and allows us to share more code between hi-dpi and browser zoom. Much of this is discussed in detail in the design document. While this might not be the ideal long term solution it is a step in the right direction, solves real world problems and is better than what we have today.
I'll update the doc to continue discussion on coordinate conversion. (I'm memory sheriff and need to look into P0 bug, so it'll be either tomorrow or Thursday). I put this on hold for now, and will address other issues once we reach the consensus. https://codereview.chromium.org/1412663005/diff/80001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/1412663005/diff/80001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:38: #include "cc/test/fake_output_surface_client.h" On 2015/10/24 00:08:07, danakj wrote: > not needed? Done. https://codereview.chromium.org/1412663005/diff/80001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:6403: MULTI_THREAD_TEST_F(LayerTreeHostTestPaintedDeviceScaleFactor); On 2015/10/24 00:08:07, danakj wrote: > Why is it MULTI only and not SINGLE_AND_MULTI? Done. https://codereview.chromium.org/1412663005/diff/80001/cc/trees/layer_tree_imp... File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/1412663005/diff/80001/cc/trees/layer_tree_imp... cc/trees/layer_tree_impl.cc:61: painted_device_scale_factor_(0.f), On 2015/10/25 00:07:11, aelias wrote: > This one should default to 1 as well. Done. https://codereview.chromium.org/1412663005/diff/80001/content/renderer/render... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1412663005/diff/80001/content/renderer/render... content/renderer/render_widget.cc:1961: if (IsUseZoomForDSFEnabled()) { On 2015/10/25 00:07:11, aelias wrote: > This is pretty ugly and looks like it will spread. It's really nonobvious from > the standpoint of RenderWidget why a conversion to DIP is appropriate here. The > consumer of DIP is Aura so I think the conversion should be near Aura in the > browser-side. Both Aura/Mac uses DIP at least. Android isn't using DIP here? For the code itself, I was thinking of removing the implicit conversion between WebRect and gfx:: and provide explicit functions to convert between blink <-> screen coordinates. That will make it clear in which coordinates rects/size/points are in, and eliminate if block.
https://codereview.chromium.org/1412663005/diff/80001/content/renderer/render... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/1412663005/diff/80001/content/renderer/render... content/renderer/render_widget.cc:1961: if (IsUseZoomForDSFEnabled()) { On 2015/10/27 20:28:09, oshima wrote: > On 2015/10/25 00:07:11, aelias wrote: > > This is pretty ugly and looks like it will spread. It's really nonobvious > from > > the standpoint of RenderWidget why a conversion to DIP is appropriate here. > The > > consumer of DIP is Aura so I think the conversion should be near Aura in the > > browser-side. > > Both Aura/Mac uses DIP at least. Android isn't using DIP here? > > For the code itself, I was thinking of removing the implicit conversion between > WebRect and gfx:: and provide explicit functions to convert between blink <-> > screen coordinates. That will make it clear in which coordinates > rects/size/points are in, and eliminate if block. I'm not sure why Aura is coming up here, but I probably misunderstand. The renderer (blink) is DIP on every platform today, and we'd like to do layout/painting in physical pixels. How come Aura is related to what we're doing in RenderWidget? The browser (UI) is DIP on Aura and physical on Android. Android is moving toward using Aura.. but Aura is moving toward using physical pixels for paint/layout also (slower than blink is).
OK, lgtm to land this as is, based on the point that the coordinate conversion boundary is already messy and this only makes it somewhat worse. On 2015/10/27 at 20:33:47, danakj wrote: > I'm not sure why Aura is coming up here, but I probably misunderstand. The renderer (blink) is DIP on every platform today, and we'd like to do layout/painting in physical pixels. > > How come Aura is related to what we're doing in RenderWidget? > > The browser (UI) is DIP on Aura and physical on Android. Android is moving toward using Aura.. but Aura is moving toward using physical pixels for paint/layout also (slower than blink is). There are three reasons: 1) Aura is using DIP, Android is using physical pixels in the browser (I didn't remember Aura was planning to transition, thanks for pointing that out) 2) Android cares a lot more about pinch zoom and this approach leaves the exact same bugs existing in the presence of pinch zoom, so it's significantly less valuable there. 3) Android has more UI elements that need to be coordinate-space adjusted manually (the ones I listed above). The extra code oshima@ is adding to RenderWidget is to convert things like popups to DIP for use by Aura, so it would require different logic for Android. It sounds like this new code will need to be deleted when Aura transitions to physical pixels as well, so fortunately this divergence won't be permanent.
On 2015/10/28 02:00:55, aelias wrote: > OK, lgtm to land this as is, based on the point that the coordinate conversion > boundary is already messy and this only makes it somewhat worse. > > On 2015/10/27 at 20:33:47, danakj wrote: > > I'm not sure why Aura is coming up here, but I probably misunderstand. The > renderer (blink) is DIP on every platform today, and we'd like to do > layout/painting in physical pixels. > > > > How come Aura is related to what we're doing in RenderWidget? > > > > The browser (UI) is DIP on Aura and physical on Android. Android is moving > toward using Aura.. but Aura is moving toward using physical pixels for > paint/layout also (slower than blink is). I'll try to consolidate conversion point so that the rest of the code do not have to worry about much. > > There are three reasons: > 1) Aura is using DIP, Android is using physical pixels in the browser (I didn't > remember Aura was planning to transition, thanks for pointing that out) > 2) Android cares a lot more about pinch zoom and this approach leaves the exact > same bugs existing in the presence of pinch zoom, so it's significantly less > valuable there. > 3) Android has more UI elements that need to be coordinate-space adjusted > manually (the ones I listed above). > > The extra code oshima@ is adding to RenderWidget is to convert things like > popups to DIP for use by Aura, so it would require different logic for Android. > It sounds like this new code will need to be deleted when Aura transitions to > physical pixels as well, so fortunately this divergence won't be permanent. My understanding is that aura's painting code will be physical pixel but coordinate itself remains DIP (at least it needs to be DIP for ChromeOS and Mac. Maybe Windows will move to physical pixel).
The CQ bit was checked by oshima@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412663005/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412663005/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Patchset #7 (id:140001) has been deleted
The CQ bit was checked by oshima@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412663005/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412663005/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by oshima@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412663005/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412663005/180001
oshima@chromium.org changed reviewers: + rbyers@chromium.org
piman@, enne@, please take another look. rbyers@ -> WebKit/public owner https://codereview.chromium.org/1412663005/diff/1/content/renderer/gpu/render... File content/renderer/gpu/render_widget_compositor.cc (right): https://codereview.chromium.org/1412663005/diff/1/content/renderer/gpu/render... content/renderer/gpu/render_widget_compositor.cc:625: layer_tree_host_->SetPaintedDeviceScaleFactor(device_scale); On 2015/10/23 22:05:02, oshima wrote: > On 2015/10/23 21:21:25, enne wrote: > > On 2015/10/23 at 19:30:33, piman (slow to review) wrote: > > > > > I kinda agree with Dana. WebViewImpl (and others) know what they are doing > and > > should tell RWC how to render / pass metadata, instead of RWC second-guessing > > what they intend to do based on the flag (seems like a bit of a layering > > violation). > > > > Agreed here as well. > > How about letting WebWidgetClient set the device scale instead? I wanted Blink > not to know the mode. I think the way I changed WebViewImpl was mistake. I > should instead have had a method like WebViewImpl::setZoomFactorForDSF, and > remove enableUseZoomXxxx (I updated this part). This way, we can restrict the > mode knowledge in RenderWidget/RenderViewImpl. > > What do you think? This change wasn't actually necessary. I changed the following * Add SetPaintedDeviceScaleFactor on RWC * Let RenderViewImpl call RWC::SetPaintedDeviceScaleFactor. This is enough because blink's dsf won't change. I also removed the WebWidgetClient::deviceScaleFactor as it isn't necessary.
https://codereview.chromium.org/1412663005/diff/180001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebView.h (right): https://codereview.chromium.org/1412663005/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/web/WebView.h:296: // Sets the additional zoom factor used for device scale factor. Can you please elaborate in this (and/or above) comments? At the moment I'm confused by the distinction of the three values (though I admit it's been awhile since I read your design doc). The plan is to remove deviceScaleFactor from blink (so this API will replace the above one), right? Adding a comment to clarify that we're in transition should help.
https://codereview.chromium.org/1412663005/diff/180001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebView.h (right): https://codereview.chromium.org/1412663005/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/web/WebView.h:296: // Sets the additional zoom factor used for device scale factor. Can you please elaborate in this (and/or above) comments? At the moment I'm confused by the distinction of the three values (though I admit it's been awhile since I read your design doc). The plan is to remove deviceScaleFactor from blink (so this API will replace the above one), right? Adding a comment to clarify that we're in transition should help.
https://codereview.chromium.org/1412663005/diff/180001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebView.h (right): https://codereview.chromium.org/1412663005/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/web/WebView.h:296: // Sets the additional zoom factor used for device scale factor. On 2015/10/30 17:50:10, Rick Byers wrote: > Can you please elaborate in this (and/or above) comments? At the moment I'm > confused by the distinction of the three values (though I admit it's been awhile > since I read your design doc). So the idea is to use zoom *mechanism* to implement DSF behavior, but we still need to keep zooming semantics as a browser feature (one you can set 120% 150% using ctrl/+-). Therefore, we have two inputs: zoom level DSF and the actual zoom factor applied inside Blink is: DSF * zoom factor (computed from zoom level) > The plan is to remove deviceScaleFactor from blink (so this API will replace the > above one), right? Adding a comment to clarify that we're in transition should > help. I updated the comment.
https://codereview.chromium.org/1412663005/diff/180001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebView.h (right): https://codereview.chromium.org/1412663005/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/web/WebView.h:296: // Sets the additional zoom factor used for device scale factor. On 2015/10/30 18:14:18, oshima wrote: > On 2015/10/30 17:50:10, Rick Byers wrote: > > Can you please elaborate in this (and/or above) comments? At the moment I'm > > confused by the distinction of the three values (though I admit it's been > awhile > > since I read your design doc). > > So the idea is to use zoom *mechanism* to implement DSF behavior, but we still > need to keep > zooming semantics as a browser feature (one you can set 120% 150% using > ctrl/+-). > > Therefore, we have two inputs: > > zoom level > DSF > > and the actual zoom factor applied inside Blink is: > > DSF * zoom factor (computed from zoom level) > > > The plan is to remove deviceScaleFactor from blink (so this API will replace > the > > above one), right? Adding a comment to clarify that we're in transition > should > > help. > > I updated the comment. Makes sense, thanks! WebKit/public/ LGTM
https://codereview.chromium.org/1412663005/diff/200001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/1412663005/diff/200001/content/renderer/rende... content/renderer/render_view_impl.cc:3708: compositor_->SetPaintedDeviceScaleFactor(device_scale_factor_); side note: this is probably good enough for now, but I suspect later you'll want to do this one at the RenderWidget level, for popups etc. consistency. https://codereview.chromium.org/1412663005/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1412663005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:3112: m_zoomFactorForDeviceScaleFactor = zoomFactorForDeviceScaleFactor; ISTM you want to save the zoomFactorForDSF regardless of whether the layerTreeView is initialized yet or not.
https://codereview.chromium.org/1412663005/diff/200001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/1412663005/diff/200001/content/renderer/rende... content/renderer/render_view_impl.cc:3708: compositor_->SetPaintedDeviceScaleFactor(device_scale_factor_); On 2015/10/30 21:32:02, piman (slow to review) wrote: > side note: this is probably good enough for now, but I suspect later you'll want > to do this one at the RenderWidget level, for popups etc. consistency. Acknowledged. I removed the dcheck in RWC becasue it's indeed called for popup. I'll deal with it in a separate CL. https://codereview.chromium.org/1412663005/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1412663005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:3112: m_zoomFactorForDeviceScaleFactor = zoomFactorForDeviceScaleFactor; On 2015/10/30 21:32:02, piman (slow to review) wrote: > ISTM you want to save the zoomFactorForDSF regardless of whether the > layerTreeView is initialized yet or not. Done. I also remove the equality check so that it updates zoom level if the call didn't update zoom level.
lgtm
The CQ bit was checked by oshima@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, aelias@chromium.org, rbyers@chromium.org Link to the patchset: https://codereview.chromium.org/1412663005/#ps220001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412663005/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412663005/220001
Message was sent while issue was closed.
Committed patchset #10 (id:220001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/750cb4349069151c8ad1f446f92ecf66a2f9023f Cr-Commit-Position: refs/heads/master@{#357245} |
