Bug 135268

Created attachment 236164 [details] Patch

Created attachment 236234 [details] Patch

Created attachment 236307 [details] Patch

Created attachment 236310 [details] Patch

Comment on attachment 236310 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236310&action=review > Source/JavaScriptCore/ChangeLog:6 > + Enable CSS_SCROLL_SNAP on iPhoneOS and simulator. It seems odd to do this in the same commit. > Source/WebCore/page/scrolling/ScrollSnapManager.cpp:41 > +SnapPositionList::SnapPositionList() > +{ > +} Can probably remove this constructor > Source/WebCore/page/scrolling/ScrollSnapManager.cpp:43 > +SnapPositionList::SnapPositionList(Vector<LayoutUnit> snapPositions) You're copying a Vector by value here. > Source/WebCore/page/scrolling/ScrollSnapManager.cpp:51 > +size_t SnapPositionList::size() const > +{ > + return positions.size(); > +} Should be inline > Source/WebCore/page/scrolling/ScrollSnapManager.cpp:66 > +void SnapPositionList::clear() > +{ > + positions.clear(); > +} > + > +void SnapPositionList::resize(size_t size) > +{ > + positions.resize(size); > +} > + > +void SnapPositionList::append(LayoutUnit position) > +{ > + positions.append(position); > +} This is just exposing Vector functionality. Maybe you don't need SnapPositionList at all? > Source/WebCore/page/scrolling/ScrollSnapManager.cpp:68 > +LayoutUnit SnapPositionList::closestSnapPosition(LayoutUnit scrollDestination, float velocity) const This could either be a static function or a function on ScrollSnap"Manager"? > Source/WebCore/page/scrolling/ScrollSnapManager.cpp:74 > + if (scrollDestination >= positions.at(positions.size() - 1)) > + return positions.at(positions.size() - 1); Vector has last(). You'd better check that size() > 0. > Source/WebCore/page/scrolling/ScrollSnapManager.cpp:80 > + middleIndex = (lowerIndex + upperIndex) / 2; middleIndex should be declared here. > Source/WebCore/page/scrolling/ScrollSnapManager.cpp:89 > + if (scrollDestination < positions.at(middleIndex)) > + upperIndex = middleIndex; > + else if (scrollDestination > positions.at(middleIndex)) > + lowerIndex = middleIndex; > + else { > + upperIndex = middleIndex; > + lowerIndex = middleIndex; > + break; > + } Maybe std::binary_search? > Source/WebCore/page/scrolling/ScrollSnapManager.cpp:96 > + // If velocity is zero, then the user released without flicking, so we simply snap to the closer point. I don't think this comment is useful. > Source/WebCore/page/scrolling/ScrollSnapManager.cpp:97 > + return scrollDestination - lowerSnapPosition <= upperSnapPosition - scrollDestination ? lowerSnapPosition : upperSnapPosition; Break out "scrollDestination - lowerSnapPosition <= upperSnapPosition - scrollDestination" into a named variable so I can tell what it is. > Source/WebCore/page/scrolling/ScrollSnapManager.cpp:100 > +ScrollSnapManager::ScrollSnapManager(ScrollEventAxis axis) Maybe the name should reflect that this "manager" only handles a single axis. > Source/WebCore/page/scrolling/ScrollSnapManager.cpp:148 > +void ScrollSnapManager::updateSnapDataFromFrameView(FrameView* frameView) > +{ > + if (RenderView* renderView = frameView->renderView()) { > + HTMLElement* body = renderView->document().body(); > + if (body) { > + const RenderStyle& style = body->renderer()->style(); > + LayoutUnit size = m_axis == ScrollEventAxis::Horizontal ? renderView->width() : renderView->height(); > + LayoutUnit scrollSize = m_axis == ScrollEventAxis::Horizontal ? renderView->scrollWidth() : renderView->scrollHeight(); > + if (scrollSize <= 0 || size <= 0 || size >= scrollSize) > + return; > + > + maxScrollOffset = scrollSize - size; > + if ((m_axis == ScrollEventAxis::Horizontal && !style.scrollSnapUsesElementsX()) > + || (m_axis == ScrollEventAxis::Vertical && !style.scrollSnapUsesElementsY())) > + updateSnapDataFromStyle(style, size); > + else > + updateSnapDataFromStyleAndParentElement(style, body, size); > + } > + } > +} > + > +void ScrollSnapManager::updateSnapDataFromParentElement(HTMLElement* parent) > +{ > + if (RenderBox* parentBox = parent->renderBox()) { > + const RenderStyle& style = parentBox->style(); > + LayoutUnit size = m_axis == ScrollEventAxis::Horizontal ? parentBox->width() : parentBox->height(); > + LayoutUnit scrollSize = m_axis == ScrollEventAxis::Horizontal ? parentBox->scrollWidth() : parentBox->scrollHeight(); > + if (scrollSize <= 0 || size <= 0 || size >= scrollSize) > + return; > + > + maxScrollOffset = scrollSize - size; > + if ((m_axis == ScrollEventAxis::Horizontal && !style.scrollSnapUsesElementsX()) > + || (m_axis == ScrollEventAxis::Vertical && !style.scrollSnapUsesElementsY())) > + updateSnapDataFromStyle(style, size); > + else > + updateSnapDataFromStyleAndParentElement(style, parent, size); > + } > +} The point of ScrollableArea is to abstract out differences between different scrollable things, so you should re-write this in terms of ScrollableArea, and not have different function for FrameView and overflow. > Source/WebCore/page/scrolling/ScrollSnapManager.cpp:154 > + LayoutUnit parentOffset = m_axis == ScrollEventAxis::Horizontal ? parent->renderBox()->offsetLeft() : parent->renderBox()->offsetTop(); We try not to use offset* in our own code. It usually indicates code that failed to consider CSS transforms. I don' think you need parentOffset at all here. > Source/WebCore/page/scrolling/ScrollSnapManager.cpp:160 > + while (child && lastPotentialSnapPosition < maxScrollOffset) { This walking of the nodes under the "elements" scroller could be very expensive. I think it would be better to somehow register RenderObjects with scrollSnapCoordinates() with their enclosing scroller. > Source/WebCore/page/scrolling/ScrollSnapManager.cpp:164 > + LayoutUnit childOffset = (m_axis == ScrollEventAxis::Horizontal ? childBox->offsetLeft() : childBox->offsetTop()) - parentOffset; This should use localToContainerPoint() or similar. You should test content with CSS transforms between the scrolling thing and the target elements. > Source/WebCore/page/scrolling/ScrollSnapManager.cpp:178 > + // Always put a snap point on the maximum scroll offset. Is that behavior defined in the spec? > Source/WebCore/page/scrolling/ScrollSnapManager.cpp:183 > +void ScrollSnapManager::updateSnapDataFromStyle(const RenderStyle& style, LayoutUnit size) It's unclear what the "size" param refers to. > Source/WebCore/page/scrolling/ScrollSnapManager.cpp:202 > + LayoutUnit curSnapPositionShift = -destinationOffset; Not clear why. > Source/WebCore/page/scrolling/ScrollSnapManager.h:42 > +#include <wtf/Vector.h> > +#include <wtf/text/CString.h> > +#include <wtf/text/WTFString.h> I don't think you need these. > Source/WebCore/page/scrolling/ScrollSnapManager.h:58 > +class ScrollSnapManager { http://blog.codinghorror.com/i-shall-call-it-somethingmanager/ > Source/WebCore/page/scrolling/ScrollSnapManager.h:66 > + LayoutUnit maxScrollOffset; m_maxScrollOffset > Source/WebCore/page/scrolling/ScrollSnapManager.h:67 > + SnapPositionList snapPositions; m_snapPositions. I think both these should be private.

Created attachment 236348 [details] Patch

Comment on attachment 236310 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236310&action=review Thanks for the feedback! I've restructured my code to avoid having a "ScrollSnapManager" altogether, mainly by separating snap position storage from snap animation into classes that are less vague than "Manager". >> Source/JavaScriptCore/ChangeLog:6 >> + Enable CSS_SCROLL_SNAP on iPhoneOS and simulator. > > It seems odd to do this in the same commit. Removed. >> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:68 >> +LayoutUnit SnapPositionList::closestSnapPosition(LayoutUnit scrollDestination, float velocity) const > > This could either be a static function or a function on ScrollSnap"Manager"? I'll change this to a static function. Good call. >> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:74 >> + return positions.at(positions.size() - 1); > > Vector has last(). You'd better check that size() > 0. Fixed to use last(), and added early return for empty vector. >> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:80 >> + middleIndex = (lowerIndex + upperIndex) / 2; > > middleIndex should be declared here. Fixed. >> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:89 >> + } > > Maybe std::binary_search? I'll update the implementation to use std's binary search. >> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:96 >> + // If velocity is zero, then the user released without flicking, so we simply snap to the closer point. > > I don't think this comment is useful. Removed. >> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:97 >> + return scrollDestination - lowerSnapPosition <= upperSnapPosition - scrollDestination ? lowerSnapPosition : upperSnapPosition; > > Break out "scrollDestination - lowerSnapPosition <= upperSnapPosition - scrollDestination" into a named variable so I can tell what it is. Fixed. It indicates whether or not the scroll destination is closer to the lower snap point. >> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:100 >> +ScrollSnapManager::ScrollSnapManager(ScrollEventAxis axis) > > Maybe the name should reflect that this "manager" only handles a single axis. Fixed -- does SingleAxisSnapList sound better? I'm also separating animation code from the SingleAxisSnapList. >> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:148 >> +} > > The point of ScrollableArea is to abstract out differences between different scrollable things, so you should re-write this in terms of ScrollableArea, and not have different function for FrameView and overflow. Moved the bulk of the logic over to ScrollableArea, which is shared by FrameView and RenderLayer. >> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:160 >> + while (child && lastPotentialSnapPosition < maxScrollOffset) { > > This walking of the nodes under the "elements" scroller could be very expensive. I think it would be better to somehow register RenderObjects with scrollSnapCoordinates() with their enclosing scroller. Agreed. If it's alright, I think I'll keep it like this for this patch, and it can be an optimization later on. >> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:164 >> + LayoutUnit childOffset = (m_axis == ScrollEventAxis::Horizontal ? childBox->offsetLeft() : childBox->offsetTop()) - parentOffset; > > This should use localToContainerPoint() or similar. You should test content with CSS transforms between the scrolling thing and the target elements. Good point -- I'll fix this. >> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:178 >> + // Always put a snap point on the maximum scroll offset. > > Is that behavior defined in the spec? Added a comment indicating that it's something I added to prevent unreachable content. >> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:183 >> +void ScrollSnapManager::updateSnapDataFromStyle(const RenderStyle& style, LayoutUnit size) > > It's unclear what the "size" param refers to. Changed to viewSize. >> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:202 >> + LayoutUnit curSnapPositionShift = -destinationOffset; > > Not clear why. Agreed, looks kind of weird. Changed implementation to not use -destinationOffset. >> Source/WebCore/page/scrolling/ScrollSnapManager.h:58 >> +class ScrollSnapManager { > > http://blog.codinghorror.com/i-shall-call-it-somethingmanager/ This was enlightening :) The ScrollSnapManager was originally intended to handle storing snap positions, and handling snapping animations/determining where to snap. I plan on separating the snap point data from the snap animation code, so the former will be in SingleAxisSnapList and the latter in something like ScrollSnapAnimator (hopefully "Animator" is a lot less nebulous than "Manager").

Comment on attachment 236348 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236348&action=review > Source/WebCore/html/HTMLElement.cpp:364 > +void HTMLElement::appendChildSnapOffsetsToVector(ScrollEventAxis axis, Vector<LayoutUnit>& snapOffsets) > +{ > + Element* child = children()->collectionBegin(); > + while (child) { > + child->appendSnapOffsetsToVector(axis, snapOffsets); > + child = child->nextElementSibling(); > + } > +} > + > +void HTMLElement::appendSnapOffsetsToVector(ScrollEventAxis axis, Vector<LayoutUnit>& snapOffsets) > +{ > + RenderBox* box = renderBox(); > + LayoutUnit viewSize = axis == ScrollEventAxis::Horizontal ? box->width() : box->height(); > + LayoutUnit positionOffset = axis == ScrollEventAxis::Horizontal ? box->x() : box->y(); > + for (SnapCoordinate coordinate : box->style().scrollSnapCoordinates()) { > + LayoutUnit lastPotentialSnapPosition = positionOffset + valueForLength(axis == ScrollEventAxis::Horizontal ? coordinate.first : coordinate.second, viewSize); > + if (lastPotentialSnapPosition > 0) > + snapOffsets.append(lastPotentialSnapPosition); > + } > +} > +#endif > + Are you sure these functions need to be on Element? RenderBox is not even guaranteed here. (HTMLElement vs. RenderBox). These functions look to me like simple helpers and I am sure there are better places to put them. > Source/WebCore/html/HTMLElement.h:108 > +#if ENABLE(CSS_SCROLL_SNAP) > + void appendChildSnapOffsetsToVector(ScrollEventAxis, Vector<LayoutUnit>&); > + virtual void appendSnapOffsetsToVector(ScrollEventAxis, Vector<LayoutUnit>&) override; > +#endif Same question. Why are these on HTMLElement? > Source/WebCore/page/FrameView.cpp:856 > + if (!renderView() || !renderView()->document().body()) > + return; > + > + Element* body = renderView()->document().body(); if you use frame().document().body(), no need to check against renderview. > Source/WebCore/page/FrameView.cpp:857 > + updateSnapAxisFromElementAndStyle(axis, body, renderView(), body->renderer()->style()); body always has renderer() here? -and we always have body?

Created attachment 236405 [details] Patch

Comment on attachment 236405 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236405&action=review > Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:43 > +void AxisScrollSnapOffsets::appendChildSnapOffsetsToVector(HTMLElement* parent, ScrollEventAxis axis, Vector<LayoutUnit>& snapOffsetSubsequence) HTMLElement& ? > Source/WebCore/page/scrolling/AxisScrollSnapOffsets.h:31 > +#include "HTMLElement.h" class HTMLElement instead? > Source/WebCore/page/scrolling/AxisScrollSnapOffsets.h:47 > + void updateFromOffsets(LayoutUnit, LayoutUnit, const Vector<LayoutUnit>&, LayoutUnit, bool, LayoutUnit); > + LayoutUnit closestSnapPosition(LayoutUnit, float = 0) const; Could you add the argument names? There's no way to tell what those LayoutUnits are for by looking at the .h file. > Source/WebCore/page/scrolling/AxisScrollSnapOffsets.h:52 > + Vector<LayoutUnit> offsets; private? m_offsets? > Source/WebCore/page/scrolling/AxisScrollSnapOffsets.h:53 > + bool dirty; not used. > Source/WebCore/platform/ScrollableArea.cpp:45 > +#include "Element.h" HTMLElement.h includes Element.h (indirectly) > Source/WebCore/platform/ScrollableArea.cpp:59 > + void* horizontalSnapAxisPointer; > + void* verticalSnapAxisPointer; > +#endif not used? > Source/WebCore/platform/ScrollableArea.cpp:207 > +void ScrollableArea::updateSnapAxisFromElementAndStyle(ScrollEventAxis axis, Element* scrollingElement, RenderBox* box, const RenderStyle& style) Element& RenderBox& ? > Source/WebCore/platform/ScrollableArea.h:31 > +#include "AxisScrollSnapOffsets.h" wrong order. > Source/WebCore/platform/ScrollableArea.h:47 > +class AxisScrollSnapOffsets; already included.

Comment on attachment 236405 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236405&action=review r- for the ScrollableArea layering violation. > Source/WebCore/page/FrameView.cpp:860 > + updateSnapAxisFromElementAndStyle(axis, body, renderView(), body->renderer()->style()); To get document scroll snapping, are authors expected to style the body element? Maybe this is a spec issue; what if they specify scroll snap points on <html>? > Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:57 > + Element* child = parent->children()->collectionBegin(); > + while (child) { > + RenderBox* box = child->renderBox(); > + LayoutUnit viewSize = isHorizontalAxis ? box->width() : box->height(); > + LayoutUnit positionOffset = isHorizontalAxis ? box->x() : box->y(); > + for (SnapCoordinate coordinate : box->style().scrollSnapCoordinates()) { > + LayoutUnit lastPotentialSnapPosition = positionOffset + valueForLength(isHorizontalAxis ? coordinate.first : coordinate.second, viewSize); > + if (lastPotentialSnapPosition > 0) > + snapOffsetSubsequence.append(lastPotentialSnapPosition); > + } > + child = child->nextElementSibling(); > + } Please add a FIXME that this is temporary code. > Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:65 > +void AxisScrollSnapOffsets::updateFromOffsets(LayoutUnit viewSize, LayoutUnit scrollSize, const Vector<LayoutUnit>& snapOffsetSubsequence, LayoutUnit destinationOffset, bool hasRepeat, LayoutUnit repeatOffset) AxisScrollSnapOffsets::updateFromOffsets is awkward. updateFromStyle()? Can it just take a RenderStyle&? > Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:146 > + for (size_t i = 0; i < snapAxis.offsets.size(); i++) { > + if (snapAxis.offsets.at(i) != offsets.at(i)) > + return false; > + } Can't you just compare the Vectors? >> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.h:52 >> + Vector<LayoutUnit> offsets; > > private? m_offsets? Yes. > Source/WebCore/platform/ScrollableArea.cpp:49 > +#include "HTMLElement.h" > +#include "RenderBox.h" > +#include "RenderStyle.h" > +#endif Bunch of layering violations here. ScrollableArea should not know anything about elements or renderers. > Source/WebCore/platform/ScrollableArea.h:72 > + virtual void updateSnapAxis(ScrollEventAxis) { }; The name could be better; you're updating offsets for an axis, not the axis itself. Also, this means you're doing TWO tree walks per update (one per axis). You should do both axes at the same time. > Source/WebCore/platform/ScrollableArea.h:279 > + void updateSnapAxisFromElementAndStyle(ScrollEventAxis, Element*, RenderBox*, const RenderStyle&); To resolve this layering violation, you either have to make this virtual, or compute the offsets externally and put them onto ScrollableArea. > Source/WebCore/platform/ScrollableArea.h:298 > + mutable OwnPtr<AxisScrollSnapOffsets> m_horizontalSnapAxis; > + mutable OwnPtr<AxisScrollSnapOffsets> m_verticalSnapAxis; Why mutable? Better names would be m_horizontalSnapOffsets etc.

Comment on attachment 236405 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236405&action=review Thank you Simon and Zalan for the reviews! The logic is now contained in AxisScrollSnapOffsets, so I won't need Element and ScrollableArea to know about classes they shouldn't. >> Source/WebCore/page/FrameView.cpp:860 >> + updateSnapAxisFromElementAndStyle(axis, body, renderView(), body->renderer()->style()); > > To get document scroll snapping, are authors expected to style the body element? Maybe this is a spec issue; what if they specify scroll snap points on <html>? I'll contact Ted about this. Thanks! >> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:43 >> +void AxisScrollSnapOffsets::appendChildSnapOffsetsToVector(HTMLElement* parent, ScrollEventAxis axis, Vector<LayoutUnit>& snapOffsetSubsequence) > > HTMLElement& ? Fixed. >> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:57 >> + } > > Please add a FIXME that this is temporary code. Ah, right -- added a FIXME. >> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:65 >> +void AxisScrollSnapOffsets::updateFromOffsets(LayoutUnit viewSize, LayoutUnit scrollSize, const Vector<LayoutUnit>& snapOffsetSubsequence, LayoutUnit destinationOffset, bool hasRepeat, LayoutUnit repeatOffset) > > AxisScrollSnapOffsets::updateFromOffsets is awkward. updateFromStyle()? Can it just take a RenderStyle&? I agree -- using RenderStyle makes things a bit simpler. >> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:146 >> + } > > Can't you just compare the Vectors? Removed the operator==. I'll keep this in mind for the next patch though. >> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.h:31 >> +#include "HTMLElement.h" > > class HTMLElement instead? Changed to forward declaration. >> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.h:47 >> + LayoutUnit closestSnapPosition(LayoutUnit, float = 0) const; > > Could you add the argument names? There's no way to tell what those LayoutUnits are for by looking at the .h file. Fixed. >>> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.h:52 >>> + Vector<LayoutUnit> offsets; >> >> private? m_offsets? > > Yes. Made it private. >> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.h:53 >> + bool dirty; > > not used. Removed. >> Source/WebCore/platform/ScrollableArea.cpp:45 >> +#include "Element.h" > > HTMLElement.h includes Element.h (indirectly) Got it. Removed Element.h >> Source/WebCore/platform/ScrollableArea.cpp:49 >> +#endif > > Bunch of layering violations here. ScrollableArea should not know anything about elements or renderers. Got it. Moved bulk of logic to AxisScrollSnapOffsets. >> Source/WebCore/platform/ScrollableArea.cpp:59 >> +#endif > > not used? Used to add pointers to the snap axes in ScrollableArea. >> Source/WebCore/platform/ScrollableArea.cpp:207 >> +void ScrollableArea::updateSnapAxisFromElementAndStyle(ScrollEventAxis axis, Element* scrollingElement, RenderBox* box, const RenderStyle& style) > > Element& RenderBox& ? Fixed. >> Source/WebCore/platform/ScrollableArea.h:31 >> +#include "AxisScrollSnapOffsets.h" > > wrong order. Changed to use just a forward declaration. >> Source/WebCore/platform/ScrollableArea.h:47 >> +class AxisScrollSnapOffsets; > > already included. Removed. >> Source/WebCore/platform/ScrollableArea.h:72 >> + virtual void updateSnapAxis(ScrollEventAxis) { }; > > The name could be better; you're updating offsets for an axis, not the axis itself. > Also, this means you're doing TWO tree walks per update (one per axis). You should do both axes at the same time. Changed to updateSnapOffsets(). Also changed to make sure only 1 tree walk happens iff at least 1 axis depends on elements. >> Source/WebCore/platform/ScrollableArea.h:279 >> + void updateSnapAxisFromElementAndStyle(ScrollEventAxis, Element*, RenderBox*, const RenderStyle&); > > To resolve this layering violation, you either have to make this virtual, or compute the offsets externally and put them onto ScrollableArea. Moved logic to AxisScrollSnapOffsets. >> Source/WebCore/platform/ScrollableArea.h:298 >> + mutable OwnPtr<AxisScrollSnapOffsets> m_verticalSnapAxis; > > Why mutable? > Better names would be m_horizontalSnapOffsets etc. It was mutable because my getter was marked const but created a new snap offset list if it didn't exist (I looked to scrollAnimator() const for reference -- m_scrollAnimator is also a mutable OwnPtr). I've changed the getter to not use const.

Created attachment 236449 [details] Patch

Created attachment 236450 [details] Patch

Created attachment 236453 [details] Patch

Comment on attachment 236453 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236453&action=review > Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:94 > + LayoutUnit viewWidth = box->width(); > + LayoutUnit viewHeight = box->height(); > + LayoutUnit positionX = box->x(); > + LayoutUnit positionY = box->y(); This is making assumptions about the render tree that may not be correct in some cases; box->x() isn't necessarily the offset from parent's renderer. It also ignores CSS transforms (for which you need to add test cases). You should use RenderObject conversion functions. What happens if the snap points are non-ordered (e.g. child has position: absolute; left: -200px). Do you need to sort the sequence somewhere? > Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:130 > + m_offsets.append(potentialSnapPosition); > + lastSnapPosition = potentialSnapPosition + destinationOffset; As we discussed, this is susceptible to memory explosion with crazy content (repeat 1px). Please add a FIXME. > Source/WebCore/platform/ScrollableArea.cpp:36 > +#include "AxisScrollSnapOffsets.h" Uh oh, platform code can't include page/scrolling code. > Source/WebCore/platform/ScrollableArea.h:288 > + OwnPtr<AxisScrollSnapOffsets> m_horizontalSnapOffsets; > + OwnPtr<AxisScrollSnapOffsets> m_verticalSnapOffsets; I think these need to be raw Vector<LayoutUnit> here to avoid layering violations. That turns AxisScrollSnapOffsets into a helper that just spits out a Vector<LayoutUnit>.

Comment on attachment 236453 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236453&action=review Thank you for the review! >> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:94 >> + LayoutUnit positionY = box->y(); > > This is making assumptions about the render tree that may not be correct in some cases; box->x() isn't necessarily the offset from parent's renderer. It also ignores CSS transforms (for which you need to add test cases). > > You should use RenderObject conversion functions. > > What happens if the snap points are non-ordered (e.g. child has position: absolute; left: -200px). Do you need to sort the sequence somewhere? Good point -- totally missed this. I'll need to sort the subsequence here after iterating through the children. Same goes for the "... repeat(...)" case. >> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:130 >> + lastSnapPosition = potentialSnapPosition + destinationOffset; > > As we discussed, this is susceptible to memory explosion with crazy content (repeat 1px). Please add a FIXME. Got it. >> Source/WebCore/platform/ScrollableArea.cpp:36 >> +#include "AxisScrollSnapOffsets.h" > > Uh oh, platform code can't include page/scrolling code. :( >> Source/WebCore/platform/ScrollableArea.h:288 >> + OwnPtr<AxisScrollSnapOffsets> m_verticalSnapOffsets; > > I think these need to be raw Vector<LayoutUnit> here to avoid layering violations. That turns AxisScrollSnapOffsets into a helper that just spits out a Vector<LayoutUnit>. Fixed. (I've made AxisScrollSnapOffsets just contain a helper function for constructing snap points. It will also contain a function to get the closest snap offset in the future.)

Created attachment 236482 [details] Patch

Comment on attachment 236482 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236482&action=review > Source/WebCore/platform/ScrollableArea.h:271 > + OwnPtr<Vector<LayoutUnit>> m_horizontalSnapOffsets; > + OwnPtr<Vector<LayoutUnit>> m_verticalSnapOffsets; These should be std::unique_ptrs

Created attachment 236522 [details] Patch

Comment on attachment 236482 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236482&action=review >> Source/WebCore/platform/ScrollableArea.h:271 >> + OwnPtr<Vector<LayoutUnit>> m_verticalSnapOffsets; > > These should be std::unique_ptrs Fixed. Thanks!

Comment on attachment 236522 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236522&action=review > Source/WebCore/page/FrameView.cpp:869 > + if (!m_horizontalSnapOffsets) > + m_horizontalSnapOffsets = std::make_unique<Vector<LayoutUnit>>(); > + > + if (!m_verticalSnapOffsets) > + m_verticalSnapOffsets = std::make_unique<Vector<LayoutUnit>>(); You shouldn't allocate these vectors if there are no snap points. > Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:98 > +void updateHorizontalVerticalSnapOffsets(Vector<LayoutUnit>& horizontalSnapOffsets, Vector<LayoutUnit>& verticalSnapOffsets, HTMLElement& scrollingElement, const RenderBox& box, const RenderStyle& style) This should have two unique_ptr<Vector<LayoutUnit>>& out params so that you can only allocate the vectors if you have snap points. > Source/WebCore/platform/ScrollableArea.h:271 > + std::unique_ptr<Vector<LayoutUnit>> m_horizontalSnapOffsets; > + std::unique_ptr<Vector<LayoutUnit>> m_verticalSnapOffsets; Please make these private and provide protected setters.

Created attachment 236550 [details] Patch

Created attachment 236552 [details] Patch

Comment on attachment 236550 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236550&action=review > Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:60 > + std::sort(horizontalSnapOffsetSubsequence.begin(), horizontalSnapOffsetSubsequence.end()); sort somehow knows how to sort LayoutUnits? > Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:75 > + LayoutUnit potentialSnapPosition; This should be declared inside the loop. > Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:112 > + if (!computeHorizontalSnapOffsets && !computeVerticalSnapOffsets) > + return; Shouldn't you call scrollableArea.clearHorizontalSnapOffsets() etc here too? Or just return after the next two blocks. > Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:130 > + if (style.scrollSnapUsesElementsX() || style.scrollSnapUsesElementsY()) > + appendAndSortChildSnapOffsets(scrollingElement, computeHorizontalSnapOffsets, horizontalSnapOffsetSubsequence, computeVerticalSnapOffsets, verticalSnapOffsetSubsequence); > + > + if (computeHorizontalSnapOffsets) { > + if (!style.scrollSnapUsesElementsX()) { This logic is awkward. Would be nicer to set computeHorizontalSnapOffsets to false if style.scrollSnapUsesElementsX() is set, and rename it to horizontalOffsetsNeedsRecomputing. > Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:134 > + for (Length snapLength : style.scrollSnapOffsetsX()) > + horizontalSnapOffsetSubsequence.append(valueForLength(snapLength, viewWidth)); > + > + std::sort(horizontalSnapOffsetSubsequence.begin(), horizontalSnapOffsetSubsequence.end()); Why doesn't updateFromStyle() do this work? > Source/WebCore/page/scrolling/AxisScrollSnapOffsets.h:43 > +void updateSnapOffsetsForScrollableArea(ScrollableArea&, HTMLElement& scrollingElement, const RenderBox&, const RenderStyle&); The RenderBox needs a name; does it correspond with the element? So does the style; which element's style is it? > Source/WebCore/platform/ScrollableArea.cpp:396 > + m_horizontalSnapOffsets.reset(); m_horizontalSnapOffsets.reset = nullptr > Source/WebCore/platform/ScrollableArea.cpp:401 > + m_verticalSnapOffsets.reset(); m_verticalSnapOffsets = nullptr > Source/WebCore/platform/ScrollableArea.h:63 > + Vector<LayoutUnit>* horizontalSnapOffsets() const { return m_horizontalSnapOffsets.get(); }; > + Vector<LayoutUnit>* verticalSnapOffsets() const { return m_verticalSnapOffsets.get(); }; Should turn const Vector<>..... Don't want the caller messing with it.

Comment on attachment 236550 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236550&action=review >> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:60 >> + std::sort(horizontalSnapOffsetSubsequence.begin(), horizontalSnapOffsetSubsequence.end()); > > sort somehow knows how to sort LayoutUnits? By default, std::sort uses operator<, which is declared in LayoutUnit.h, so this should be fine. >> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:75 >> + LayoutUnit potentialSnapPosition; > > This should be declared inside the loop. Fixed. >> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:112 >> + return; > > Shouldn't you call scrollableArea.clearHorizontalSnapOffsets() etc here too? Or just return after the next two blocks. Good catch -- the early return should happen afterwards. >> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:130 >> + if (!style.scrollSnapUsesElementsX()) { > > This logic is awkward. Would be nicer to set computeHorizontalSnapOffsets to false if style.scrollSnapUsesElementsX() is set, and rename it to horizontalOffsetsNeedsRecomputing. Moved around some logic and renamed variables to make what I'm doing more clear. The part where I build the subsequence from "elements" or from "...repeat(...)" is now in the same place, which happens before I make calls to updateFromStyle. >> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:134 >> + std::sort(horizontalSnapOffsetSubsequence.begin(), horizontalSnapOffsetSubsequence.end()); > > Why doesn't updateFromStyle() do this work? Moved sorting and size check over to updateFromStyle. >> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.h:43 >> +void updateSnapOffsetsForScrollableArea(ScrollableArea&, HTMLElement& scrollingElement, const RenderBox&, const RenderStyle&); > > The RenderBox needs a name; does it correspond with the element? So does the style; which element's style is it? Fixed (scrollingElement___) >> Source/WebCore/platform/ScrollableArea.cpp:396 >> + m_horizontalSnapOffsets.reset(); > > m_horizontalSnapOffsets.reset = nullptr Changed to = nullptr. >> Source/WebCore/platform/ScrollableArea.cpp:401 >> + m_verticalSnapOffsets.reset(); > > m_verticalSnapOffsets = nullptr Changed to = nullptr. >> Source/WebCore/platform/ScrollableArea.h:63 >> + Vector<LayoutUnit>* verticalSnapOffsets() const { return m_verticalSnapOffsets.get(); }; > > Should turn const Vector<>..... Don't want the caller messing with it. Fixed.

Created attachment 236558 [details] Patch

Comment on attachment 236558 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=236558&action=review > Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:128 > + if (!scrollingElementStyle.scrollSnapUsesElementsY() && canComputeHorizontalOffsets) { Typo: canComputeVerticalOffsets

Created attachment 236560 [details] Patch

Comment on attachment 236560 [details] Patch Clearing flags on attachment: 236560 Committed r172543: <http://trac.webkit.org/changeset/172543>

All reviewed patches have been landed. Closing bug.