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

[bugzilla-daemon at webkit.org]

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


Zalan Bujtas <zalan at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #236348|                            |commit-queue-
               Flag|                            |




--- Comment #8 from Zalan Bujtas <zalan at apple.com>  2014-08-11 10:31:01 PST ---
(From update of attachment 236348)
View in context: https://bugs.webkit.org/attachment.cgi?id=236348&action=review

> Source/WebCore/html/HTMLElement.cpp:364
> +void HTMLElement::appendChildSnapOffsetsToVector(ScrollEventAxis axis, Vector<LayoutUnit>& snapOffsets)
> +{
> +    Element* child = children()->collectionBegin();
> +    while (child) {
> +        child->appendSnapOffsetsToVector(axis, snapOffsets);
> +        child = child->nextElementSibling();
> +    }
> +}
> +
> +void HTMLElement::appendSnapOffsetsToVector(ScrollEventAxis axis, Vector<LayoutUnit>& snapOffsets)
> +{
> +    RenderBox* box = renderBox();
> +    LayoutUnit viewSize = axis == ScrollEventAxis::Horizontal ? box->width() : box->height();
> +    LayoutUnit positionOffset = axis == ScrollEventAxis::Horizontal ? box->x() : box->y();
> +    for (SnapCoordinate coordinate : box->style().scrollSnapCoordinates()) {
> +        LayoutUnit lastPotentialSnapPosition = positionOffset + valueForLength(axis == ScrollEventAxis::Horizontal ? coordinate.first : coordinate.second, viewSize);
> +        if (lastPotentialSnapPosition > 0)
> +            snapOffsets.append(lastPotentialSnapPosition);
> +    }
> +}
> +#endif
> +

Are you sure these functions need to be on Element? RenderBox is not even guaranteed here. (HTMLElement vs. RenderBox). These functions look to me like simple helpers and I am sure there are better places to put them.

> Source/WebCore/html/HTMLElement.h:108
> +#if ENABLE(CSS_SCROLL_SNAP)
> +    void appendChildSnapOffsetsToVector(ScrollEventAxis, Vector<LayoutUnit>&);
> +    virtual void appendSnapOffsetsToVector(ScrollEventAxis, Vector<LayoutUnit>&) override;
> +#endif

Same question. Why are these on HTMLElement?

> Source/WebCore/page/FrameView.cpp:856
> +    if (!renderView() || !renderView()->document().body())
> +        return;
> +
> +    Element* body = renderView()->document().body();

if you use frame().document().body(), no need to check against renderview.

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

body always has renderer() here? -and we always have body?

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