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

Mon Aug 11 16:24:57 PDT 2014

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

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

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

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

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