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

[bugzilla-daemon at webkit.org]

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
https://bugs.webkit.org/show_bug.cgi?id=135268

Attachment 236453: Patch
https://bugs.webkit.org/attachment.cgi?id=236453&action=review

------- 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>.