Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2561693002:
Move sticky position constraints update to the pre-paint tree walk. (Closed)
|
Created:
4 years ago by chrishtr Modified:
4 years ago Reviewers:
pdr. CC:
blink-reviews, blink-reviews-layout_chromium.org, blink-reviews-paint_chromium.org, chromium-reviews, dshwang, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMove sticky position constraints update to the pre-paint tree walk.
As part of this, also moved computation of PaintLayer::ancestorScrollingLayer,
because that is required for sticky position constraint updates.
BUG=646188
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Committed: https://crrev.com/d4485c145cfb09160c216e6792a995b133cbc3c3
Cr-Commit-Position: refs/heads/master@{#437671}
Patch Set 1 #Patch Set 2 : none #Patch Set 3 : none #Patch Set 4 : none #Patch Set 5 : none #Patch Set 6 : none #Patch Set 7 : none #Patch Set 8 : none #
Total comments: 2
Patch Set 9 : none #
Total comments: 1
Patch Set 10 : none #
Messages
Total messages: 51 (32 generated)
Description was changed from ========== none none none none BUG= ========== to ========== none none none none BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== none none none none BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Move stick position constraints update to the pre-paint tree walk. BUG=646188 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Move stick position constraints update to the pre-paint tree walk. BUG=646188 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Move sticky position constraints update to the pre-paint tree walk. BUG=646188 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
chrishtr@chromium.org changed reviewers: + pdr@chromium.org
The CQ bit was checked by chrishtr@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...
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
The CQ bit was checked by chrishtr@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_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
Description was changed from ========== Move sticky position constraints update to the pre-paint tree walk. BUG=646188 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Move sticky position constraints update to the pre-paint tree walk. As part of this, also moved computation of PaintLayer::ancestorScrollingLayer, because that is required for sticky position constraint updates. BUG=646188 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Updated to also move ancestorScrollingLayer, which is needed for sticky constraints.
The CQ bit was checked by chrishtr@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2561693002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp (right): https://codereview.chromium.org/2561693002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:30: // The ancestor in the PaintLayer tree which has overflow clip, or The PrePaintTreeWalk is shared by paint invalidation, property tree building, and compositing thingers. Unfortunately, there is a little paint property logic spilling over (paint offset diffing) but otherwise we've kept real logic in other classes. In the interest of keeping the prepaint tree walk as simple as possible, WDYT of moving this into a helper file?
https://codereview.chromium.org/2561693002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp (right): https://codereview.chromium.org/2561693002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:30: // The ancestor in the PaintLayer tree which has overflow clip, or On 2016/12/09 at 00:04:10, pdr. wrote: > The PrePaintTreeWalk is shared by paint invalidation, property tree building, and compositing thingers. Unfortunately, there is a little paint property logic spilling over (paint offset diffing) but otherwise we've kept real logic in other classes. In the interest of keeping the prepaint tree walk as simple as possible, WDYT of moving this into a helper file? Are you suggesting making an AuxiliaryObjectPropertiesContext struct, and moving the updateAuxiliaryObjectProperties() into that file?
The CQ bit was checked by chrishtr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/12/09 at 01:22:55, chrishtr wrote: > https://codereview.chromium.org/2561693002/diff/140001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp (right): > > https://codereview.chromium.org/2561693002/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:30: // The ancestor in the PaintLayer tree which has overflow clip, or > On 2016/12/09 at 00:04:10, pdr. wrote: > > The PrePaintTreeWalk is shared by paint invalidation, property tree building, and compositing thingers. Unfortunately, there is a little paint property logic spilling over (paint offset diffing) but otherwise we've kept real logic in other classes. In the interest of keeping the prepaint tree walk as simple as possible, WDYT of moving this into a helper file? > > Are you suggesting making an AuxiliaryObjectPropertiesContext struct, and moving the updateAuxiliaryObjectProperties() into that file? If that is the suggestion, maybe it's premature. If I add another property then I can do that refactoring. WDYT?
On 2016/12/09 at 02:01:39, chrishtr wrote: > On 2016/12/09 at 01:22:55, chrishtr wrote: > > https://codereview.chromium.org/2561693002/diff/140001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp (right): > > > > https://codereview.chromium.org/2561693002/diff/140001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:30: // The ancestor in the PaintLayer tree which has overflow clip, or > > On 2016/12/09 at 00:04:10, pdr. wrote: > > > The PrePaintTreeWalk is shared by paint invalidation, property tree building, and compositing thingers. Unfortunately, there is a little paint property logic spilling over (paint offset diffing) but otherwise we've kept real logic in other classes. In the interest of keeping the prepaint tree walk as simple as possible, WDYT of moving this into a helper file? > > > > Are you suggesting making an AuxiliaryObjectPropertiesContext struct, and moving the updateAuxiliaryObjectProperties() into that file? > > If that is the suggestion, maybe it's premature. If I add another property then I can do that refactoring. WDYT? That was the suggestion. I'm okay waiting to see if there are more. High level question: in both PaintPropertyTreeBuilder and PaintInvalidator, we've not needed to use PaintLayer much, in part because it is the old spv1 compositing system. Should we be refactoring the compositing inputs to be PaintLayer-agnostic?
One more question.. why do we now fail spv2 tests in the latest patch?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
The CQ bit was checked by chrishtr@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...
Fixed the tests in fast/css/sticky/ for SPv2. Turned out I needed to move updateAuxiliaryObjectProperties higher in the walk() method, because updateContextForBoxPosition needs to call LayoutBoxModelObject::stickyPositionOffset to compute box offset position.
The CQ bit was unchecked by chrishtr@chromium.org
The CQ bit was checked by chrishtr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org Link to the patchset: https://codereview.chromium.org/2561693002/#ps160001 (title: "none")
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
Try jobs failed on following builders: linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)
https://codereview.chromium.org/2561693002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp (right): https://codereview.chromium.org/2561693002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:130: // latter reads some of the state computed uere. nit: here What dependent state is read by updateContextForBoxPosition?
On 2016/12/09 at 05:38:49, pdr wrote: > https://codereview.chromium.org/2561693002/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp (right): > > https://codereview.chromium.org/2561693002/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:130: // latter reads some of the state computed uere. > nit: here > > What dependent state is read by updateContextForBoxPosition? Call stack: #1 0x7f27b96e6e17 blink::LayoutBoxModelObject::stickyPositionOffset() #2 0x7f27b96e7380 blink::LayoutBoxModelObject::offsetForInFlowPosition() #3 0x7f27b9a62a7d blink::PaintPropertyTreeBuilder::updateContextForBoxPosition()
On 2016/12/09 at 17:43:48, chrishtr wrote: > On 2016/12/09 at 05:38:49, pdr wrote: > > https://codereview.chromium.org/2561693002/diff/160001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp (right): > > > > https://codereview.chromium.org/2561693002/diff/160001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/paint/PrePaintTreeWalk.cpp:130: // latter reads some of the state computed uere. > > nit: here > > > > What dependent state is read by updateContextForBoxPosition? > > Call stack: > > #1 0x7f27b96e6e17 blink::LayoutBoxModelObject::stickyPositionOffset() > #2 0x7f27b96e7380 blink::LayoutBoxModelObject::offsetForInFlowPosition() > #3 0x7f27b9a62a7d blink::PaintPropertyTreeBuilder::updateContextForBoxPosition() Cool, for this reason it now sounds good that we call setNeedsPaintPropertyUpdate
LGTM
Also, fast/block/float/float-change-composited-scrolling.html is now broken in SPv2 mode. This is because composited scrolling is broken with SPv2, and the float code gets confused about that.
The CQ bit was checked by chrishtr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org Link to the patchset: https://codereview.chromium.org/2561693002/#ps180001 (title: "none")
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": 180001, "attempt_start_ts": 1481312081788350,
"parent_rev": "a02366f1aef48d61d0c65c6030fc744bd2e52814", "commit_rev":
"c27205108eecf45b7d1f5ced6586e073c72e83ff"}
Message was sent while issue was closed.
Description was changed from ========== Move sticky position constraints update to the pre-paint tree walk. As part of this, also moved computation of PaintLayer::ancestorScrollingLayer, because that is required for sticky position constraint updates. BUG=646188 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Move sticky position constraints update to the pre-paint tree walk. As part of this, also moved computation of PaintLayer::ancestorScrollingLayer, because that is required for sticky position constraint updates. BUG=646188 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2561693002 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Move sticky position constraints update to the pre-paint tree walk. As part of this, also moved computation of PaintLayer::ancestorScrollingLayer, because that is required for sticky position constraint updates. BUG=646188 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2561693002 ========== to ========== Move sticky position constraints update to the pre-paint tree walk. As part of this, also moved computation of PaintLayer::ancestorScrollingLayer, because that is required for sticky position constraint updates. BUG=646188 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/d4485c145cfb09160c216e6792a995b133cbc3c3 Cr-Commit-Position: refs/heads/master@{#437671} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/d4485c145cfb09160c216e6792a995b133cbc3c3 Cr-Commit-Position: refs/heads/master@{#437671} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
