Remove Layer/LayerImpl's scroll_clip_layer and instead track scrolling bounds [chromium/src : master]
Philip Rogers (Gerrit)
Philip Rogers would like enne and Chris harrelson to review this change.
Remove Layer/LayerImpl's scroll_clip_layer and instead track scrolling bounds
This patch removes scroll_clip_layer from Layer and LayerImpl which removes
a major layer dependency for scrolling (e.g., with this patch, ScrollNode's
owning_layer_id can be removed). Instead of tracking the scroll container
bounds with a Layer, this patch tracks it explicitly as Layer/LayerImpl's
scroll_container_bounds. If clipping is desired, a clip layer will still
need to be added and manually kept up-to-date with the scrolling layer's
scroll container bounds.
Because the scroll container bounds are now tracked on the scrolling layer,
scrollability (i.e., Layer::scrollable()) is based on the scroll container
bounds being non-empty, and the scrollable bit has been removed.
A TODO has been added to rename ScrollNode's scroll_clip_bounds to be
scroll_container_bounds. The scroll_clip concept was particularily
confusing for WebView which has a scrolling concept that does not clip.
Bug: 723263
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: Idd994862b01b3fa9d9c50648fb9ecf816886c558
---
M cc/blink/web_layer_impl.cc
M cc/blink/web_layer_impl.h
M cc/input/scrollbar_animation_controller_unittest.cc
M cc/input/single_scrollbar_animation_controller_thinning_unittest.cc
M cc/layers/layer.cc
M cc/layers/layer.h
M cc/layers/layer_impl.cc
M cc/layers/layer_impl.h
M cc/layers/layer_impl_unittest.cc
M cc/layers/layer_perftest.cc
M cc/layers/layer_position_constraint_unittest.cc
M cc/layers/layer_unittest.cc
M cc/layers/painted_scrollbar_layer_impl_unittest.cc
M cc/layers/scrollbar_layer_unittest.cc
M cc/layers/solid_color_scrollbar_layer_impl_unittest.cc
M cc/test/layer_tree_json_parser.cc
M cc/test/layer_tree_test.cc
M cc/test/test_layer_tree_host_base.cc
M cc/trees/draw_property_utils.cc
M cc/trees/layer_tree_host_common_unittest.cc
M cc/trees/layer_tree_host_impl.cc
M cc/trees/layer_tree_host_impl.h
M cc/trees/layer_tree_host_impl_unittest.cc
M cc/trees/layer_tree_host_unittest.cc
M cc/trees/layer_tree_host_unittest_animation.cc
M cc/trees/layer_tree_host_unittest_damage.cc
M cc/trees/layer_tree_host_unittest_picture.cc
M cc/trees/layer_tree_host_unittest_scroll.cc
M cc/trees/property_tree_builder.cc
M cc/trees/scroll_node.h
M cc/trees/tree_synchronizer_unittest.cc
M third_party/WebKit/Source/core/frame/LocalFrameView.cpp
M third_party/WebKit/Source/core/frame/VisualViewport.cpp
M third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp
M third_party/WebKit/Source/platform/graphics/GraphicsLayerTest.cpp
M third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp
M third_party/WebKit/public/platform/WebLayer.h
M ui/compositor/layer.cc
M ui/compositor/layer.h
M ui/views/controls/scroll_view.cc
40 files changed, 291 insertions(+), 379 deletions(-)
To view, visit change 545089. To unsubscribe, visit settings.
Philip Rogers (Gerrit)
Philip Rogers posted comments on this change.
Patch set 3:Commit-Queue +1
enne (Gerrit)
enne posted comments on this change.
Patch set 3:
(2 comments)
File third_party/WebKit/Source/core/frame/LocalFrameView.cpp:
Patch Set #3, Line 1640:
if (layout_view->UsesCompositing()) {Why does this block move?
File ui/views/controls/scroll_view.cc:
Patch Set #3, Line 242:
a_view->layer()->SetScrollContainerBounds(Can the contents viewport change size? How does this get updated, if so?
To view, visit change 545089. To unsubscribe, visit settings.
Chris harrelson (Gerrit)
Chris harrelson posted comments on this change.
Patch set 3:
(3 comments)
File third_party/WebKit/Source/core/frame/LocalFrameView.cpp:
I think we can avoid this change if you re-introduce the scrollable bit as
a private variable in cc::Layer, which will allow you to distiguish between
scrollability and a scrolling container bound that happens to be empty.
File third_party/WebKit/public/platform/WebLayer.h:
Patch Set #3, Line 174:
virtual void SetScrollContainerBounds(const WebSize&) = 0;How about:
SetScrollable(const WebSize& scroll_container_bounds)? If changed, a bunch of names
in various files in this patch would change also.
File ui/views/controls/scroll_view.cc:
Patch Set #3, Line 478:
contents_->layer()->SetScrollContainerBounds(viewport_bounds.size());Why is this new line needed?
To view, visit change 545089. To unsubscribe, visit settings.
Philip Rogers (Gerrit)
Philip Rogers posted comments on this change.
Patch set 4:Commit-Queue +1
(5 comments)
I think we can avoid this change if you re-introduce the scrollable bit as
Done
Patch Set #3, Line 1640:
// If this is the main frame, we might have got here by hiding/showing the
Why does this block move?
This was originally because we needed to set the size before updating scroll offset in case the size changed to/from zero which affected scrollability. With Chris's suggestion to add a scrollable bit, this change is no longer needed.
File third_party/WebKit/public/platform/WebLayer.h:
Patch Set #3, Line 174:
virtual void SetScrollable(const WebSize& scroll_container_bounds) = 0;How about:
I like how this turned out. Done.
File ui/views/controls/scroll_view.cc:
Patch Set #3, Line 242:
a_view->layer()->SetScrollable(contents_viewport_->bounds().size());
Can the contents viewport change size? How does this get updated, if so?
Yes, this happens in ScrollView::Layout when we call "contents_viewport_->SetBoundsRect". In the same function we now update the contents_->layer's scroll container bounds.
Why is this new line needed?
This is to handle the case when the viewport layer's bounds change.
To view, visit change 545089. To unsubscribe, visit settings.
Philip Rogers (Gerrit)
Philip Rogers posted comments on this change.
Patch set 5:Commit-Queue +1
Philip Rogers (Gerrit)
Philip Rogers uploaded patch set #6 to this change.
Remove Layer/LayerImpl's scroll_clip_layer and instead track scrolling bounds
This patch removes scroll_clip_layer from Layer and LayerImpl which removes
a major layer dependency for scrolling (e.g., with this patch, ScrollNode's
owning_layer_id can be removed). Instead of tracking the scroll container
bounds with a Layer, this patch tracks it explicitly as Layer/LayerImpl's
scroll_container_bounds. If clipping is desired, a clip layer will still
need to be added and manually kept up-to-date with the scrolling layer's
scroll container bounds.
M third_party/WebKit/Source/core/frame/VisualViewport.cpp
M third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp
M third_party/WebKit/Source/platform/graphics/GraphicsLayerTest.cpp
M third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp
M third_party/WebKit/Source/web/tests/data/mhtml/simple_test.mht
M third_party/WebKit/public/platform/WebLayer.h
M ui/compositor/layer.cc
M ui/compositor/layer.h
M ui/views/controls/scroll_view.cc
40 files changed, 308 insertions(+), 391 deletions(-)
To view, visit change 545089. To unsubscribe, visit settings.
Philip Rogers (Gerrit)
Philip Rogers posted comments on this change.
Patch set 6:Commit-Queue +1
Chris harrelson (Gerrit)
Chris harrelson posted comments on this change.
Patch set 6:
(19 comments)
File cc/input/scrollbar_animation_controller_unittest.cc:
Patch Set #6, Line 1364:
clip_layer_->SetBounds(gfx::Size(100, 100));Can this layer be removed from the test?
File cc/layers/layer_impl_unittest.cc:
Patch Set #6, Line 133:
std::unique_ptr<LayerImpl> root_clip_ptr =Is this layer necessary for the test?
Patch Set #6, Line 224:
std::unique_ptr<LayerImpl> root_clip_ptr =ditto
File cc/layers/scrollbar_layer_unittest.cc:
Patch Set #6, Line 360:
scoped_refptr<Layer> root_clip_layer = Layer::Create();ditto
File cc/trees/layer_tree_host_common_unittest.cc:
Patch Set #6, Line 545:
clip_layer->SetBounds(ditto
File cc/trees/layer_tree_host_impl_unittest.cc:
Patch Set #6, Line 342:
gfx::Size viewport_scroll_bounds =ditto
Patch Set #6, Line 355:
std::unique_ptr<LayerImpl> outer_clip =ditto
Patch Set #6, Line 418:
std::unique_ptr<LayerImpl> clip = LayerImpl::Create(layer_tree_impl, 2);ditto
Patch Set #6, Line 524:
LayerImpl* clip_layer) {remove third argument?
Patch Set #6, Line 818:
auto* root_clip = root_clip_owned.get();ditto
Patch Set #6, Line 865:
std::unique_ptr<LayerImpl> child_clip =ditto
Patch Set #6, Line 1204:
std::unique_ptr<LayerImpl> clip = LayerImpl::Create(layer_tree_impl, 2);ditto
Patch Set #6, Line 3317:
std::unique_ptr<LayerImpl> child_clip =ditto
Patch Set #6, Line 6197:
std::unique_ptr<LayerImpl> clip_layer =ditto
Patch Set #6, Line 7011:
std::unique_ptr<LayerImpl> clip = LayerImpl::Create(layer_tree_impl, 10);ditto
Patch Set #6, Line 10035:
std::unique_ptr<LayerImpl> inner_clip =ditto
File cc/trees/layer_tree_host_unittest_damage.cc:
Patch Set #6, Line 342:
scoped_refptr<Layer> scroll_clip_layer = Layer::Create();ditto
File cc/trees/layer_tree_host_unittest_picture.cc:
Patch Set #6, Line 408:
scoped_refptr<Layer> root_clip = Layer::Create();ditto
File cc/trees/tree_synchronizer_unittest.cc:
Patch Set #6, Line 500:
scoped_refptr<Layer> transient_scroll_layer = Layer::Create();ditto
To view, visit change 545089. To unsubscribe, visit settings.
Philip Rogers (Gerrit)
Philip Rogers posted comments on this change.
Patch set 6:Commit-Queue +1
Philip Rogers (Gerrit)
Philip Rogers posted comments on this change.
Patch set 7:Commit-Queue +1
Woohoo! This patch is now -230 LOC.
(19 comments)
File cc/input/scrollbar_animation_controller_unittest.cc:
Patch Set #6, Line 1364:
scroll_layer->SetScrollable(gfx::Size(100, 100));
Can this layer be removed from the test?
Done
File cc/layers/layer_impl_unittest.cc:
Patch Set #6, Line 133:
std::unique_ptr<LayerImpl> root_ptr =
Is this layer necessary for the test?
Done
Patch Set #6, Line 224:
host_impl.active_tree()->SetRootLayerForTesting(std::move(root_ptr));ditto
Done
File cc/layers/scrollbar_layer_unittest.cc:
Patch Set #6, Line 360:
scoped_refptr<Layer> content_layer = Layer::Create();ditto
Done
File cc/trees/layer_tree_host_common_unittest.cc:
Patch Set #6, Line 545:
SetScrollOffsetDelta(scroll_layer, kScrollDelta);ditto
Done
File cc/trees/layer_tree_host_impl_unittest.cc:
Patch Set #6, Line 342:
gfx::Size viewport_scroll_bounds =ditto
This one is a viewport clip layer which is needed.
Patch Set #6, Line 355:
std::unique_ptr<LayerImpl> outer_clip =ditto
This one is a viewport clip layer which is needed.
Patch Set #6, Line 418:
std::unique_ptr<LayerImpl> scroll = LayerImpl::Create(layer_tree_impl, 3);ditto
Done
Patch Set #6, Line 524:
gfx::Size scroll_container_bounds =remove third argument?
Done and removed some of the clip_layers that were passed in. Most are used as viewport layers though.
Patch Set #6, Line 818:
host_impl_->active_tree()->SetRootLayerForTesting(std::move(root_owned));ditto
Done
Patch Set #6, Line 865:
InputHandler::ScrollStatus status = host_impl_->ScrollBegin(ditto
Done
ditto
Done
Patch Set #6, Line 3317:
child_scroll_element_id));ditto
Done
ditto
This one is needed. It's sort of setting up a real transformed scrollable area.
ditto
Done
ditto
This is a special viewport layer and is needed.
File cc/trees/layer_tree_host_unittest_damage.cc:
Patch Set #6, Line 342:
content_layer_ = FakePictureLayer::Create(&client_);ditto
Done
File cc/trees/layer_tree_host_unittest_picture.cc:
Patch Set #6, Line 408:
scoped_refptr<Layer> root_clip = Layer::Create();ditto
This one is a special viewport layer and is needed.
File cc/trees/tree_synchronizer_unittest.cc:
Patch Set #6, Line 500:
layer_tree_root->AddChild(transient_scroll_layer);ditto
Done
To view, visit change 545089. To unsubscribe, visit settings.
Chris harrelson (Gerrit)
Chris harrelson posted comments on this change.
Patch set 7:Code-Review +1
Philip Rogers (Gerrit)
Philip Rogers posted comments on this change.
Patch set 8:Commit-Queue +1
Philip Rogers (Gerrit)
Philip Rogers (Gerrit)
Philip Rogers (Gerrit)
Philip Rogers posted comments on this change.
Patch set 9:Commit-Queue +1
Philip Rogers (Gerrit)
Philip Rogers (Gerrit)
@enne, this is ready for a final review.
@chrishtr, I made one big change after your review to remove the has_scroll_node member on Layer.
enne (Gerrit)
enne posted comments on this change.
Patch set 9:Code-Review +1
(1 comment)
Patch Set #9, Line 223:
void SetScrollable(const gfx::Size& scroll_container_bounds);Can you leave a comment about how to make a layer not scrollable (or mention that it's not possible to undo this).
To view, visit change 545089. To unsubscribe, visit settings.
Chris harrelson (Gerrit)
Chris harrelson posted comments on this change.
Patch set 9:
Still LGTM.
Philip Rogers (Gerrit)
Philip Rogers posted comments on this change.
Patch set 10:Commit-Queue +2
wooohoo!
Commit Bot (Gerrit)
Commit Bot posted comments on this change.
Patch set 10:
CQ is trying da patch.
Note: The patchset sent to CQ was uploaded after this CL was approved.
"Address reviewer comments" https://chromium-review.googlesource.com/c/545089/10
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/545089/10
Bot data: {"action": "start", "triggered_at": "2017-06-26T18:19:58.0Z", "cq_cfg_revision": "e12d437dc7f395d72995b548c9dacf21b0b1526e", "revision": "556b9503d595c8f255c2d18d4a57301ac13d9554"}
Commit Bot (Gerrit)
Try jobs failed on following builders:
linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/415431)
Bot data: {"action": "cancel", "triggered_at": "2017-06-26T18:19:58.0Z", "cq_cfg_revision": "e12d437dc7f395d72995b548c9dacf21b0b1526e", "revision": "556b9503d595c8f255c2d18d4a57301ac13d9554"}
Philip Rogers (Gerrit)
Philip Rogers posted comments on this change.
Patch set 10:Commit-Queue +1
+danakj for ui/compositor
+msw for ui/views
Philip Rogers (Gerrit)
danakj (Gerrit)
danakj posted comments on this change.
Patch set 10:
Patch Set 10: Commit-Queue+1
+danakj for ui/compositor
+msw for ui/views
I just saw this by checking for missed CLs, a few gerrit threads are not getting marked important even tho I'm asked to review them. I will fix gmail.
danakj (Gerrit)
danakj posted comments on this change.
Patch set 10:Code-Review +1
ui/compositor LGTM w/ 1 nit
(1 comment)
Patch Set #10, Line 876:
const base::Callback<void(const gfx::ScrollOffset&)>& on_scroll) {Could you take this by value and move the callback to cc. And do the same on cc::Layer if you didn't to store the callback?
Philip Rogers (Gerrit)
Philip Rogers posted comments on this change.
Patch set 12:Commit-Queue +1
(1 comment)
Patch Set #10, Line 876:
base::Callback<void(const gfx::ScrollOffset&)> callback) {
Could you take this by value and move the callback to cc. And do the same o
Done
To view, visit change 545089. To unsubscribe, visit settings.
Philip Rogers (Gerrit)
Philip Rogers posted comments on this change.
Patch set 13:Commit-Queue +1
Philip Rogers (Gerrit)
-msw (honeymoon)
+sky
Scott, can you review ui/views/controls/scroll_view.cc?
The big idea in this change is that we now track the scrolling container bounds independently of the scroll clip layer. For ScrollView, the scrolling container bounds are no longer automatically updated when the viewport changes size so a call to SetScrollable(viewport bounds) is needed in ScrollView::Layout().
Philip Rogers (Gerrit)
Philip Rogers removed Michael Wasserman from this change.
To view, visit change 545089. To unsubscribe, visit settings.
Scott Violet (Gerrit)
Scott Violet posted comments on this change.
Patch set 13:
(3 comments)
Patch Set #13, Line 371:
et_did_scroll_callbaunix_hacker_style is used for inline cheap functions. The implementation is going to copy a callback, which I don't think is cheap. Please use SetDidScrollCallback().
Name the argument.
File ui/views/controls/scroll_view.cc:
Patch Set #13, Line 242:
Scrollable(conteWould SetScrollableSize be a better name here?
To view, visit change 545089. To unsubscribe, visit settings.
Philip Rogers (Gerrit)
Philip Rogers posted comments on this change.
Patch set 14:Commit-Queue +1
(3 comments)
Patch Set #13, Line 371:
et_did_scroll_callbaunix_hacker_style is used for inline cheap functions. The implementation is
This ends up not being a copy since we std::move it in. I'm still learning the conventions around naming these. In a case where this calls cc::Layer::set_did_scroll_callback which just does a std::move, would we still want to name this SetDidScrollCallback? If so, should I rename all of the functions from set_did_scroll_callback to SetDidScrollCallback?
Name the argument.
Done
File ui/views/controls/scroll_view.cc:
Patch Set #13, Line 242:
Scrollable(conteWould SetScrollableSize be a better name here?
This is tough because there are really two scrolling sizes: the scroll container size and the scrollable layer's size. This was originally called SetScrollContainerBounds in cc::Layer but we ended up switching to SetScrollable(container bounds) (with comments) in this review. Do you think it makes sense to leave this as-is to match the cc::Layer name?
To view, visit change 545089. To unsubscribe, visit settings.
danakj (Gerrit)
danakj posted comments on this change.
Patch set 14:
(1 comment)
Patch Set #13, Line 371:
et_did_scroll_callba
This ends up not being a copy since we std::move it in. I'm still learning
If it's not inline I would not hacker_case it.
Philip Rogers (Gerrit)
Philip Rogers posted comments on this change.
Patch set 15:Commit-Queue +1
Philip Rogers (Gerrit)
Patch Set 14:
(1 comment)
That makes sense, done.
Scott Violet (Gerrit)
Philip Rogers (Gerrit)
Philip Rogers posted comments on this change.
Patch set 15:Commit-Queue +2
This was a large patch and not the easiest to review. Thanks Scott, Dana, Chris, and Enne!
Commit Bot (Gerrit)
Commit Bot merged this change.
Remove Layer/LayerImpl's scroll_clip_layer and instead track scrolling bounds
This patch removes scroll_clip_layer from Layer and LayerImpl which removes
a major layer dependency for scrolling (e.g., with this patch, ScrollNode's
owning_layer_id can be removed). Instead of tracking the scroll container
bounds with a Layer, this patch tracks it explicitly as Layer/LayerImpl's
scroll_container_bounds. If clipping is desired, a clip layer will still
need to be added and manually kept up-to-date with the scrolling layer's
scroll container bounds.
A TODO has been added to rename ScrollNode's scroll_clip_bounds to be
scroll_container_bounds. The scroll_clip concept was particularily
confusing for WebView which has a scrolling concept that does not clip.
Bug: 723263
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: Idd994862b01b3fa9d9c50648fb9ecf816886c558
Reviewed-on: https://chromium-review.googlesource.com/545089
Reviewed-by: Scott Violet <s...@chromium.org>
Reviewed-by: danakj <dan...@chromium.org>
Reviewed-by: enne <en...@chromium.org>
Reviewed-by: Chris Harrelson <chri...@chromium.org>
Commit-Queue: Philip Rogers <p...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#482827}
---
M cc/blink/web_layer_impl.cc
M cc/blink/web_layer_impl.h
M cc/input/scrollbar_animation_controller_unittest.cc
M cc/input/single_scrollbar_animation_controller_thinning_unittest.cc
M cc/layers/layer.cc
M cc/layers/layer.h
M cc/layers/layer_impl.cc
M cc/layers/layer_impl.h
M cc/layers/layer_impl_unittest.cc
M cc/layers/layer_perftest.cc
M cc/layers/layer_position_constraint_unittest.cc
M cc/layers/layer_unittest.cc
M cc/layers/painted_scrollbar_layer_impl_unittest.cc
M cc/layers/scrollbar_layer_unittest.cc
M cc/layers/solid_color_scrollbar_layer_impl_unittest.cc
M cc/test/layer_tree_json_parser.cc
M cc/test/layer_tree_test.cc
M cc/test/test_layer_tree_host_base.cc
M cc/trees/draw_property_utils.cc
M cc/trees/layer_tree_host_common_unittest.cc
M cc/trees/layer_tree_host_impl.cc
M cc/trees/layer_tree_host_impl.h
M cc/trees/layer_tree_host_impl_unittest.cc
M cc/trees/layer_tree_host_unittest.cc
M cc/trees/layer_tree_host_unittest_animation.cc
M cc/trees/layer_tree_host_unittest_damage.cc
M cc/trees/layer_tree_host_unittest_picture.cc
M cc/trees/layer_tree_host_unittest_scroll.cc
M cc/trees/property_tree_builder.cc
M cc/trees/scroll_node.h
M cc/trees/tree_synchronizer_unittest.cc
M third_party/WebKit/Source/core/frame/VisualViewport.cpp
M third_party/WebKit/Source/core/page/scrolling/ScrollingCoordinator.cpp
M third_party/WebKit/Source/platform/graphics/GraphicsLayerTest.cpp
M third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp
M third_party/WebKit/public/platform/WebLayer.h
M ui/compositor/layer.cc
M ui/compositor/layer.h
M ui/views/controls/scroll_view.cc
39 files changed, 407 insertions(+), 642 deletions(-)