[webkit-reviews] review denied: [Bug 135268] Compute and store snap point positions : [Attachment 236522] 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 236522: Patch
https://bugs.webkit.org/attachment.cgi?id=236522&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=236522&action=review


> Source/WebCore/page/FrameView.cpp:869
> +    if (!m_horizontalSnapOffsets)
> +	   m_horizontalSnapOffsets = std::make_unique<Vector<LayoutUnit>>();
> +
> +    if (!m_verticalSnapOffsets)
> +	   m_verticalSnapOffsets = std::make_unique<Vector<LayoutUnit>>();

You shouldn't allocate these vectors if there are no snap points.

> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:98
> +void updateHorizontalVerticalSnapOffsets(Vector<LayoutUnit>&
horizontalSnapOffsets, Vector<LayoutUnit>& verticalSnapOffsets, HTMLElement&
scrollingElement, const RenderBox& box, const RenderStyle& style)

This should have two unique_ptr<Vector<LayoutUnit>>& out params so that you can
only allocate the vectors if you have snap points.

> Source/WebCore/platform/ScrollableArea.h:271
> +    std::unique_ptr<Vector<LayoutUnit>> m_horizontalSnapOffsets;
> +    std::unique_ptr<Vector<LayoutUnit>> m_verticalSnapOffsets;

Please make these private and provide protected setters.