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

Tue Aug 12 09:05:20 PDT 2014

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

--- Comment #12 from Wenson Hsieh <wenson_hsieh at apple.com>  2014-08-12 09:05:22 PST ---
(From update of attachment 236405)
View in context: https://bugs.webkit.org/attachment.cgi?id=236405&action=review

Thank you Simon and Zalan for the reviews! The logic is now contained in AxisScrollSnapOffsets, so I won't need Element and ScrollableArea to know about classes they shouldn't.

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

I'll contact Ted about this. Thanks!

>> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:43
>> +void AxisScrollSnapOffsets::appendChildSnapOffsetsToVector(HTMLElement* parent, ScrollEventAxis axis, Vector<LayoutUnit>& snapOffsetSubsequence)
> 
> HTMLElement& ?

Fixed.

>> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:57
>> +    }
> 
> Please add a FIXME that this is temporary code.

Ah, right -- added a FIXME.

>> 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&?

I agree -- using RenderStyle makes things a bit simpler.

>> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.cpp:146
>> +    }
> 
> Can't you just compare the Vectors?

Removed the operator==. I'll keep this in mind for the next patch though.

>> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.h:31
>> +#include "HTMLElement.h"
> 
> class HTMLElement instead?

Changed to forward declaration.

>> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.h:47
>> +    LayoutUnit closestSnapPosition(LayoutUnit, float = 0) const;
> 
> Could you add the argument names? There's no way to tell what those LayoutUnits are for by looking at the .h file.

Fixed.

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

Made it private.

>> Source/WebCore/page/scrolling/AxisScrollSnapOffsets.h:53
>> +    bool dirty;
> 
> not used.

Removed.

>> Source/WebCore/platform/ScrollableArea.cpp:45
>> +#include "Element.h"
> 
> HTMLElement.h includes Element.h (indirectly)

Got it. Removed Element.h

>> Source/WebCore/platform/ScrollableArea.cpp:49
>> +#endif
> 
> Bunch of layering violations here. ScrollableArea should not know anything about elements or renderers.

Got it. Moved bulk of logic to AxisScrollSnapOffsets.

>> Source/WebCore/platform/ScrollableArea.cpp:59
>> +#endif
> 
> not used?

Used to add pointers to the snap axes in ScrollableArea.

>> Source/WebCore/platform/ScrollableArea.cpp:207
>> +void ScrollableArea::updateSnapAxisFromElementAndStyle(ScrollEventAxis axis, Element* scrollingElement, RenderBox* box, const RenderStyle& style)
> 
> Element& RenderBox& ?

Fixed.

>> Source/WebCore/platform/ScrollableArea.h:31
>> +#include "AxisScrollSnapOffsets.h"
> 
> wrong order.

Changed to use just a forward declaration.

>> Source/WebCore/platform/ScrollableArea.h:47
>> +class AxisScrollSnapOffsets;
> 
> already included.

Removed.

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

Changed to updateSnapOffsets(). Also changed to make sure only 1 tree walk happens iff at least 1 axis depends on elements.

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

Moved logic to AxisScrollSnapOffsets.

>> Source/WebCore/platform/ScrollableArea.h:298
>> +    mutable OwnPtr<AxisScrollSnapOffsets> m_verticalSnapAxis;
> 
> Why mutable?
> Better names would be m_horizontalSnapOffsets etc.

It was mutable because my getter was marked const but created a new snap offset list if it didn't exist (I looked to scrollAnimator() const for reference -- m_scrollAnimator is also a mutable OwnPtr). I've changed the getter to not use const.

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