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

[bugzilla-daemon at webkit.org]

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


Wenson Hsieh <wenson_hsieh at apple.com> changed:

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




--- Comment #26 from Wenson Hsieh <wenson_hsieh at apple.com>  2014-08-13 15:37:18 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?

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.

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