[Webkit-unassigned] [Bug 135268] Compute and store snap point positions

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=135268





--- Comment #7 from Wenson Hsieh <wenson_hsieh at apple.com>  2014-08-10 13:17:55 PST ---
(From update of attachment 236310)
View in context: https://bugs.webkit.org/attachment.cgi?id=236310&action=review

Thanks for the feedback! I've restructured my code to avoid having a "ScrollSnapManager" altogether, mainly by separating snap position storage from snap animation into classes that are less vague than "Manager".

>> Source/JavaScriptCore/ChangeLog:6
>> +        Enable CSS_SCROLL_SNAP on iPhoneOS and simulator.
> 
> It seems odd to do this in the same commit.

Removed.

>> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:68
>> +LayoutUnit SnapPositionList::closestSnapPosition(LayoutUnit scrollDestination, float velocity) const
> 
> This could either be a static function or a function on ScrollSnap"Manager"?

I'll change this to a static function. Good call.

>> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:74
>> +        return positions.at(positions.size() - 1);
> 
> Vector has last(). You'd better check that size() > 0.

Fixed to use last(), and added early return for empty vector.

>> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:80
>> +        middleIndex = (lowerIndex + upperIndex) / 2;
> 
> middleIndex should be declared here.

Fixed.

>> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:89
>> +        }
> 
> Maybe std::binary_search?

I'll update the implementation to use std's binary search.

>> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:96
>> +    // If velocity is zero, then the user released without flicking, so we simply snap to the closer point.
> 
> I don't think this comment is useful.

Removed.

>> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:97
>> +    return scrollDestination - lowerSnapPosition <= upperSnapPosition - scrollDestination ? lowerSnapPosition : upperSnapPosition;
> 
> Break out "scrollDestination - lowerSnapPosition <= upperSnapPosition - scrollDestination" into a named variable so I can tell what it is.

Fixed. It indicates whether or not the scroll destination is closer to the lower snap point.

>> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:100
>> +ScrollSnapManager::ScrollSnapManager(ScrollEventAxis axis)
> 
> Maybe the name should reflect that this "manager" only handles a single axis.

Fixed -- does SingleAxisSnapList sound better? I'm also separating animation code from the SingleAxisSnapList.

>> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:148
>> +}
> 
> The point of ScrollableArea is to abstract out differences between different scrollable things, so you should re-write this in terms of ScrollableArea, and not have different function for FrameView and overflow.

Moved the bulk of the logic over to ScrollableArea, which is shared by FrameView and RenderLayer.

>> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:160
>> +    while (child && lastPotentialSnapPosition < maxScrollOffset) {
> 
> This walking of the nodes under the "elements" scroller could be very expensive. I think it would be better to somehow register RenderObjects with scrollSnapCoordinates() with their enclosing scroller.

Agreed. If it's alright, I think I'll keep it like this for this patch, and it can be an optimization later on.

>> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:164
>> +            LayoutUnit childOffset = (m_axis == ScrollEventAxis::Horizontal ? childBox->offsetLeft() : childBox->offsetTop()) - parentOffset;
> 
> This should use localToContainerPoint() or similar. You should test content with CSS transforms between the scrolling thing and the target elements.

Good point -- I'll fix this.

>> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:178
>> +    // Always put a snap point on the maximum scroll offset.
> 
> Is that behavior defined in the spec?

Added a comment indicating that it's something I added to prevent unreachable content.

>> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:183
>> +void ScrollSnapManager::updateSnapDataFromStyle(const RenderStyle& style, LayoutUnit size)
> 
> It's unclear what the "size" param refers to.

Changed to viewSize.

>> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:202
>> +    LayoutUnit curSnapPositionShift = -destinationOffset;
> 
> Not clear why.

Agreed, looks kind of weird. Changed implementation to not use -destinationOffset.

>> Source/WebCore/page/scrolling/ScrollSnapManager.h:58
>> +class ScrollSnapManager {
> 
> http://blog.codinghorror.com/i-shall-call-it-somethingmanager/

This was enlightening :) The ScrollSnapManager was originally intended to handle storing snap positions, and handling snapping animations/determining where to snap. I plan on separating the snap point data from the snap animation code, so the former will be in SingleAxisSnapList and the latter in something like ScrollSnapAnimator (hopefully "Animator" is a lot less nebulous than "Manager").

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.