[Webkit-unassigned] [Bug 135769] Implement snapping behavior for iOS

[bugzilla-daemon at webkit.org]

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





--- Comment #13 from Wenson Hsieh <wenson_hsieh at apple.com>  2014-08-15 09:30:28 PST ---
(From update of attachment 236637)
View in context: https://bugs.webkit.org/attachment.cgi?id=236637&action=review

>> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:64
>> +    ASSERT(node);
> 
> Node should be passed as a reference instead of asserting the Node is non null.
> 
> It is odd the output parameter is the second one, shouldn't it be the first?

Fixed. Thanks!

>> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:66
>> +    snapOffsetsAsFloat.reserveCapacity(snapOffsets.size());
> 
> reserveCapacity -> reserveInitialCapacity

Fixed.

>> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:117
>> +        setStateScrollingNodeSnapOffsetsAsFloat(ScrollEventAxis::Horizontal, node, *frameView->horizontalSnapOffsets(), frameView->frame().document()->deviceScaleFactor());
> 
> if (const Vector<LayoutUnit>* horizontalSnapOffsets = frameView->horizontalSnapOffsets())
>    setXXX(..., horizontalSnapOffsets, ...)

Fixed. Looks much cleaner!

>> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.h:46
>> +T closestSnapOffset(const Vector<T>& snapOffsets, T scrollDestination, float velocity)
> 
> This code is used with floats and doubles for the velocity. You should templatize "velocity" (can be implicit).
> 
> The value of scrollDestination can also be a CGFloat.
> 
> I don't get the template here really.

Added comments explaining the template, as well as distinguished LayoutType from VelocityType.

>> Source/WebCore/rendering/RenderLayerCompositor.cpp:3776
>> +            if (auto* offsets = layer.verticalSnapOffsets())
> 
> I disagree with auto here, the type is not obvious.

Changed back to Vector<LayoutUnit>

>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1305
>> +        CGSize maxScrollDimensions = CGSizeMake(scrollView.contentSize.width - scrollView.bounds.size.width, scrollView.contentSize.height - scrollView.bounds.size.height);
> 
> This looks very weird, can you explain what you are trying to do?
> 
> Don't forget WKWebView has two additional kind of insets.

Added a comment describing the role of maxScrollDimensions. We'll probably have to change it to account for the insets.

>> Source/WebKit2/UIProcess/Scrolling/ios/ScrollingTreeOverflowScrollingNodeIOS.mm:83
>> +    if (_scrollingTreeNode->horizontalSnapOffsets().size() > 0)
> 
> size() > 0 -> isEmpty().

Changed. Thanks!

>> Source/WebKit2/UIProcess/Scrolling/ios/ScrollingTreeOverflowScrollingNodeIOS.mm:85
>> +    if (_scrollingTreeNode->verticalSnapOffsets().size() > 0)
> 
> ditto.

Changed.

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