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

Mon Aug 11 16:24:53 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
https://bugs.webkit.org/show_bug.cgi?id=135268

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

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

r- for the ScrollableArea layering violation.

> Source/WebCore/page/FrameView.cpp:860
> +    updateSnapAxisFromElementAndStyle(axis, body, renderView(),
body->renderer()->style());

To get document scroll snapping, are authors expected to style the body
element? Maybe this is a spec issue; what if they specify scroll snap points on
<html>?

> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:57
> +    Element* child = parent->children()->collectionBegin();
> +    while (child) {
> +	   RenderBox* box = child->renderBox();
> +	   LayoutUnit viewSize = isHorizontalAxis ? box->width() :
box->height();
> +	   LayoutUnit positionOffset = isHorizontalAxis ? box->x() : box->y();
> +	   for (SnapCoordinate coordinate :
box->style().scrollSnapCoordinates()) {
> +	       LayoutUnit lastPotentialSnapPosition = positionOffset +
valueForLength(isHorizontalAxis ? coordinate.first : coordinate.second,
viewSize);
> +	       if (lastPotentialSnapPosition > 0)
> +		   snapOffsetSubsequence.append(lastPotentialSnapPosition);
> +	   }
> +	   child = child->nextElementSibling();
> +    }

Please add a FIXME that this is temporary code.

> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:65
> +void AxisScrollSnapOffsets::updateFromOffsets(LayoutUnit viewSize,
LayoutUnit scrollSize, const Vector<LayoutUnit>& snapOffsetSubsequence,
LayoutUnit destinationOffset, bool hasRepeat, LayoutUnit repeatOffset)

AxisScrollSnapOffsets::updateFromOffsets is awkward. updateFromStyle()? Can it
just take a RenderStyle&?

> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:146
> +    for (size_t i = 0; i < snapAxis.offsets.size(); i++) {
> +	   if (snapAxis.offsets.at(i) != offsets.at(i))
> +	       return false;
> +    }

Can't you just compare the Vectors?

>> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.h:52
>> +	Vector<LayoutUnit> offsets;
> 
> private? m_offsets?

Yes.

> Source/WebCore/platform/ScrollableArea.cpp:49
> +#include "HTMLElement.h"
> +#include "RenderBox.h"
> +#include "RenderStyle.h"
> +#endif

Bunch of layering violations here. ScrollableArea should not know anything
about elements or renderers.

> Source/WebCore/platform/ScrollableArea.h:72
> +    virtual void updateSnapAxis(ScrollEventAxis) { };

The name could be better; you're updating offsets for an axis, not the axis
itself.
Also, this means you're doing TWO tree walks per update (one per axis). You
should do both axes at the same time.

> Source/WebCore/platform/ScrollableArea.h:279
> +    void updateSnapAxisFromElementAndStyle(ScrollEventAxis, Element*,
RenderBox*, const RenderStyle&);

To resolve this layering violation, you either have to make this virtual, or
compute the offsets externally and put them onto ScrollableArea.

> Source/WebCore/platform/ScrollableArea.h:298
> +    mutable OwnPtr<AxisScrollSnapOffsets> m_horizontalSnapAxis;
> +    mutable OwnPtr<AxisScrollSnapOffsets> m_verticalSnapAxis;

Why mutable?
Better names would be m_horizontalSnapOffsets etc.