Bug 135769

Created attachment 236563 [details] Patch

Comment on attachment 236563 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236563&action=review Looks good other than the encoding/decoding. > Source/WebCore/page/scrolling/ScrollingTreeScrollingNode.h:62 > + Vector<LayoutUnit> horizontalSnapOffsets() const { return m_horizontalSnapOffsets; } > + Vector<LayoutUnit> verticalSnapOffsets() const { return m_verticalSnapOffsets; } Please return a const reference. > Source/WebCore/rendering/RenderLayer.cpp:3041 > + Whitespace. > Source/WebCore/rendering/RenderLayer.cpp:3316 > +#if ENABLE(CSS_SCROLL_SNAP) > + // FIXME: Ensure that offsets are also updated in case of programmatic style changes. > + updateSnapOffsets(); > +#endif Shouldn't this be in an earlier patch? > Source/WebKit2/Shared/Scrolling/RemoteScrollingCoordinatorTransaction.cpp:135 > + encoder << static_cast<uint64_t>(node.horizontalSnapOffsets().size()); > + for (LayoutUnit position : node.horizontalSnapOffsets()) > + encoder << position.toDouble(); Can't you just encode the Vector directly? > Source/WebKit2/Shared/Scrolling/RemoteScrollingCoordinatorTransaction.cpp:222 > + if (node.hasChangedProperty(ScrollingStateScrollingNode::HorizontalSnapOffsets)) { > + if (!decoder.decode(size)) > + return false; > + > + Vector<LayoutUnit> horizontalSnapOffsets; > + for (size_t i = 0; i < size; i++) { > + double position = 0; > + if (!decoder.decode(position)) > + return false; > + > + horizontalSnapOffsets.append(position); > + } > + node.setHorizontalSnapOffsets(horizontalSnapOffsets); Similarly, I think the vector should just be decodable directly.

Zalan is noting that you should avoid encoding LayoutUnits and instead encode pixel-snapped floats, and avoid LayoutUnits in the UI process at all.

Created attachment 236591 [details] Patch

Comment on attachment 236591 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236591&action=review > Source/WebCore/page/scrolling/AxisScrollSnapOffsets.h:46 > +T closestSnapOffset(const Vector<T>& snapOffsets, T scrollDestination, float velocity) This function is an updated version of the one found in the Mac animations patch. > Source/WebKit2/UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm:112 > +bool RemoteScrollingCoordinatorProxy::shouldHorizontalSnapForMainframeScrolling() const To prevent requiring ScrollTypes, I split these methods into horizontal and vertical versions. Otherwise, I could pass in a WebCore::ScrollEventAxis

Comment on attachment 236591 [details] Patch This looks ok to me. Let's see what Tim says.

Comment on attachment 236591 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236591&action=review > Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:106 > + // FIXME: Make sure we snap these pixel values. Why not do this now? I'm sure Zalan can tell you how, and I think it's not too hard. > Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:108 > + snapOffsetsAsFloat.append(frameView->horizontalSnapOffsets()->at(i).round()); you're .at()ing again > Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:112 > + if (frameView->verticalSnapOffsets()) { perhaps a reusable static snapOffsetsAsFloats or something? this code is here four times > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:86 > +#if ENABLE(CSS_SCROLL_SNAP) This doesn't belong in the middle of the big block if it has #if, but I don't think it needs the #if, either. > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1306 > + if (_page->mainFrame() && _page->mainFrame()->page() && _page->mainFrame()->page()->scrollingCoordinatorProxy()) { I wonder if this code can go somewhere more scrollingy (RemoteScrollingCoordinatorProxy?) and we can just call something->adjustTargetContentOffsetForSnapping(targetContentOffset) or something. > Source/WebKit2/UIProcess/Scrolling/ios/ScrollingTreeOverflowScrollingNodeIOS.mm:33 > +#if ENABLE(CSS_SCROLL_SNAP) no #if blocks inside the big block of #imports! > Source/WebKit2/UIProcess/Scrolling/ios/ScrollingTreeOverflowScrollingNodeIOS.mm:206 > + // FIXME: In only one axis snaps in 2D scrolling, the other axis will decelerate fast as well. Is this what we want? there's some mixup at the beginning of this sentence > Source/WebKit2/UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm:40 > +#include <WebCore/ScrollingTreeFrameScrollingNode.h> this is not sorted right >> Source/WebKit2/UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm:112 >> +bool RemoteScrollingCoordinatorProxy::shouldHorizontalSnapForMainframeScrolling() const > > To prevent requiring ScrollTypes, I split these methods into horizontal and vertical versions. Otherwise, I could pass in a WebCore::ScrollEventAxis Deduplicate! > Source/WebKit2/UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm:143 > +float RemoteScrollingCoordinatorProxy::closestVerticalSnapOffsetForMainframeScrolling(float scrollDestination, float velocity) const deduplicate!

Created attachment 236615 [details] Patch

Comment on attachment 236591 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236591&action=review >> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:106 >> + // FIXME: Make sure we snap these pixel values. > > Why not do this now? I'm sure Zalan can tell you how, and I think it's not too hard. Fixed. >> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:108 >> + snapOffsetsAsFloat.append(frameView->horizontalSnapOffsets()->at(i).round()); > > you're .at()ing again Good catch. Fixed. >> Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp:112 >> + if (frameView->verticalSnapOffsets()) { > > perhaps a reusable static snapOffsetsAsFloats or something? this code is here four times So much cleaner :) >> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:86 >> +#if ENABLE(CSS_SCROLL_SNAP) > > This doesn't belong in the middle of the big block if it has #if, but I don't think it needs the #if, either. Got it. Removed #if. >> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1306 >> + if (_page->mainFrame() && _page->mainFrame()->page() && _page->mainFrame()->page()->scrollingCoordinatorProxy()) { > > I wonder if this code can go somewhere more scrollingy (RemoteScrollingCoordinatorProxy?) and we can just call something->adjustTargetContentOffsetForSnapping(targetContentOffset) or something. Added adjustTargetContentOffsetForSnapping to RemoteScrollingCoordinatorProxy. >> Source/WebKit2/UIProcess/Scrolling/ios/ScrollingTreeOverflowScrollingNodeIOS.mm:33 >> +#if ENABLE(CSS_SCROLL_SNAP) > > no #if blocks inside the big block of #imports! Got it -- fixed. >> Source/WebKit2/UIProcess/Scrolling/ios/ScrollingTreeOverflowScrollingNodeIOS.mm:206 >> + // FIXME: In only one axis snaps in 2D scrolling, the other axis will decelerate fast as well. Is this what we want? > > there's some mixup at the beginning of this sentence Oops, good catch! (it's supposed to be "if") >>> Source/WebKit2/UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm:112 >>> +bool RemoteScrollingCoordinatorProxy::shouldHorizontalSnapForMainframeScrolling() const >> >> To prevent requiring ScrollTypes, I split these methods into horizontal and vertical versions. Otherwise, I could pass in a WebCore::ScrollEventAxis > > Deduplicate! Re-merged into 1 method. >> Source/WebKit2/UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm:143 >> +float RemoteScrollingCoordinatorProxy::closestVerticalSnapOffsetForMainframeScrolling(float scrollDestination, float velocity) const > > deduplicate! Fixed.

Comment on attachment 236615 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236615&action=review > Source/WebCore/rendering/RenderLayer.cpp:3314 > + // FIXME: Ensure that offsets are also updated in case of programmatic style changes. Is there a bugzilla bug for this? If so, put the number here. > Source/WebCore/rendering/RenderLayerCompositor.cpp:3774 > + if (layer.horizontalSnapOffsets()) if (auto& offsets = layer.horizontalSnapOffsets()) scrollingGeometry.horizontalSnapOffsets = offsets; maybe? > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1304 > + if (_page->mainFrame() && _page->mainFrame()->page() && _page->mainFrame()->page()->scrollingCoordinatorProxy()) { All of this incrementally grabbing optional things is an unusual style for us. Usually assignment to a local and then early returning if null is more common, I think. > Source/WebKit2/UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm:41 > +#include <WebCore/AxisScrollSnapOffsets.h> these should all be #import :| including the ones above :| > Source/WebKit2/UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm:119 > + float potentialSnapPosition = closestSnapOffsetForMainframeScrolling(WebCore::ScrollEventAxis::Horizontal, targetContentOffset->x, velocity.x); the f in Mainframe should be capitalized

Created attachment 236637 [details] Patch

Comment on attachment 236637 [details] Patch 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.

Comment on attachment 236637 [details] Patch 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.

Created attachment 236651 [details] Patch

Comment on attachment 236651 [details] Patch This patch is covering a lot of different areas, and I think it's a good idea to break it down into several small patches. I'll be leaving comments to what parts should be separated out into individual patches.

Comment on attachment 236651 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236651&action=review > Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:-49 > - LayoutUnit lastPotentialSnapPositionX = LayoutUnit(position.x()) + valueForLength(coordinate.first, viewWidth); This should be in a separate patch. It seems localToContainerPoint was giving me strange offsets for child elements, so I resorted to using offset(Top|Left) on the child element instead, with a FIXME indicating that using offset(Left|Top) wouldn't take into account transforms. > Source/WebCore/page/scrolling/AxisScrollSnapOffsets.h:44 > +// closestSnapOffset is a templated function that takes in a Vector representing snap offsets as LayoutTypes (e.g. LayoutUnit or float) and These fixes to closestSnapOffset add a template for velocity as well as snapOffset. > 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) > Source/WebKit2/UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm:-26 > -#include "config.h" As Tim mentioned, these should have been using #import all along instead of #include. > Source/WebKit2/UIProcess/ios/RemoteScrollingCoordinatorProxyIOS.mm:-32 > -#include "LayerRepresentation.h" (and these too: we should just have a patch that changes these to use #import)

Comment on attachment 236651 [details] Patch 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!

Comment on attachment 236651 [details] Patch Clearing flags on attachment: 236651 Committed r172649: <http://trac.webkit.org/changeset/172649>

All reviewed patches have been landed. Closing bug.