Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2383113003:
Refactor ScrollableArea::setScrollPosition. (Closed)
|
Created:
4 years, 2 months ago by szager1 Modified:
4 years, 2 months ago CC:
chromium-reviews, blink-reviews-html_chromium.org, blink-reviews-platform-graphics_chromium.org, dshwang, eae+blinkwatch, shans, kinuko+watch, rwlbuis, krit, drott+blinkwatch_chromium.org, szager+layoutwatch_chromium.org, Justin Novosad, dglazkov+blink, Rik, jchaffraix+rendering, blink-reviews-paint_chromium.org, blink-reviews, ajuma+watch_chromium.org, Eric Willigers, rjwright, zoltan1, blink-reviews-layout_chromium.org, jbroman, pdr+graphicswatchlist_chromium.org, darktears, Stephen Chennney, blink-reviews-animation_chromium.org, pdr+renderingwatchlist_chromium.org, leviw+renderwatch, slimming-paint-reviews_chromium.org, blink-layers+watch_chromium.org, f(malita), danakj+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor ScrollableArea::setScrollPosition.
- Eliminate some redundant PaintLayerScrollableArea methods and
indirection.
- ScrollableArea::setScrollPosition now always clamps its argument.
Code inspection revealed that nearly all callers were clamping
the position argument anyway.
- ScrollableArea::setScrollPosition returns immediately if the scroll
position did not change. This avoids a bunch of unnecessary
work.
- For the rare instances where the scroll position should not be
clamped, or the scroll position update code should run even if the
scroll position didn't change, add the method
setScrollPositionUnconditionally.
- When modifying the scroll position due to a change in effective
zoom, preserve the offset from beginning of flow, rather than the
distance from top-left of overflow area.
BUG=417782
R=skobes@chromium.org,bokan@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Committed: https://crrev.com/cddc823c56d453758003a831fd5a2475e94c0387
Cr-Commit-Position: refs/heads/master@{#423033}
Patch Set 1 #
Total comments: 8
Patch Set 2 : tweaks #Patch Set 3 : Back out offset/position renames #Patch Set 4 : rebase #Patch Set 5 : Fix clamping, comment tweaks #
Total comments: 16
Patch Set 6 : nits and rebase #Dependent Patchsets: Messages
Total messages: 44 (26 generated)
Description was changed from ========== Refactor ScrollableArea::setScrollPosition. Previously, the terms 'position' and 'offset' were used inconsitently, particularly in PaintLayerScrollableArea. This change enforces consistent usage: 'position' is the offset of a scrolling viewport from the beginning of flow of its contents (e.g., for LTR contents it means scrolled all the way to the left, for RTL contents it means scrolled all the way to the right). 'offsetFromOrigin' is the position of the scrolling viewport relative to the scroll origin of the ScrollableArea. For a description of 'scroll origin' (with ascii art!) refer to core/layout/README.md. Other changes in this patch: - Eliminate some redundant PLSA methods and indirection. - setScrollPosition always clamps its argument. Code inspection revealed that nearly all callers were clamping the position argument anyway. - setScrollPosition returns immediately if the scroll position did not change. This avoids a bunch of unnecessary work. - For the rare instances where the scroll position should not be clamped, or the scroll position update code should run even if the scroll position didn't change, add the method setScrollPositionUnconditionally. - When modifying the scroll position due to a change in effective zoom, preserve the 'position' (i.e, distance from beginning of flow) rather that 'offsetFromOrigin' (i.e., the distance from top-left of overflow area). BUG=417782 R=skobes@chromium.org,bokan@chromium.org ========== to ========== Refactor ScrollableArea::setScrollPosition. Previously, the terms 'position' and 'offset' were used inconsitently, particularly in PaintLayerScrollableArea. This change enforces consistent usage: 'position' is the offset of a scrolling viewport from the beginning of flow of its contents (e.g., for LTR contents it means scrolled all the way to the left, for RTL contents it means scrolled all the way to the right). 'offsetFromOrigin' is the position of the scrolling viewport relative to the scroll origin of the ScrollableArea. For a description of 'scroll origin' (with ascii art!) refer to core/layout/README.md. Other changes in this patch: - Eliminate some redundant PLSA methods and indirection. - setScrollPosition always clamps its argument. Code inspection revealed that nearly all callers were clamping the position argument anyway. - setScrollPosition returns immediately if the scroll position did not change. This avoids a bunch of unnecessary work. - For the rare instances where the scroll position should not be clamped, or the scroll position update code should run even if the scroll position didn't change, add the method setScrollPositionUnconditionally. - When modifying the scroll position due to a change in effective zoom, preserve the 'position' (i.e, distance from beginning of flow) rather that 'offsetFromOrigin' (i.e., the distance from top-left of overflow area). BUG=417782 R=skobes@chromium.org,bokan@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by szager@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.
Note that some of the naming here ('position' and 'offsetFromOrigin') will
change in the dependent patch which switches all scroll offsets to float.
Thank you for splitting it up, this is much easier to digest. I'm about half way through but got confused so I need some feedback before continuing. https://codereview.chromium.org/2383113003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): https://codereview.chromium.org/2383113003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:2824: // Note: this is just the scroll offset, *not* the "adjusted scroll offset". Scroll offset This comment needs to be updated https://codereview.chromium.org/2383113003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h (right): https://codereview.chromium.org/2383113003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:316: DoubleSize offsetFromOrigin() const { I find this naming to be confusing, I think what this method is doing is returning the distance of the ScrollableArea's top-left corner from the overflow content's top-left corner. It took me a while to realize that "offsetFromOrigin" isn't "offsetFrom*Scroll*Origin". Perhaps we should call it "offsetFromTopLeft"? Alternatively, leave it so that the origin means the overflow content's top left corner and rename scrollOrigin() to writingModeAdjustment() or something like that. Or am I completely misunderstanding? https://codereview.chromium.org/2383113003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:561: // This is the absolute scroll position, *not* relative to m_scrollOrigin. Ok, I think I'm missing something. Absolute would mean relative to the top left? This seems to be the opposite implied in the description: > 'position' is the offset of a scrolling viewport from the beginning of > flow of its contents (e.g., for LTR contents it means scrolled all the > way to the left, for RTL contents it means scrolled all the way to the > right). Is this supposed to mean "position (0, 0) is scrolled all the way up and left in LTR and down and right in RTL"? In that case, that does seem to be relative to m_scrollOrigin. > 'offsetFromOrigin' is the position of the scrolling viewport relative > to the scroll origin of the ScrollableArea. For a description of > 'scroll origin' (with ascii art!) refer to core/layout/README.md. offsetFromOrigin() is defined as: return toDoubleSize(DoublePoint(scrollOrigin())) + m_scrollPosition; So (referring to the ASCII art in the README.md) if we're RTL and assume scrollOrigin is (200, 0) and scrollPosition is (0, 0), offsetFromOrigin will be (200, 0) which implies it's not relative to the origin but the top left. https://codereview.chromium.org/2383113003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (right): https://codereview.chromium.org/2383113003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:195: scrollAnimator().adjustAnimationAndSetScrollPosition(clampedPosition, I think you can remove the clamps in ScrollAnimator::adjustAnimationAndSetScrollPosition and ScrollAnimatorCompositorCoordinator::adjustAnimationAndSetScrollPosition.
Description was changed from ========== Refactor ScrollableArea::setScrollPosition. Previously, the terms 'position' and 'offset' were used inconsitently, particularly in PaintLayerScrollableArea. This change enforces consistent usage: 'position' is the offset of a scrolling viewport from the beginning of flow of its contents (e.g., for LTR contents it means scrolled all the way to the left, for RTL contents it means scrolled all the way to the right). 'offsetFromOrigin' is the position of the scrolling viewport relative to the scroll origin of the ScrollableArea. For a description of 'scroll origin' (with ascii art!) refer to core/layout/README.md. Other changes in this patch: - Eliminate some redundant PLSA methods and indirection. - setScrollPosition always clamps its argument. Code inspection revealed that nearly all callers were clamping the position argument anyway. - setScrollPosition returns immediately if the scroll position did not change. This avoids a bunch of unnecessary work. - For the rare instances where the scroll position should not be clamped, or the scroll position update code should run even if the scroll position didn't change, add the method setScrollPositionUnconditionally. - When modifying the scroll position due to a change in effective zoom, preserve the 'position' (i.e, distance from beginning of flow) rather that 'offsetFromOrigin' (i.e., the distance from top-left of overflow area). BUG=417782 R=skobes@chromium.org,bokan@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Refactor ScrollableArea::setScrollPosition. Previously, the terms 'position' and 'offset' were used inconsitently, particularly in PaintLayerScrollableArea. This change enforces consistent usage: 'position' is the offset of a scrolling viewport from the beginning of flow of its contents (e.g., for LTR contents it means scrolled all the way to the left, for RTL contents it means scrolled all the way to the right). 'offsetFromOrigin' is the position of the scrolling viewport relative to the scroll origin of the ScrollableArea. For a description of 'scroll origin' (with ascii art!) refer to core/layout/README.md. Other changes in this patch: - Eliminate some redundant PLSA methods and indirection. - setScrollPosition always clamps its argument. Code inspection revealed that nearly all callers were clamping the position argument anyway. - setScrollPosition returns immediately if the scroll position did not change. This avoids a bunch of unnecessary work. - For the rare instances where the scroll position should not be clamped, or the scroll position update code should run even if the scroll position didn't change, add the method setScrollPositionUnconditionally. - When modifying the scroll position due to a change in effective zoom, preserve the 'position' (i.e, distance from beginning of flow) rather that 'offsetFromOrigin' (i.e., the distance from top-left of overflow area). BUG=417782 R=skobes@chromium.org,bokan@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
szager@chromium.org changed reviewers: + pdr@chromium.org
+pdr for Source/platform https://codereview.chromium.org/2383113003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp (right): https://codereview.chromium.org/2383113003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp:2824: // Note: this is just the scroll offset, *not* the "adjusted scroll offset". Scroll offset On 2016/10/01 20:32:47, bokan wrote: > This comment needs to be updated Done. https://codereview.chromium.org/2383113003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h (right): https://codereview.chromium.org/2383113003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:316: DoubleSize offsetFromOrigin() const { On 2016/10/01 20:32:47, bokan wrote: > I find this naming to be confusing, I think what this method is doing is > returning the distance of the ScrollableArea's top-left corner from the overflow > content's top-left corner. It took me a while to realize that "offsetFromOrigin" > isn't "offsetFrom*Scroll*Origin". > > Perhaps we should call it "offsetFromTopLeft"? Alternatively, leave it so that > the origin means the overflow content's top left corner and rename > scrollOrigin() to writingModeAdjustment() or something like that. Or am I > completely misunderstanding? In discussion with skobes, we had different ideas about how to interpret "origin": I imagined it as the top/left of the overflow rect, skobes imagines it as the top/left of the beginning of flow position. The only consensus, I think, is that "origin" is a universally confusing term. These names are temporary, the subsequent patch changes them to: "offset": Distance from beginning of flow "absolutePosition": Distance from top/left ... which avoids the term "origin". I can add documentation to that patch to clarify their meaning. Changing the names is part of what makes the subsequent patch so big, so in this patch, I just did s/adjustedScrollOffset/offsetFromOrigin/ because it was a relatively small change and I thought it added clarity. https://codereview.chromium.org/2383113003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.h:561: // This is the absolute scroll position, *not* relative to m_scrollOrigin. On 2016/10/01 20:32:47, bokan wrote: > Ok, I think I'm missing something. Absolute would mean relative to the top left? > This seems to be the opposite implied in the description: > > > 'position' is the offset of a scrolling viewport from the beginning of > > flow of its contents (e.g., for LTR contents it means scrolled all the > > way to the left, for RTL contents it means scrolled all the way to the > > right). > > Is this supposed to mean "position (0, 0) is scrolled all the way up and left in > LTR and down and right in RTL"? In that case, that does seem to be relative to > m_scrollOrigin. > > > 'offsetFromOrigin' is the position of the scrolling viewport relative > > to the scroll origin of the ScrollableArea. For a description of > > 'scroll origin' (with ascii art!) refer to core/layout/README.md. > > offsetFromOrigin() is defined as: > > return toDoubleSize(DoublePoint(scrollOrigin())) + m_scrollPosition; > > So (referring to the ASCII art in the README.md) if we're RTL and assume > scrollOrigin is (200, 0) and scrollPosition is (0, 0), offsetFromOrigin will be > (200, 0) which implies it's not relative to the origin but the top left. Yeah, this is a bad comment; I changed it. https://codereview.chromium.org/2383113003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp (right): https://codereview.chromium.org/2383113003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scroll/ScrollableArea.cpp:195: scrollAnimator().adjustAnimationAndSetScrollPosition(clampedPosition, On 2016/10/01 20:32:47, bokan wrote: > I think you can remove the clamps in > ScrollAnimator::adjustAnimationAndSetScrollPosition and > ScrollAnimatorCompositorCoordinator::adjustAnimationAndSetScrollPosition. Done.
I am totally confused by your change description, your definitions of 'position' and 'offsetFromOrigin' are identical to each other. Why is the switch-to-float patch changing the naming? Can we have one patch for the renaming and another patch for the switch-to-float? We definitely shouldn't introduce a new origin concept. Any references to "origin" should be the scroll origin as expressed by ScrollableArea::scrollOrigin(). In general I'd prefer shorter names, just "offset" and "position" (consistently defined) rather than offsetFromOrigin or absolutePosition.
On 2016/10/03 18:24:11, skobes wrote: > I am totally confused by your change description, your definitions of 'position' > and 'offsetFromOrigin' are identical to each other. You and I have discussed this before, and we have different mental models of 'origin'. In my mental model, the 'origin' is the top/left of the overflow region. In the subsequent patch, I use the names 'offset' to mean 'distance from beginning of flow', and 'position' to mean 'distance from top/left of overflow region'. > Why is the switch-to-float patch changing the naming? Can we have one patch for > the renaming and another patch for the switch-to-float? I don't want to have two patches that each touch 100+ files. In the follow-on patch, there is a correlation between naming and types: things named 'offset' represent a distance from beginning of flow, and they are of type ScrollOffset. Things named 'position' represent a point in the coordinate system of the overflow rect, and they are of type FloatPoint. > We definitely shouldn't introduce a new origin concept. Any references to > "origin" should be the scroll origin as expressed by > ScrollableArea::scrollOrigin(). As I said, I have a different mental model for this, based on reading the existing code. The current code uses the terms 'position', 'offset', and 'origin' very inconsistently, which is a big part of what I'm trying to fix. I think the end state represented by the second patch is much more consistent. Maybe the thing to do is to back out all of the renaming in this patch, so it's just a refactor of ScrollableArea::setScrollOffset. That will mean that the second patch is bigger. Aside from being a lot of work, I don't think that trying to split the renaming from the type conversions in the subsequent CL will be helpful. A patch with an intermediate state where things named 'offset' might be FloatSize and might be FloatPoint, and similarly for 'position', would be very confusing for me and for reviewers. > In general I'd prefer shorter names, just "offset" and "position" (consistently > defined) rather than offsetFromOrigin or absolutePosition. I opted for 'offset' and 'absolutePosition' in the subsequent patch, just because I wanted to be as explicit as possible, but I am open to changing that in the other patch.
On 2016/10/03 20:00:25, szager1 wrote: > You and I have discussed this before, and we have different mental models of > 'origin'. In my mental model, the 'origin' is the top/left of the overflow > region. In the subsequent patch, I use the names 'offset' to mean 'distance > from beginning of flow', and 'position' to mean 'distance from top/left of > overflow region'. That's fine, I think the change description just needs to be updated. > I don't want to have two patches that each touch 100+ files. In the follow-on > patch, there is a correlation between naming and types: things named 'offset' > represent a distance from beginning of flow, and they are of type ScrollOffset. > Things named 'position' represent a point in the coordinate system of the > overflow rect, and they are of type FloatPoint. Makes sense. As discussed offline I'd rather not change the names twice. If we can avoid that by putting all the renaming in the second patch then let's do that. > > We definitely shouldn't introduce a new origin concept. Any references to > > "origin" should be the scroll origin as expressed by > > ScrollableArea::scrollOrigin(). > > As I said, I have a different mental model for this, based on reading the > existing code. The current code uses the terms 'position', 'offset', and > 'origin' very inconsistently, which is a big part of what I'm trying to fix. It sounds like we're in agreement that "origin" should correspond to what ScrollableArea::scrollOrigin() returns, we just disagree about how to interpret that visually. Let's continue that discussion on the other patch.
The CQ bit was checked by szager@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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by szager@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...
Description was changed from ========== Refactor ScrollableArea::setScrollPosition. Previously, the terms 'position' and 'offset' were used inconsitently, particularly in PaintLayerScrollableArea. This change enforces consistent usage: 'position' is the offset of a scrolling viewport from the beginning of flow of its contents (e.g., for LTR contents it means scrolled all the way to the left, for RTL contents it means scrolled all the way to the right). 'offsetFromOrigin' is the position of the scrolling viewport relative to the scroll origin of the ScrollableArea. For a description of 'scroll origin' (with ascii art!) refer to core/layout/README.md. Other changes in this patch: - Eliminate some redundant PLSA methods and indirection. - setScrollPosition always clamps its argument. Code inspection revealed that nearly all callers were clamping the position argument anyway. - setScrollPosition returns immediately if the scroll position did not change. This avoids a bunch of unnecessary work. - For the rare instances where the scroll position should not be clamped, or the scroll position update code should run even if the scroll position didn't change, add the method setScrollPositionUnconditionally. - When modifying the scroll position due to a change in effective zoom, preserve the 'position' (i.e, distance from beginning of flow) rather that 'offsetFromOrigin' (i.e., the distance from top-left of overflow area). BUG=417782 R=skobes@chromium.org,bokan@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Refactor ScrollableArea::setScrollPosition. - Eliminate some redundant PaintLayerScrollableArea methods and indirection. - ScrollableArea::setScrollPosition now always clamps its argument. Code inspection revealed that nearly all callers were clamping the position argument anyway. - ScrollableArea::setScrollPosition returns immediately if the scroll position did not change. This avoids a bunch of unnecessary work. - For the rare instances where the scroll position should not be clamped, or the scroll position update code should run even if the scroll position didn't change, add the method setScrollPositionUnconditionally. - When modifying the scroll position due to a change in effective zoom, preserve the offset from beginning of flow, rather than the distance from top-left of overflow area. BUG=417782 R=skobes@chromium.org,bokan@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Please wrap the change description to 72 characters. This makes it more readable in various tools and punchcards. LGTM, but please wait for both skobes and bokan to give the go-ahead as well.
Description was changed from ========== Refactor ScrollableArea::setScrollPosition. - Eliminate some redundant PaintLayerScrollableArea methods and indirection. - ScrollableArea::setScrollPosition now always clamps its argument. Code inspection revealed that nearly all callers were clamping the position argument anyway. - ScrollableArea::setScrollPosition returns immediately if the scroll position did not change. This avoids a bunch of unnecessary work. - For the rare instances where the scroll position should not be clamped, or the scroll position update code should run even if the scroll position didn't change, add the method setScrollPositionUnconditionally. - When modifying the scroll position due to a change in effective zoom, preserve the offset from beginning of flow, rather than the distance from top-left of overflow area. BUG=417782 R=skobes@chromium.org,bokan@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Refactor ScrollableArea::setScrollPosition. - Eliminate some redundant PaintLayerScrollableArea methods and indirection. - ScrollableArea::setScrollPosition now always clamps its argument. Code inspection revealed that nearly all callers were clamping the position argument anyway. - ScrollableArea::setScrollPosition returns immediately if the scroll position did not change. This avoids a bunch of unnecessary work. - For the rare instances where the scroll position should not be clamped, or the scroll position update code should run even if the scroll position didn't change, add the method setScrollPositionUnconditionally. - When modifying the scroll position due to a change in effective zoom, preserve the offset from beginning of flow, rather than the distance from top-left of overflow area. BUG=417782 R=skobes@chromium.org,bokan@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== Refactor ScrollableArea::setScrollPosition. - Eliminate some redundant PaintLayerScrollableArea methods and indirection. - ScrollableArea::setScrollPosition now always clamps its argument. Code inspection revealed that nearly all callers were clamping the position argument anyway. - ScrollableArea::setScrollPosition returns immediately if the scroll position did not change. This avoids a bunch of unnecessary work. - For the rare instances where the scroll position should not be clamped, or the scroll position update code should run even if the scroll position didn't change, add the method setScrollPositionUnconditionally. - When modifying the scroll position due to a change in effective zoom, preserve the offset from beginning of flow, rather than the distance from top-left of overflow area. BUG=417782 R=skobes@chromium.org,bokan@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Refactor ScrollableArea::setScrollPosition. - Eliminate some redundant PaintLayerScrollableArea methods and indirection. - ScrollableArea::setScrollPosition now always clamps its argument. Code inspection revealed that nearly all callers were clamping the position argument anyway. - ScrollableArea::setScrollPosition returns immediately if the scroll position did not change. This avoids a bunch of unnecessary work. - For the rare instances where the scroll position should not be clamped, or the scroll position update code should run even if the scroll position didn't change, add the method setScrollPositionUnconditionally. - When modifying the scroll position due to a change in effective zoom, preserve the offset from beginning of flow, rather than the distance from top-left of overflow area. BUG=417782 R=skobes@chromium.org,bokan@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
OK, here is a minimal patch without any renaming, PTAL.
The CQ bit was checked by szager@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_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Just a nit and a question but otherwise lgtm https://codereview.chromium.org/2383113003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2383113003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:3565: void FrameView::setScrollOffset(const DoublePoint& position, This param renaming was probably meant for the second patch? https://codereview.chromium.org/2383113003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2383113003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:810: ScrollableArea::setScrollPosition(scrollPositionDouble(), Why not just ScrollableArea::setScrollPosition
lgtm w/ nits https://codereview.chromium.org/2383113003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/2383113003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:550: getScrollableArea()->scrollToOffset( nit: use local scrollableArea var https://codereview.chromium.org/2383113003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:562: getScrollableArea()->scrollToOffset( nit: use local scrollableArea var https://codereview.chromium.org/2383113003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/ScrollEnums.h (left): https://codereview.chromium.org/2383113003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/ScrollEnums.h:15: enum ScrollOffsetClamping { ScrollOffsetUnclamped, ScrollOffsetClamped }; I'm very happy to see this gone. :) https://codereview.chromium.org/2383113003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2383113003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:644: cancelScrollAnimation(); Is this what happened before? (cancelling the animation) https://codereview.chromium.org/2383113003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp (right): https://codereview.chromium.org/2383113003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp:193: IntSize actualAdjustment = Rename this to "adjustment" (the "actual" part makes less sense without the clamping).
https://codereview.chromium.org/2383113003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2383113003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:3565: void FrameView::setScrollOffset(const DoublePoint& position, On 2016/10/04 22:17:25, bokan wrote: > This param renaming was probably meant for the second patch? Indeed, fixed. https://codereview.chromium.org/2383113003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/LayoutBox.cpp (right): https://codereview.chromium.org/2383113003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:550: getScrollableArea()->scrollToOffset( On 2016/10/04 22:25:26, skobes wrote: > nit: use local scrollableArea var Done. https://codereview.chromium.org/2383113003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/LayoutBox.cpp:562: getScrollableArea()->scrollToOffset( On 2016/10/04 22:25:26, skobes wrote: > nit: use local scrollableArea var Done. https://codereview.chromium.org/2383113003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2383113003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:644: cancelScrollAnimation(); On 2016/10/04 22:25:26, skobes wrote: > Is this what happened before? (cancelling the animation) Yes, it's the same thing that ProgrammaticScrollAnimator::scrollToOffsetWithoutAnimation() does. https://codereview.chromium.org/2383113003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:810: ScrollableArea::setScrollPosition(scrollPositionDouble(), On 2016/10/04 22:17:25, bokan wrote: > Why not just ScrollableArea::setScrollPosition I don't understand the question; maybe you're asking why ScrollableArea::setScrollPosition() instead of just setScrollPosition()? If so, the answer is: PLSA::setScrollPosition takes an adjusted scroll position argument, SA::setScrollPosition takes an un-adjusted scroll position argument. Which is really just awful, hence my eagerness to rename these suckers. https://codereview.chromium.org/2383113003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp (right): https://codereview.chromium.org/2383113003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scroll/ScrollAnimator.cpp:193: IntSize actualAdjustment = On 2016/10/04 22:25:26, skobes wrote: > Rename this to "adjustment" (the "actual" part makes less sense without the > clamping). Done.
The CQ bit was checked by szager@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org, bokan@chromium.org, skobes@chromium.org Link to the patchset: https://codereview.chromium.org/2383113003/#ps100001 (title: "nits and rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2383113003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2383113003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:810: ScrollableArea::setScrollPosition(scrollPositionDouble(), On 2016/10/04 22:54:14, szager1 wrote: > On 2016/10/04 22:17:25, bokan wrote: > > Why not just ScrollableArea::setScrollPosition > > I don't understand the question; maybe you're asking why > ScrollableArea::setScrollPosition() instead of just setScrollPosition()? If so, > the answer is: PLSA::setScrollPosition takes an adjusted scroll position > argument, SA::setScrollPosition takes an un-adjusted scroll position argument. > > Which is really just awful, hence my eagerness to rename these suckers. What I meant was, since ScrollableArea::setScrollPosition clamps, why the setScrollPositionUnconditionally to the clamped position above? Couldn't we just remove the branch and call SA::SetScrollPosition(scrollPositionDouble()) in all cases?
https://codereview.chromium.org/2383113003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2383113003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:810: ScrollableArea::setScrollPosition(scrollPositionDouble(), On 2016/10/04 22:56:51, bokan wrote: > On 2016/10/04 22:54:14, szager1 wrote: > > On 2016/10/04 22:17:25, bokan wrote: > > > Why not just ScrollableArea::setScrollPosition > > > > I don't understand the question; maybe you're asking why > > ScrollableArea::setScrollPosition() instead of just setScrollPosition()? If > so, > > the answer is: PLSA::setScrollPosition takes an adjusted scroll position > > argument, SA::setScrollPosition takes an un-adjusted scroll position argument. > > > > Which is really just awful, hence my eagerness to rename these suckers. > > What I meant was, since ScrollableArea::setScrollPosition clamps, why the > setScrollPositionUnconditionally to the clamped position above? Couldn't we just > remove the branch and call SA::SetScrollPosition(scrollPositionDouble()) in all > cases? No, this is necessary for the case where the scroll position hasn't changed, but the scroll origin has changed. With this patch, ScrollableArea::setScrollPosition will early return if the scroll position hasn't changed, but when the scroll origin changes, we need all of the post-scroll-position-update code to run (i.e., everything in SA::scrollOffsetChanged(), which calls into PLSA::updateScrollOffset()).
https://codereview.chromium.org/2383113003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp (right): https://codereview.chromium.org/2383113003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerScrollableArea.cpp:810: ScrollableArea::setScrollPosition(scrollPositionDouble(), On 2016/10/04 23:37:37, szager1 wrote: > On 2016/10/04 22:56:51, bokan wrote: > > On 2016/10/04 22:54:14, szager1 wrote: > > > On 2016/10/04 22:17:25, bokan wrote: > > > > Why not just ScrollableArea::setScrollPosition > > > > > > I don't understand the question; maybe you're asking why > > > ScrollableArea::setScrollPosition() instead of just setScrollPosition()? If > > so, > > > the answer is: PLSA::setScrollPosition takes an adjusted scroll position > > > argument, SA::setScrollPosition takes an un-adjusted scroll position > argument. > > > > > > Which is really just awful, hence my eagerness to rename these suckers. > > > > What I meant was, since ScrollableArea::setScrollPosition clamps, why the > > setScrollPositionUnconditionally to the clamped position above? Couldn't we > just > > remove the branch and call SA::SetScrollPosition(scrollPositionDouble()) in > all > > cases? > > No, this is necessary for the case where the scroll position hasn't changed, but > the scroll origin has changed. With this patch, > ScrollableArea::setScrollPosition will early return if the scroll position > hasn't changed, but when the scroll origin changes, we need all of the > post-scroll-position-update code to run (i.e., everything in > SA::scrollOffsetChanged(), which calls into PLSA::updateScrollOffset()). Got it, thanks.
Message was sent while issue was closed.
Description was changed from ========== Refactor ScrollableArea::setScrollPosition. - Eliminate some redundant PaintLayerScrollableArea methods and indirection. - ScrollableArea::setScrollPosition now always clamps its argument. Code inspection revealed that nearly all callers were clamping the position argument anyway. - ScrollableArea::setScrollPosition returns immediately if the scroll position did not change. This avoids a bunch of unnecessary work. - For the rare instances where the scroll position should not be clamped, or the scroll position update code should run even if the scroll position didn't change, add the method setScrollPositionUnconditionally. - When modifying the scroll position due to a change in effective zoom, preserve the offset from beginning of flow, rather than the distance from top-left of overflow area. BUG=417782 R=skobes@chromium.org,bokan@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Refactor ScrollableArea::setScrollPosition. - Eliminate some redundant PaintLayerScrollableArea methods and indirection. - ScrollableArea::setScrollPosition now always clamps its argument. Code inspection revealed that nearly all callers were clamping the position argument anyway. - ScrollableArea::setScrollPosition returns immediately if the scroll position did not change. This avoids a bunch of unnecessary work. - For the rare instances where the scroll position should not be clamped, or the scroll position update code should run even if the scroll position didn't change, add the method setScrollPositionUnconditionally. - When modifying the scroll position due to a change in effective zoom, preserve the offset from beginning of flow, rather than the distance from top-left of overflow area. BUG=417782 R=skobes@chromium.org,bokan@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Refactor ScrollableArea::setScrollPosition. - Eliminate some redundant PaintLayerScrollableArea methods and indirection. - ScrollableArea::setScrollPosition now always clamps its argument. Code inspection revealed that nearly all callers were clamping the position argument anyway. - ScrollableArea::setScrollPosition returns immediately if the scroll position did not change. This avoids a bunch of unnecessary work. - For the rare instances where the scroll position should not be clamped, or the scroll position update code should run even if the scroll position didn't change, add the method setScrollPositionUnconditionally. - When modifying the scroll position due to a change in effective zoom, preserve the offset from beginning of flow, rather than the distance from top-left of overflow area. BUG=417782 R=skobes@chromium.org,bokan@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Refactor ScrollableArea::setScrollPosition. - Eliminate some redundant PaintLayerScrollableArea methods and indirection. - ScrollableArea::setScrollPosition now always clamps its argument. Code inspection revealed that nearly all callers were clamping the position argument anyway. - ScrollableArea::setScrollPosition returns immediately if the scroll position did not change. This avoids a bunch of unnecessary work. - For the rare instances where the scroll position should not be clamped, or the scroll position update code should run even if the scroll position didn't change, add the method setScrollPositionUnconditionally. - When modifying the scroll position due to a change in effective zoom, preserve the offset from beginning of flow, rather than the distance from top-left of overflow area. BUG=417782 R=skobes@chromium.org,bokan@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/cddc823c56d453758003a831fd5a2475e94c0387 Cr-Commit-Position: refs/heads/master@{#423033} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/cddc823c56d453758003a831fd5a2475e94c0387 Cr-Commit-Position: refs/heads/master@{#423033}
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/2387393003/ by imcheng@chromium.org. The reason for reverting is: Blocks revert of another patch: https://codereview.chromium.org/2395683002/. |
