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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Aug 12 11:25:12 PDT 2014


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


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

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #236453|review?                     |review-
               Flag|                            |




--- Comment #16 from Simon Fraser (smfr) <simon.fraser at apple.com>  2014-08-12 11:25:16 PST ---
(From update of attachment 236453)
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>.

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



More information about the webkit-unassigned mailing list