Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2364103003:
Switch RootLayerScrolls to use root transform and scroll properties (Closed)
|
Created:
4 years, 2 months ago by pdr. Modified:
4 years, 2 months ago CC:
ajuma+watch_chromium.org, blink-reviews, blink-reviews-paint_chromium.org, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj+watch_chromium.org, dshwang, drott+blinkwatch_chromium.org, krit, f(malita), jbroman, Justin Novosad, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney, slimming-paint-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSwitch RootLayerScrolls to use root transform and scroll properties
This is a followup to [1] to start using the root transform and scroll
properties with root layer scrolling enabled. With this patch, the
RLS/non-RLS property trees are no longer different.
A real codechange in this patch is to not create a scroll translation
for the FrameView unless it actually scrolls. This matches the logic
used for adding LayoutView scroll translation properties.
[1] https://crrev.com/a518f5340802393f8bdaa6b3d376dc5482049351
BUG=645615
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Committed: https://crrev.com/0abd71cfec612222822a4e4a1bb0bc89e937a9ce
Cr-Commit-Position: refs/heads/master@{#421085}
Patch Set 1 #
Total comments: 6
Depends on Patchset: Messages
Total messages: 23 (12 generated)
Description was changed from ========== Switch RootLayerScrolls to use root transform and scroll properties This is a followup to [1] to start using the root transform and scroll properties with root layer scrolling enabled. With this patch, the RLS/non-RLS property trees are no longer different. A real codechange in this patch is to not create a scroll translation for the FrameView unless it actually scrolls. This matches the logic used for adding LayoutView scroll translation properties. [1] https://crrev.com/a518f5340802393f8bdaa6b3d376dc5482049351 BUG=645615 ========== to ========== Switch RootLayerScrolls to use root transform and scroll properties This is a followup to [1] to start using the root transform and scroll properties with root layer scrolling enabled. With this patch, the RLS/non-RLS property trees are no longer different. A real codechange in this patch is to not create a scroll translation for the FrameView unless it actually scrolls. This matches the logic used for adding LayoutView scroll translation properties. [1] https://crrev.com/a518f5340802393f8bdaa6b3d376dc5482049351 BUG=645615 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by pdr@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.
pdr@chromium.org changed reviewers: + chrishtr@chromium.org, szager@chromium.org, trchen@chromium.org
https://codereview.chromium.org/2364103003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/2364103003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:482: // Fixed position transform and scroll nodes should not be affected. Why? Isn't a LayoutView a container for fixed position?
https://codereview.chromium.org/2364103003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/2364103003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:482: // Fixed position transform and scroll nodes should not be affected. On 2016/09/26 at 18:02:29, chrishtr wrote: > Why? Isn't a LayoutView a container for fixed position? It is, but only specific property tree nodes should be used by fixed position children. The initial scroll/transform tree nodes are setup for the layoutview (or FrameView, if !RLS) in PaintPropertyTreeBuilder::buildTreeNodes(FrameView& frameView, ...). In that code we set the correct fixed position transform and scroll nodes. In the subsequent calls in PaintPropertyTreeBuilder (updateTransform, updateEffect, updateCssClip, etc...), we can append to the transform/scroll trees but we don't want to use those nodes for the fixed pos children, which explains the code here. For example, for the scroll tree, we don't want the fixed position element to be scrolled by the layoutview, so we need to make sure the scroll node introduced by the layoutview is not used. Here's an example showing that effect nodes should be used, unlike scroll: http://jsbin.com/rasewe
https://codereview.chromium.org/2364103003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/2364103003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:143: DoubleSize scrollOffset = frameView.scrollOffsetDouble(); DoubleSize scrollOffset = frameView.layoutViewportScrollableArea()->scrollOffsetDouble(); https://codereview.chromium.org/2364103003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:445: if (!scrollOffset.isZero() || layer->scrollsOverflow()) { I think you still need to create the transform for LayoutView even if scrolls don't overflow. Have you tried running tests with RLS enabled to verify?
https://codereview.chromium.org/2364103003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/2364103003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:143: DoubleSize scrollOffset = frameView.scrollOffsetDouble(); On 2016/09/26 at 19:57:54, szager1 wrote: > DoubleSize scrollOffset = frameView.layoutViewportScrollableArea()->scrollOffsetDouble(); Why change this? It matches the code before this change so I'd prefer to do it later. https://codereview.chromium.org/2364103003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:445: if (!scrollOffset.isZero() || layer->scrollsOverflow()) { On 2016/09/26 at 19:57:54, szager1 wrote: > I think you still need to create the transform for LayoutView even if scrolls don't overflow. Have you tried running tests with RLS enabled to verify? Why? With this patch I've switched FrameView to not do that, and all tests pass.
lgtm LGTM % szager
lgtm, fire away
On 2016/09/26 at 21:09:55, szager wrote: > lgtm, fire away Thanks for the reviews! In we go
The CQ bit was checked by pdr@chromium.org
The CQ bit was unchecked by pdr@chromium.org
The CQ bit was checked by pdr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Switch RootLayerScrolls to use root transform and scroll properties This is a followup to [1] to start using the root transform and scroll properties with root layer scrolling enabled. With this patch, the RLS/non-RLS property trees are no longer different. A real codechange in this patch is to not create a scroll translation for the FrameView unless it actually scrolls. This matches the logic used for adding LayoutView scroll translation properties. [1] https://crrev.com/a518f5340802393f8bdaa6b3d376dc5482049351 BUG=645615 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Switch RootLayerScrolls to use root transform and scroll properties This is a followup to [1] to start using the root transform and scroll properties with root layer scrolling enabled. With this patch, the RLS/non-RLS property trees are no longer different. A real codechange in this patch is to not create a scroll translation for the FrameView unless it actually scrolls. This matches the logic used for adding LayoutView scroll translation properties. [1] https://crrev.com/a518f5340802393f8bdaa6b3d376dc5482049351 BUG=645615 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/0abd71cfec612222822a4e4a1bb0bc89e937a9ce Cr-Commit-Position: refs/heads/master@{#421085} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/0abd71cfec612222822a4e4a1bb0bc89e937a9ce Cr-Commit-Position: refs/heads/master@{#421085}
Message was sent while issue was closed.
Description was changed from ========== Switch RootLayerScrolls to use root transform and scroll properties This is a followup to [1] to start using the root transform and scroll properties with root layer scrolling enabled. With this patch, the RLS/non-RLS property trees are no longer different. A real codechange in this patch is to not create a scroll translation for the FrameView unless it actually scrolls. This matches the logic used for adding LayoutView scroll translation properties. [1] https://crrev.com/a518f5340802393f8bdaa6b3d376dc5482049351 BUG=645615 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/0abd71cfec612222822a4e4a1bb0bc89e937a9ce Cr-Commit-Position: refs/heads/master@{#421085} ========== to ========== Switch RootLayerScrolls to use root transform and scroll properties This is a followup to [1] to start using the root transform and scroll properties with root layer scrolling enabled. With this patch, the RLS/non-RLS property trees are no longer different. A real codechange in this patch is to not create a scroll translation for the FrameView unless it actually scrolls. This matches the logic used for adding LayoutView scroll translation properties. [1] https://crrev.com/a518f5340802393f8bdaa6b3d376dc5482049351 BUG=645615, 650530 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/0abd71cfec612222822a4e4a1bb0bc89e937a9ce Cr-Commit-Position: refs/heads/master@{#421085} ==========
Message was sent while issue was closed.
Description was changed from ========== Switch RootLayerScrolls to use root transform and scroll properties This is a followup to [1] to start using the root transform and scroll properties with root layer scrolling enabled. With this patch, the RLS/non-RLS property trees are no longer different. A real codechange in this patch is to not create a scroll translation for the FrameView unless it actually scrolls. This matches the logic used for adding LayoutView scroll translation properties. [1] https://crrev.com/a518f5340802393f8bdaa6b3d376dc5482049351 BUG=645615, 650530 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/0abd71cfec612222822a4e4a1bb0bc89e937a9ce Cr-Commit-Position: refs/heads/master@{#421085} ========== to ========== Switch RootLayerScrolls to use root transform and scroll properties This is a followup to [1] to start using the root transform and scroll properties with root layer scrolling enabled. With this patch, the RLS/non-RLS property trees are no longer different. A real codechange in this patch is to not create a scroll translation for the FrameView unless it actually scrolls. This matches the logic used for adding LayoutView scroll translation properties. [1] https://crrev.com/a518f5340802393f8bdaa6b3d376dc5482049351 BUG=645615 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/0abd71cfec612222822a4e4a1bb0bc89e937a9ce Cr-Commit-Position: refs/heads/master@{#421085} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
