Bug 142590

<rdar://problem/20125088>

Created attachment 252758 [details] Resizing example The attached file is a manual test for scroll snapping on resizable regions.

Created attachment 252762 [details] Patch

Comment on attachment 252762 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252762&action=review > Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:570 > + if (!m_page) > + return; Surprised that check is here, and not in frameViewForScrollingNode. > Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:576 > + FrameView* frameViewPtr = frameViewForScrollingNode(scrollingNodeID); > + if (!frameViewPtr) > + return; > + > + FrameView& frameView = *frameViewPtr; I admire your desire to use a reference rather than a pointer, but normally we just use pointers in cases like this. > Source/WebCore/page/scrolling/AsyncScrollingCoordinator.h:60 > + void updateScrollSnapOffsetIndices(ScrollingNodeID, size_t horizontalIndex, size_t verticalIndex); Does this really need to be size_t? I’m concerned that we are using 64-bit without really knowing why when the engine normally uses 32-bit. > Source/WebCore/platform/ScrollView.cpp:1526 > + IntPoint currentPosition = scrollPosition(); No need to put this into a local variable just to use it once. In many cases it seems that setScrollPosition does an early exit if it’s not changing. Do we really need to do an additional early exit here? > Source/WebCore/platform/ScrollView.h:391 > + void scrollToNearestActiveSnapPoint() override; Can we also mark this final? > Source/WebCore/platform/ScrollableArea.cpp:196 > + if (scrollAnimator().activeScrollSnapOffsetIndexDidChange()) { > + setCurrentHorizontalSnapPointIndex(scrollAnimator().activeScrollSnapOffsetIndexForAxis(ScrollEventAxis::Horizontal)); > + setCurrentVerticalSnapPointIndex(scrollAnimator().activeScrollSnapOffsetIndexForAxis(ScrollEventAxis::Vertical)); iOS build failures: ScrollableArea.cpp:194:26: error: no member named 'activeScrollSnapOffsetIndexDidChange' in 'WebCore::ScrollAnimator' ScrollableArea.cpp:195:61: error: no member named 'activeScrollSnapOffsetIndexForAxis' in 'WebCore::ScrollAnimator' ScrollableArea.cpp:196:59: error: no member named 'activeScrollSnapOffsetIndexForAxis' in 'WebCore::ScrollAnimator' > Source/WebCore/platform/ScrollableArea.cpp:435 > + ScrollAnimator* scrollAnimator = existingScrollAnimator(); > + if (!scrollAnimator) No need for a local variable for the scrollAnimator, which is never used below. > Source/WebCore/rendering/RenderLayer.cpp:3214 > + scrollToOffset(IntSize(correctedPosition.x(), correctedPosition.y())); Why on earth does scrollToOffset take an IntSize rather than an IntPoint given that scrollPosition returns an IntPoint? Please use toIntSize instead of writing out the x()/y() thing.

Created attachment 252864 [details] Patch v2 (Updated with Darin's comments)

Comment on attachment 252864 [details] Patch v2 (Updated with Darin's comments) Uploading a new patch to confirm iOS builds properly.

Created attachment 252870 [details] Patch v3 (Forgot to include <wtf/MathExtras.h>

Comment on attachment 252870 [details] Patch v3 (Forgot to include <wtf/MathExtras.h> Build fix due to missing header.

Created attachment 252873 [details] Patch v4 (Correct iOS build failure)

Comment on attachment 252873 [details] Patch v4 (Correct iOS build failure) View in context: https://bugs.webkit.org/attachment.cgi?id=252873&action=review > Source/WebCore/page/FrameView.cpp:3005 > + if (ScrollAnimator* scrollAnimator = existingScrollAnimator()) > + scrollAnimator->updateScrollAnimatorsAndTimers(); Not this patch, but updateScrollAnimatorsAndTimers is a bad name for a public member function. Why is it the business of the caller to know that scrollAnimator is maintaining timers? > Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:565 > +void AsyncScrollingCoordinator::updateScrollSnapOffsetIndices(ScrollingNodeID scrollingNodeID, unsigned horizontalIndex, unsigned verticalIndex) How do you express "one active snap point, at 0"? Or is that not allowed? > Source/WebCore/page/scrolling/AsyncScrollingCoordinator.h:60 > + void updateScrollSnapOffsetIndices(ScrollingNodeID, unsigned horizontalIndex, unsigned verticalIndex); "update" is a bit vague here. Does it mean "scroll to these indices", or "these are the currently active indices"? > Source/WebCore/page/scrolling/AxisScrollSnapOffsets.h:77 > + activeSnapIndex = (velocity < 0) ? clampTo<unsigned>(lowerIndex) : clampTo<unsigned>(upperIndex); Why the clampTo<unsigned>(lowerIndex)? All this clamping seems to imply that you're uneasy with the unsigned math. > Source/WebCore/page/scrolling/ThreadedScrollingTree.h:65 > + void updateScrollSnapOffsetIndices(ScrollingNodeID, unsigned horizontalIndex, unsigned verticalIndex) override; "update" again. > Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:190 > + m_scrollController.setScrollSnapOffsetIndexDidChange(false); This is a bit confusing. Why do you have to set that it didn't change? > Source/WebCore/platform/ScrollAnimator.h:134 > + bool activeScrollSnapOffsetIndexDidChange() const; > + unsigned activeScrollSnapOffsetIndexForAxis(ScrollEventAxis) const; "offset index": need both? Just scrollSnapIndex? > Source/WebCore/platform/ScrollView.cpp:1531 > +void ScrollView::scrollToNearestActiveSnapPoint() > +{ > + IntPoint currentPosition = scrollPosition(); > + IntPoint correctedPosition = nearestActiveSnapPoint(currentPosition); > + > + if (correctedPosition != currentPosition) > + setScrollPosition(correctedPosition); > +} Can this go down to ScrollableArea? > Source/WebCore/platform/ScrollableArea.cpp:194 > + if (scrollAnimator().activeScrollSnapOffsetIndexDidChange()) { A "didChange" is usually a callback-type thing. Here's querying state, but from when? > Source/WebCore/platform/ScrollableArea.h:74 > + IntPoint nearestActiveSnapPoint(const IntPoint&); > + virtual void scrollToNearestActiveSnapPoint() { } How do you tell the difference between an index not being set, and there being a single valid index? > Source/WebCore/rendering/RenderLayer.cpp:3215 > +void RenderLayer::scrollToNearestActiveSnapPoint() > +{ > + IntPoint currentPosition = scrollPosition(); > + IntPoint correctedPosition = nearestActiveSnapPoint(currentPosition); > + > + if (correctedPosition != currentPosition) > + scrollToOffset(toIntSize(correctedPosition)); > +} Hopefully moving that ScrollVIew code into ScrollableArea would obviate this code. > Source/WebCore/rendering/RenderLayer.cpp:3507 > if (ScrollAnimator* scrollAnimator = existingScrollAnimator()) > - return scrollAnimator->updateScrollAnimatorsAndTimers(); > + scrollAnimator->updateScrollAnimatorsAndTimers(); > + scrollToNearestActiveSnapPoint(); Perhaps this chunk can move down to ScrollableArea too.

Comment on attachment 252873 [details] Patch v4 (Correct iOS build failure) View in context: https://bugs.webkit.org/attachment.cgi?id=252873&action=review >> Source/WebCore/page/FrameView.cpp:3005 >> + scrollAnimator->updateScrollAnimatorsAndTimers(); > > Not this patch, but updateScrollAnimatorsAndTimers is a bad name for a public member function. Why is it the business of the caller to know that scrollAnimator is maintaining timers? I'll make a note for future correction. Maybe something like 'updateScrollSnapState'... >> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:565 >> +void AsyncScrollingCoordinator::updateScrollSnapOffsetIndices(ScrollingNodeID scrollingNodeID, unsigned horizontalIndex, unsigned verticalIndex) > > How do you express "one active snap point, at 0"? Or is that not allowed? This interface is just to let the scrolling thread notify the main thread about which snap offset index is currently active. I didn't include the concept of "none" because the state of 'no active scroll snap offsets' is the case where we have no offsets at all. >> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.h:60 >> + void updateScrollSnapOffsetIndices(ScrollingNodeID, unsigned horizontalIndex, unsigned verticalIndex); > > "update" is a bit vague here. Does it mean "scroll to these indices", or "these are the currently active indices"? It means "we have new currently active indices". So maybe call it "setActiveScrollSnapOffsetIndices"? >> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.h:77 >> + activeSnapIndex = (velocity < 0) ? clampTo<unsigned>(lowerIndex) : clampTo<unsigned>(upperIndex); > > Why the clampTo<unsigned>(lowerIndex)? All this clamping seems to imply that you're uneasy with the unsigned math. I thought clang would complain about assigning from size_t to unsigned, so I added some preemptive clamping. But it actually looks like things work fine without it, so I'll get rid of this (and the <wtf/MathExtras.h> include.) >> Source/WebCore/page/scrolling/ThreadedScrollingTree.h:65 >> + void updateScrollSnapOffsetIndices(ScrollingNodeID, unsigned horizontalIndex, unsigned verticalIndex) override; > > "update" again. Fixed. >> Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:190 >> + m_scrollController.setScrollSnapOffsetIndexDidChange(false); > > This is a bit confusing. Why do you have to set that it didn't change? I thought I might save a cross-thread message send if multiple 'handleWheelEvents' were processed before the ScrollController updated its state. But it's probably not worth worry about. I'll get rid of it.n >> Source/WebCore/platform/ScrollAnimator.h:134 >> + unsigned activeScrollSnapOffsetIndexForAxis(ScrollEventAxis) const; > > "offset index": need both? Just scrollSnapIndex? Sure. 'scrollSnapIndex' reads better. >> Source/WebCore/platform/ScrollView.cpp:1531 >> +} > > Can this go down to ScrollableArea? No, because 'setScrollPosition' is not part of ScrollableArea's API. We use 'setScrollPosition' in ScrollView, and 'scrollToOffset' in RenderLayer. It seems like ScrollableArea should have some unified concept for this, but the current design doesn't support it. >> Source/WebCore/platform/ScrollableArea.cpp:194 >> + if (scrollAnimator().activeScrollSnapOffsetIndexDidChange()) { > > A "didChange" is usually a callback-type thing. Here's querying state, but from when? It's checking state after the wheel event handling code. I could pass a boolean into 'handleWheelEvent' to track this state, if that would be cleaner? >> Source/WebCore/platform/ScrollableArea.h:74 >> + virtual void scrollToNearestActiveSnapPoint() { } > > How do you tell the difference between an index not being set, and there being a single valid index? When we have scroll snap points, there is always a current active snap point offset. The only time we don't have a valid index set is when there is no set of snap points (the vector of snap point offsets is empty). >> Source/WebCore/rendering/RenderLayer.cpp:3215 >> +} > > Hopefully moving that ScrollVIew code into ScrollableArea would obviate this code. I can't do that easily in this patch, because RenderLayer and ScrollView have different API for setting scroll position. It seems like ScrollalbeArea should provide a unified concept for this, but I think that's a larger change than I should do as part of this patch. >> Source/WebCore/rendering/RenderLayer.cpp:3507 >> + scrollToNearestActiveSnapPoint(); > > Perhaps this chunk can move down to ScrollableArea too. I think so. Let me try.

Comment on attachment 252873 [details] Patch v4 (Correct iOS build failure) View in context: https://bugs.webkit.org/attachment.cgi?id=252873&action=review >>> Source/WebCore/platform/ScrollView.cpp:1531 >>> +} >> >> Can this go down to ScrollableArea? > > No, because 'setScrollPosition' is not part of ScrollableArea's API. We use 'setScrollPosition' in ScrollView, and 'scrollToOffset' in RenderLayer. It seems like ScrollableArea should have some unified concept for this, but the current design doesn't support it. Does scrollToOffsetWithoutAnimation() work?

Created attachment 252912 [details] Patch

Comment on attachment 252873 [details] Patch v4 (Correct iOS build failure) View in context: https://bugs.webkit.org/attachment.cgi?id=252873&action=review >>>> Source/WebCore/platform/ScrollView.cpp:1531 >>>> +} >>> >>> Can this go down to ScrollableArea? >> >> No, because 'setScrollPosition' is not part of ScrollableArea's API. We use 'setScrollPosition' in ScrollView, and 'scrollToOffset' in RenderLayer. It seems like ScrollableArea should have some unified concept for this, but the current design doesn't support it. > > Does scrollToOffsetWithoutAnimation() work? Oh! Let me try right now...

(In reply to comment #14) > Comment on attachment 252873 [details] > > > > Does scrollToOffsetWithoutAnimation() work? > > Oh! Let me try right now... Yes -- that worked very well. I've revised things to match.

Committed r184139: <http://trac.webkit.org/changeset/184139>

Revised patch approved by Simon Fraser in person.

Removing r? from patch