Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2961613002:
Slightly refactor StickyPositionScrollingConstraints API and add documentation (Closed)
|
Created:
3 years, 5 months ago by smcgruer Modified:
3 years, 4 months ago CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, eae+blinkwatch, jchaffraix+rendering, kenneth.christiansen, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor StickyPositionScrollingConstraints API and add documentation
Main changes:
1. Removed unused method |NearestStickyAncestor|.
2. Removed |GetTotalStickyBoxStickyOffset| and
|GetTotalContainingBlockStickyOffset|, which were only accessed in the
class implementation so can be accessed directly.
3. Switched ancestor pointers to PaintLayer rather than LayoutBoxModelObject
as we always just call ->Layer() on them.
4. Passed the StickyConstraintsMap into |ComputeStickyOffset| and find the
ancestors internally, the same way as |GetOffsetForStickyPosition|.
BUG=730043, 740070
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2961613002
Cr-Commit-Position: refs/heads/master@{#490996}
Committed: https://chromium.googlesource.com/chromium/src/+/979aeb4b47044335a43dbe2d570220cd07589747
Patch Set 1 #Patch Set 2 : More documentation, split out ancestor offset calculation #
Total comments: 16
Patch Set 3 : Address some reviewer changes #Patch Set 4 : remove accidental file #
Total comments: 7
Patch Set 5 : Fixed a few comments #Patch Set 6 : Rebase #Patch Set 7 : Attempt to address remaining comments #
Total comments: 2
Patch Set 8 : Fix it's -> its #
Total comments: 12
Patch Set 9 : Address most reviewer comments #
Total comments: 1
Patch Set 10 : Add diagram + example #
Messages
Total messages: 40 (18 generated)
Description was changed from ========== Slightly refactor StickyPositionScrollingConstraints API and add documentation Since we now make StickyConstraintsMap available to GetOffsetForStickyPosition, it makes sense to also pass it into ComputeStickyOffset. This allows us to do the lookup of the ancestor StickyPositionScrollingConstraints* inside ComputeStickyOffset rather than require our caller to do so. We can also convert the stored ancestor pointers from LayoutBoxModelObject to PaintLayer. BUG=730043 ========== to ========== Slightly refactor StickyPositionScrollingConstraints API and add documentation Since we now make StickyConstraintsMap available to GetOffsetForStickyPosition, it makes sense to also pass it into ComputeStickyOffset. This allows us to do the lookup of the ancestor StickyPositionScrollingConstraints* inside ComputeStickyOffset rather than require our caller to do so. We can also convert the stored ancestor pointers from LayoutBoxModelObject to PaintLayer. BUG=730043 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from
==========
Slightly refactor StickyPositionScrollingConstraints API and add documentation
Since we now make StickyConstraintsMap available to GetOffsetForStickyPosition,
it makes sense to also pass it into ComputeStickyOffset. This allows us to
do the lookup of the ancestor StickyPositionScrollingConstraints* inside
ComputeStickyOffset rather than require our caller to do so. We can also
convert the stored ancestor pointers from LayoutBoxModelObject to PaintLayer.
BUG=730043
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========
to
==========
Refactor StickyPositionScrollingConstraints API and add documentation
Main changes:
1. Removed unused method |NearestStickyAncestor|.
2. Removed |GetTotalStickyBoxStickyOffset| and
|GetTotalContainingBlockStickyOffset|, which were only accessed in the
class implementation so can be accessed directly.
3. Switched ancestor pointers to PaintLayer rather than LayoutBoxModelObject
as we always just call ->Layer() on them.
4. Passed the StickyConstraintsMap into |ComputeStickyOffset| and find the
ancestors internally, the same way as |GetOffsetForStickyPosition|.
BUG=730043
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========
smcgruer@chromium.org changed reviewers: + flackr@chromium.org
https://codereview.chromium.org/2961613002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right): https://codereview.chromium.org/2961613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:1098: StickyPositionScrollingConstraints* constraints = &it->value; Refactoring this code made me realize how dangerous this behavior is. StickyPositionScrollingConstraints is still being treated like a const PoD, however we need the specific object in the hashmap to be updated when ComputeStickyOffset is called. That only happens properly afaik because HashMap iteration happens to be implemented that way. I'm pretty sure we should change the map to be PaintLayer* -> std::unique_ptr<StickyPositionScrollingConstraints>, but haven't done so here. https://codereview.chromium.org/2961613002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp (right): https://codereview.chromium.org/2961613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp:25: // ourselves and our containing block, e.g. an inline parent. As such, I think this is inaccurate, an inline parent is insufficient here. If you look at http://output.jsbin.com/xoziyur/quiet, then sticky2 has no ancestors. It's location_container is an anonymous div whose Parent() is the scroller itself, not the outer inline sticky1 element. https://codereview.chromium.org/2961613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp:99: // TODO(smcgruer): Document why these are calculated like this and why the two As per our IM discussion, I am still trying to explain (and understand!) why we add 'ancestor_sticky_box_offset' for 'total_containing_block_sticky_offset_', especially given that no tests fail if I remove that. So far it is not easy, and I am beginning to wonder if it was predominantly table-related which is no longer reproducible since we dont support <tr> and <thead> anymore.
https://codereview.chromium.org/2961613002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right): https://codereview.chromium.org/2961613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:1098: StickyPositionScrollingConstraints* constraints = &it->value; On 2017/06/26 19:18:56, smcgruer wrote: > Refactoring this code made me realize how dangerous this behavior is. > StickyPositionScrollingConstraints is still being treated like a const PoD, > however we need the specific object in the hashmap to be updated when > ComputeStickyOffset is called. That only happens properly afaik because HashMap > iteration happens to be implemented that way. > > I'm pretty sure we should change the map to be PaintLayer* -> > std::unique_ptr<StickyPositionScrollingConstraints>, but haven't done so here. I think it's okay to modify the value being pointed to by the hashmap. https://codereview.chromium.org/2961613002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp (right): https://codereview.chromium.org/2961613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp:25: // ourselves and our containing block, e.g. an inline parent. As such, On 2017/06/26 19:18:56, smcgruer wrote: > I think this is inaccurate, an inline parent is insufficient here. If you look > at http://output.jsbin.com/xoziyur/quiet, then sticky2 has no ancestors. It's > location_container is an anonymous div whose Parent() is the scroller itself, > not the outer inline sticky1 element. Yeah so it needs to be a sticky layer that is not the containing block for the descendant but does shift it's location (i.e. location container). Maybe tables are the only real instance of this? Also, this sounds related to https://crbug.com/695481 https://codereview.chromium.org/2961613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp:125: DCHECK(constraints_map.Contains(nearest_sticky_layer_shifting_sticky_box_)); Is this check necessary when we use the constraints on the next line? i.e. does it not crash when trying to access the total_sticky_box_offset_ if that isn't in the map? https://codereview.chromium.org/2961613002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h (right): https://codereview.chromium.org/2961613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:21: // provides calculation of the sticky offset for a given viewport rectangle. I think viewport just refers to the frame scroller, mention overflow clip rectangle for the overflow scrollers. https://codereview.chromium.org/2961613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:28: // constraints (i.e. the distance from each edge the element should stick). Mention that all of these are rectangles with respect to the nearest ancestor scroller, which the sticky elements stick to. https://codereview.chromium.org/2961613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:88: // and is only safe to call if ComputeStickyOffset has already been invoked. Maybe it's better to relate this to when we expect ComputeStickyOffset must have been invoked by, i.e. when compositing inputs (or prepaint after spv2 iiuc) are clean for the sticky element.
https://codereview.chromium.org/2961613002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp (right): https://codereview.chromium.org/2961613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp:25: // ourselves and our containing block, e.g. an inline parent. As such, On 2017/06/29 15:24:10, flackr wrote: > On 2017/06/26 19:18:56, smcgruer wrote: > > I think this is inaccurate, an inline parent is insufficient here. If you look > > at http://output.jsbin.com/xoziyur/quiet, then sticky2 has no ancestors. It's > > location_container is an anonymous div whose Parent() is the scroller itself, > > not the outer inline sticky1 element. > > Yeah so it needs to be a sticky layer that is not the containing block for the > descendant but does shift it's location (i.e. location container). Maybe tables > are the only real instance of this? Also, this sounds related to > https://crbug.com/695481 Nested inline appears to work, so will likely use that as an example (not rewritten yet). Also https://crbug.com/695481 is not nested sticky, did you think it related in some other way? https://codereview.chromium.org/2961613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp:125: DCHECK(constraints_map.Contains(nearest_sticky_layer_shifting_sticky_box_)); On 2017/06/29 15:24:10, flackr wrote: > Is this check necessary when we use the constraints on the next line? i.e. does > it not crash when trying to access the total_sticky_box_offset_ if that isn't in > the map? It's necessary. at() just calls EmptyValue() if it doesn't find anything. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/wtf/H... https://codereview.chromium.org/2961613002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h (right): https://codereview.chromium.org/2961613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:21: // provides calculation of the sticky offset for a given viewport rectangle. On 2017/06/29 15:24:10, flackr wrote: > I think viewport just refers to the frame scroller, mention overflow clip > rectangle for the overflow scrollers. Done. https://codereview.chromium.org/2961613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:28: // constraints (i.e. the distance from each edge the element should stick). On 2017/06/29 15:24:10, flackr wrote: > Mention that all of these are rectangles with respect to the nearest ancestor > scroller, which the sticky elements stick to. Done. https://codereview.chromium.org/2961613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:88: // and is only safe to call if ComputeStickyOffset has already been invoked. On 2017/06/29 15:24:10, flackr wrote: > Maybe it's better to relate this to when we expect ComputeStickyOffset must have > been invoked by, i.e. when compositing inputs (or prepaint after spv2 iiuc) are > clean for the sticky element. Done.
smcgruer@chromium.org changed reviewers: + yigu@chromium.org
https://codereview.chromium.org/2961613002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp (right): https://codereview.chromium.org/2961613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp:25: // ourselves and our containing block, e.g. an inline parent. As such, On 2017/06/29 18:51:46, smcgruer wrote: > On 2017/06/29 15:24:10, flackr wrote: > > On 2017/06/26 19:18:56, smcgruer wrote: > > > I think this is inaccurate, an inline parent is insufficient here. If you > look > > > at http://output.jsbin.com/xoziyur/quiet, then sticky2 has no ancestors. > It's > > > location_container is an anonymous div whose Parent() is the scroller > itself, > > > not the outer inline sticky1 element. > > > > Yeah so it needs to be a sticky layer that is not the containing block for the > > descendant but does shift it's location (i.e. location container). Maybe > tables > > are the only real instance of this? Also, this sounds related to > > https://crbug.com/695481 > > Nested inline appears to work, so will likely use that as an example (not > rewritten yet). Also https://crbug.com/695481 is not nested sticky, did you > think it related in some other way? Yes, I was thinking it might be related if the reason the block elements aren't moving might be because their location container isn't the inline. https://codereview.chromium.org/2961613002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp (right): https://codereview.chromium.org/2961613002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp:100: // different members are necessary. There's some comments in the header file for these variables. Maybe augment this documentation?
Nice documentation! I wish I could see that earlier.. https://codereview.chromium.org/2961613002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h (right): https://codereview.chromium.org/2961613002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:90: // element. (Or after prepaint for SlimmingPaintV2). Do you need to mention that this method should be called after ComputeStickyOffset being invoked?
https://codereview.chromium.org/2961613002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h (right): https://codereview.chromium.org/2961613002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:90: // element. (Or after prepaint for SlimmingPaintV2). On 2017/07/05 19:13:56, yigu wrote: > Do you need to mention that this method should be called after > ComputeStickyOffset being invoked? In patchset 2, flackr asked me to relate this to lifecycle and explicitly not the method call. Unless I misunderstood him - flackr?
https://codereview.chromium.org/2961613002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h (right): https://codereview.chromium.org/2961613002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:90: // element. (Or after prepaint for SlimmingPaintV2). On 2017/07/06 14:25:37, smcgruer wrote: > On 2017/07/05 19:13:56, yigu wrote: > > Do you need to mention that this method should be called after > > ComputeStickyOffset being invoked? > > In patchset 2, flackr asked me to relate this to lifecycle and explicitly not > the method call. Unless I misunderstood him - flackr? That's correct, because the lifecycle is a global concept, whereas saying after that method is invoked requires then figuring out when it is invoked. If you feel like it should explicitly call that out though we could say both, e.g. "must only be called after ComputeStickyOffset has been invoked (i.e. after compositing inputs are updated or after prepaint when SlimmingPaintV2 is enabled)."
https://codereview.chromium.org/2961613002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp (right): https://codereview.chromium.org/2961613002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp:25: // ourselves and our containing block, e.g. an inline parent. As such, On 2017/06/30 02:48:35, flackr wrote: > On 2017/06/29 18:51:46, smcgruer wrote: > > On 2017/06/29 15:24:10, flackr wrote: > > > On 2017/06/26 19:18:56, smcgruer wrote: > > > > I think this is inaccurate, an inline parent is insufficient here. If you > > look > > > > at http://output.jsbin.com/xoziyur/quiet, then sticky2 has no ancestors. > > It's > > > > location_container is an anonymous div whose Parent() is the scroller > > itself, > > > > not the outer inline sticky1 element. > > > > > > Yeah so it needs to be a sticky layer that is not the containing block for > the > > > descendant but does shift it's location (i.e. location container). Maybe > > tables > > > are the only real instance of this? Also, this sounds related to > > > https://crbug.com/695481 > > > > Nested inline appears to work, so will likely use that as an example (not > > rewritten yet). Also https://crbug.com/695481 is not nested sticky, did you > > think it related in some other way? > > Yes, I was thinking it might be related if the reason the block elements aren't > moving might be because their location container isn't the inline. Fixed the comment. https://codereview.chromium.org/2961613002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp (right): https://codereview.chromium.org/2961613002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp:100: // different members are necessary. On 2017/06/30 02:48:35, flackr wrote: > There's some comments in the header file for these variables. Maybe augment this > documentation? Done.
https://codereview.chromium.org/2961613002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp (right): https://codereview.chromium.org/2961613002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp:100: // different members are necessary. On 2017/07/07 13:51:58, smcgruer wrote: > On 2017/06/30 02:48:35, flackr wrote: > > There's some comments in the header file for these variables. Maybe augment > this > > documentation? > > Done. Sorry, to be specific I meant augment the documentation in the header file if necessary. The reason they're calculated like this is because total_sticky_box_offset already includes ancestor_containing_block_offset. the sticky_offset is added to both because if this element is used in any descendant either as a proper container or just as a location container it needs to know about this element's sticky offset.
Description was changed from
==========
Refactor StickyPositionScrollingConstraints API and add documentation
Main changes:
1. Removed unused method |NearestStickyAncestor|.
2. Removed |GetTotalStickyBoxStickyOffset| and
|GetTotalContainingBlockStickyOffset|, which were only accessed in the
class implementation so can be accessed directly.
3. Switched ancestor pointers to PaintLayer rather than LayoutBoxModelObject
as we always just call ->Layer() on them.
4. Passed the StickyConstraintsMap into |ComputeStickyOffset| and find the
ancestors internally, the same way as |GetOffsetForStickyPosition|.
BUG=730043
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========
to
==========
Refactor StickyPositionScrollingConstraints API and add documentation
Main changes:
1. Removed unused method |NearestStickyAncestor|.
2. Removed |GetTotalStickyBoxStickyOffset| and
|GetTotalContainingBlockStickyOffset|, which were only accessed in the
class implementation so can be accessed directly.
3. Switched ancestor pointers to PaintLayer rather than LayoutBoxModelObject
as we always just call ->Layer() on them.
4. Passed the StickyConstraintsMap into |ComputeStickyOffset| and find the
ancestors internally, the same way as |GetOffsetForStickyPosition|.
BUG=730043,740070
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========
https://codereview.chromium.org/2961613002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp (right): https://codereview.chromium.org/2961613002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp:100: // different members are necessary. On 2017/07/19 17:13:34, flackr wrote: > On 2017/07/07 13:51:58, smcgruer wrote: > > On 2017/06/30 02:48:35, flackr wrote: > > > There's some comments in the header file for these variables. Maybe augment > > this > > > documentation? > > > > Done. > > Sorry, to be specific I meant augment the documentation in the header file if > necessary. The reason they're calculated like this is because > total_sticky_box_offset already includes ancestor_containing_block_offset. the > sticky_offset is added to both because if this element is used in any descendant > either as a proper container or just as a location container it needs to know > about this element's sticky offset. Done.
lgtm
smcgruer@chromium.org changed reviewers: + chrishtr@chromium.org, trchen@chromium.org
Adding trchen, chrishtr for OWNERS. Also if you have any thoughts on the sticky documentation and where you'd like to see more detail, please do let me know! :)
https://codereview.chromium.org/2961613002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h (right): https://codereview.chromium.org/2961613002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:27: // rectangle and it's containing block rectangle (both respective to the nearest s/it's/its/
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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by smcgruer@chromium.org to run a CQ dry run
https://codereview.chromium.org/2961613002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h (right): https://codereview.chromium.org/2961613002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:27: // rectangle and it's containing block rectangle (both respective to the nearest On 2017/07/27 00:50:32, chrishtr wrote: > s/it's/its/ Done. TIL I am bad at English grammar. It wasn't that much of a surprise.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2961613002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp (right): https://codereview.chromium.org/2961613002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp:10: FloatSize StickyPositionScrollingConstraints::ComputeStickyOffset( Not going to look at code changes here, since flackr@ already reviewed them. https://codereview.chromium.org/2961613002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h (right): https://codereview.chromium.org/2961613002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:40: class StickyPositionScrollingConstraints final { I think it would be good to add a representative example with diagram for each of the fields stored in StickyPositionScrollingConstraints. e.g. <div scroller> <div sticky></div> </div> +-------------------------------------------+ +Diagram of sticky constraints before scroll +-------------------------------------------+ +-------------------------------------------+ +Diagram of sticky constraints after scroll +-------------------------------------------+ https://codereview.chromium.org/2961613002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:175: // The layout position of the sticky element's containing block rectangle, "The layout position of the containing block of the sticky position, relative to the sticky element's scroll container." Even this though is, while correct, hard to understand unless you think about it for a while. Referring to an example as I suggested above should help. Another thing that might help is to define terms as variables, and refer to them here e.g. <containing-scroller> <containing-block> (*) <sticky-element> (*) <containing-block> might be the same as <containing-scroller> Then the sentence here would read "layout position of containing block relative to containing-scroller. https://codereview.chromium.org/2961613002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:180: // The layout position of the sticky element. When calculating the sticky ...sticky element relative to its scroll container. https://codereview.chromium.org/2961613002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:206: // containing block sticky offsets. Add example.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2961613002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp (right): https://codereview.chromium.org/2961613002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.cpp:10: FloatSize StickyPositionScrollingConstraints::ComputeStickyOffset( On 2017/07/27 15:13:39, chrishtr wrote: > Not going to look at code changes here, since flackr@ already reviewed them. Acknowledged. https://codereview.chromium.org/2961613002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h (right): https://codereview.chromium.org/2961613002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:40: class StickyPositionScrollingConstraints final { On 2017/07/27 15:13:39, chrishtr wrote: > I think it would be good to add a representative example with diagram > for each of the fields stored in StickyPositionScrollingConstraints. > > e.g. > > <div scroller> > <div sticky></div> > </div> > > +-------------------------------------------+ > +Diagram of sticky constraints before scroll > +-------------------------------------------+ > > +-------------------------------------------+ > +Diagram of sticky constraints after scroll > +-------------------------------------------+ In terms of StickyPositionScrollingConstraints publically visible state, it shouldn't change before/after scroll - only the ComputeStickyOffset return value changes. To be clearer, the only members that should ever change after scroll are total_sticky_box_sticky_offset_ and total_containing_block_sticky_offset_, which I don't think should be documented in the class header. I could still try and add a general example without the before/after if you think that'd be useful? https://codereview.chromium.org/2961613002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:175: // The layout position of the sticky element's containing block rectangle, On 2017/07/27 15:13:39, chrishtr wrote: > "The layout position of the containing block of the sticky position, relative > to the sticky element's scroll container." > > Even this though is, while correct, hard to understand unless you think about > it for a while. Referring to an example as I suggested above should help. > > Another thing that might help is to define terms as variables, and refer to them > here > > e.g. > > <containing-scroller> > <containing-block> (*) > <sticky-element> > > (*) <containing-block> might be the same as <containing-scroller> > > > Then the sentence here would read "layout position of containing block relative > to containing-scroller. Done, though I'm not sure it improved readability that much. https://codereview.chromium.org/2961613002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:180: // The layout position of the sticky element. When calculating the sticky On 2017/07/27 15:13:39, chrishtr wrote: > ...sticky element relative to its scroll container. Done. https://codereview.chromium.org/2961613002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:206: // containing block sticky offsets. On 2017/07/27 15:13:39, chrishtr wrote: > Add example. Done.
https://codereview.chromium.org/2961613002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h (right): https://codereview.chromium.org/2961613002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:40: class StickyPositionScrollingConstraints final { On 2017/07/28 at 17:35:57, smcgruer wrote: > On 2017/07/27 15:13:39, chrishtr wrote: > > I think it would be good to add a representative example with diagram > > for each of the fields stored in StickyPositionScrollingConstraints. > > > > e.g. > > > > <div scroller> > > <div sticky></div> > > </div> > > > > +-------------------------------------------+ > > +Diagram of sticky constraints before scroll > > +-------------------------------------------+ > > > > +-------------------------------------------+ > > +Diagram of sticky constraints after scroll > > +-------------------------------------------+ > > In terms of StickyPositionScrollingConstraints publically visible state, it shouldn't change before/after scroll - only the ComputeStickyOffset return value changes. To be clearer, the only members that should ever change after scroll are total_sticky_box_sticky_offset_ and total_containing_block_sticky_offset_, which I don't think should be documented in the class header. > > I could still try and add a general example without the before/after if you think that'd be useful? I think an example of the flow of information and how it's updated is quite valuable. Either here or in a markdown file. https://codereview.chromium.org/2961613002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h (right): https://codereview.chromium.org/2961613002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:37: // Unfortunately this approach breaks down in the presence of nested sticky Add an example of nested sticky, here or in the implementation
https://codereview.chromium.org/2961613002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h (right): https://codereview.chromium.org/2961613002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/page/scrolling/StickyPositionScrollingConstraints.h:40: class StickyPositionScrollingConstraints final { On 2017/07/28 18:25:51, chrishtr wrote: > On 2017/07/28 at 17:35:57, smcgruer wrote: > > On 2017/07/27 15:13:39, chrishtr wrote: > > > I think it would be good to add a representative example with diagram > > > for each of the fields stored in StickyPositionScrollingConstraints. > > > > > > e.g. > > > > > > <div scroller> > > > <div sticky></div> > > > </div> > > > > > > +-------------------------------------------+ > > > +Diagram of sticky constraints before scroll > > > +-------------------------------------------+ > > > > > > +-------------------------------------------+ > > > +Diagram of sticky constraints after scroll > > > +-------------------------------------------+ > > > > In terms of StickyPositionScrollingConstraints publically visible state, it > shouldn't change before/after scroll - only the ComputeStickyOffset return value > changes. To be clearer, the only members that should ever change after scroll > are total_sticky_box_sticky_offset_ and total_containing_block_sticky_offset_, > which I don't think should be documented in the class header. > > > > I could still try and add a general example without the before/after if you > think that'd be useful? > > I think an example of the flow of information and how it's updated is quite > valuable. Either here or in a markdown file. Done.
lgtm
The CQ bit was checked by smcgruer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org Link to the patchset: https://codereview.chromium.org/2961613002/#ps180001 (title: "Add diagram + example")
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": 1501594376142570,
"parent_rev": "bbc880900a47a3d21b407c9e5b87510dd926ed55", "commit_rev":
"979aeb4b47044335a43dbe2d570220cd07589747"}
Message was sent while issue was closed.
Description was changed from
==========
Refactor StickyPositionScrollingConstraints API and add documentation
Main changes:
1. Removed unused method |NearestStickyAncestor|.
2. Removed |GetTotalStickyBoxStickyOffset| and
|GetTotalContainingBlockStickyOffset|, which were only accessed in the
class implementation so can be accessed directly.
3. Switched ancestor pointers to PaintLayer rather than LayoutBoxModelObject
as we always just call ->Layer() on them.
4. Passed the StickyConstraintsMap into |ComputeStickyOffset| and find the
ancestors internally, the same way as |GetOffsetForStickyPosition|.
BUG=730043,740070
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========
to
==========
Refactor StickyPositionScrollingConstraints API and add documentation
Main changes:
1. Removed unused method |NearestStickyAncestor|.
2. Removed |GetTotalStickyBoxStickyOffset| and
|GetTotalContainingBlockStickyOffset|, which were only accessed in the
class implementation so can be accessed directly.
3. Switched ancestor pointers to PaintLayer rather than LayoutBoxModelObject
as we always just call ->Layer() on them.
4. Passed the StickyConstraintsMap into |ComputeStickyOffset| and find the
ancestors internally, the same way as |GetOffsetForStickyPosition|.
BUG=730043,740070
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2961613002
Cr-Commit-Position: refs/heads/master@{#490996}
Committed:
https://chromium.googlesource.com/chromium/src/+/979aeb4b47044335a43dbe2d5702...
==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/979aeb4b47044335a43dbe2d5702... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
