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

[bugzilla-daemon at webkit.org]

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





--- Comment #12 from Benjamin Poulain <benjamin at webkit.org>  2014-08-14 17:33:12 PST ---
(From update of attachment 236637)
View in context: https://bugs.webkit.org/attachment.cgi?id=236637&action=review

Some minor comments. I haven't looked into correctness at all yet.

> Source/WebCore/WebCore.exp.in:3014
> +#if ENABLE(CSS_SCROLL_SNAP)

That's not a great flag name :(

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:64
> +static inline void setStateScrollingNodeSnapOffsetsAsFloat(ScrollEventAxis axis, ScrollingStateScrollingNode* node, const Vector<LayoutUnit>& snapOffsets, float deviceScaleFactor)
> +{
> +    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?

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

reserveCapacity -> reserveInitialCapacity

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:68
> +    for (size_t i = 0; i < snapOffsets.size(); i++)
> +        snapOffsetsAsFloat.append(roundToDevicePixel(snapOffsets[i], deviceScaleFactor, false));

i++ -> ++i

This does not seem right. How can you round to device pixel without the current page scale factor?
Shouldn't you convert to float and let the UIProces do the rounding?

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

if (const Vector<LayoutUnit>* horizontalSnapOffsets = frameView->horizontalSnapOffsets())
   setXXX(..., horizontalSnapOffsets, ...)

> 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.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:3776
> +            if (auto* offsets = layer.horizontalSnapOffsets())
> +                scrollingGeometry.horizontalSnapOffsets = *offsets;
> +            if (auto* offsets = layer.verticalSnapOffsets())

I disagree with auto here, the type is not obvious.

> 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.

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

size() > 0 -> isEmpty().

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

ditto.

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