[webkit-reviews] review denied: [Bug 135268] Compute and store snap point positions : [Attachment 236453] Patch

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

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 135268: Compute and store snap point positions

Attachment 236453: Patch

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
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>.

More information about the webkit-reviews mailing list