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

[bugzilla-daemon at webkit.org]

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





--- Comment #17 from Wenson Hsieh <wenson_hsieh at apple.com>  2014-08-12 16:10:44 PST ---
(From update of attachment 236453)
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.)

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