[webkit-reviews] review granted: [Bug 135769] Implement snapping behavior for iOS : [Attachment 236651] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 135769: Implement snapping behavior for iOS
https://bugs.webkit.org/show_bug.cgi?id=135769

Attachment 236651: Patch
https://bugs.webkit.org/attachment.cgi?id=236651&action=review

------- Additional Comments from Brent Fulgham <bfulgham at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=236651&action=review


r=me. Please file a bug to implement layout tests for this (as we discussed in
person).

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:64
> +    // FIXME: Incorporate current page scale factor in snapping to device
pixel. Perhaps we should just convert to float here and let UI process do the
pixel snapping?

Pet peeve: FIXME should have a bug # in it so we don't forget to fix!

> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:67
> +    for (size_t i = 0; i < snapOffsets.size(); ++i)

How about a C++11 iterator here?

> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:47
> +	       // FIXME: Investigate why using localToContainerPoint gives the
wrong offsets for iOS mainframe. Also, these offsets won't take transforms into
account (make sure to test this!)

Bug # please

> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:51
> +	       // FIXME: Check that localToContainerPoint works with CSS
rotations.

Ditto.

>>
Source/WebKit2/UIProcess/Scrolling/ios/ScrollingTreeOverflowScrollingNodeIOS.mm
:209
>> +		scrollView.decelerationRate = horizontalSnapOffsets().size() ||
verticalSnapOffsets().size() ? UIScrollViewDecelerationRateFast :
UIScrollViewDecelerationRateNormal;
> 
> (oops, I meant to change size() here to !isEmpty() as well)

Please fix!

> Source/WebKit2/UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm:123
> +    // FIXME: We need to account for how the top navigation bar changes in
size.

This seems like an important bug to file so we don't forget!