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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Aug 15 13:51:17 PDT 2014

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

Attachment 236651: Patch

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

> 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


>> +		scrollView.decelerationRate = horizontalSnapOffsets().size() ||
verticalSnapOffsets().size() ? UIScrollViewDecelerationRateFast :
> (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

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

More information about the webkit-reviews mailing list