[Webkit-unassigned] [Bug 135268] Compute and store snap point positions

[bugzilla-daemon at webkit.org]

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


Simon Fraser (smfr) <simon.fraser at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #236550|1                           |0
        is obsolete|                            |




--- Comment #25 from Simon Fraser (smfr) <simon.fraser at apple.com>  2014-08-13 14:13:49 PST ---
(From update of attachment 236550)
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.

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