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

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


> Source/JavaScriptCore/ChangeLog:6
> +	   Enable CSS_SCROLL_SNAP on iPhoneOS and simulator.

It seems odd to do this in the same commit.

> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:41
> +SnapPositionList::SnapPositionList()
> +{
> +}

Can probably remove this constructor

> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:43
> +SnapPositionList::SnapPositionList(Vector<LayoutUnit> snapPositions)

You're copying a Vector by value here.

> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:51
> +size_t SnapPositionList::size() const
> +{
> +    return positions.size();
> +}

Should be inline

> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:66
> +void SnapPositionList::clear()
> +{
> +    positions.clear();
> +}
> +
> +void SnapPositionList::resize(size_t size)
> +{
> +    positions.resize(size);
> +}
> +
> +void SnapPositionList::append(LayoutUnit position)
> +{
> +    positions.append(position);
> +}

This is just exposing Vector functionality. Maybe you don't need
SnapPositionList at all?

> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:68
> +LayoutUnit SnapPositionList::closestSnapPosition(LayoutUnit
scrollDestination, float velocity) const

This could either be a static function or a function on ScrollSnap"Manager"?

> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:74
> +    if (scrollDestination >= positions.at(positions.size() - 1))
> +	   return positions.at(positions.size() - 1);

Vector has last(). You'd better check that size() > 0.

> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:80
> +	   middleIndex = (lowerIndex + upperIndex) / 2;

middleIndex should be declared here.

> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:89
> +	   if (scrollDestination < positions.at(middleIndex))
> +	       upperIndex = middleIndex;
> +	   else if (scrollDestination > positions.at(middleIndex))
> +	       lowerIndex = middleIndex;
> +	   else {
> +	       upperIndex = middleIndex;
> +	       lowerIndex = middleIndex;
> +	       break;
> +	   }

Maybe std::binary_search?

> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:96
> +    // If velocity is zero, then the user released without flicking, so we
simply snap to the closer point.

I don't think this comment is useful.

> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:97
> +    return scrollDestination - lowerSnapPosition <= upperSnapPosition -
scrollDestination ? lowerSnapPosition : upperSnapPosition;

Break out "scrollDestination - lowerSnapPosition <= upperSnapPosition -
scrollDestination" into a named variable so I can tell what it is.

> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:100
> +ScrollSnapManager::ScrollSnapManager(ScrollEventAxis axis)

Maybe the name should reflect that this "manager" only handles a single axis.

> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:148
> +void ScrollSnapManager::updateSnapDataFromFrameView(FrameView* frameView)
> +{
> +    if (RenderView* renderView = frameView->renderView()) {
> +	   HTMLElement* body = renderView->document().body();
> +	   if (body) {
> +	       const RenderStyle& style = body->renderer()->style();
> +	       LayoutUnit size = m_axis == ScrollEventAxis::Horizontal ?
renderView->width() : renderView->height();
> +	       LayoutUnit scrollSize = m_axis == ScrollEventAxis::Horizontal ?
renderView->scrollWidth() : renderView->scrollHeight();
> +	       if (scrollSize <= 0 || size <= 0 || size >= scrollSize)
> +		   return;
> +
> +	       maxScrollOffset = scrollSize - size;
> +	       if ((m_axis == ScrollEventAxis::Horizontal &&
!style.scrollSnapUsesElementsX())
> +		   || (m_axis == ScrollEventAxis::Vertical &&
!style.scrollSnapUsesElementsY()))
> +		   updateSnapDataFromStyle(style, size);
> +	       else
> +		   updateSnapDataFromStyleAndParentElement(style, body, size);
> +	   }
> +    }
> +}
> +
> +void ScrollSnapManager::updateSnapDataFromParentElement(HTMLElement* parent)

> +{
> +    if (RenderBox* parentBox = parent->renderBox()) {
> +	   const RenderStyle& style = parentBox->style();
> +	   LayoutUnit size = m_axis == ScrollEventAxis::Horizontal ?
parentBox->width() : parentBox->height();
> +	   LayoutUnit scrollSize = m_axis == ScrollEventAxis::Horizontal ?
parentBox->scrollWidth() : parentBox->scrollHeight();
> +	   if (scrollSize <= 0 || size <= 0 || size >= scrollSize)
> +	       return;
> +
> +	   maxScrollOffset = scrollSize - size;
> +	   if ((m_axis == ScrollEventAxis::Horizontal &&
!style.scrollSnapUsesElementsX())
> +	       || (m_axis == ScrollEventAxis::Vertical &&
!style.scrollSnapUsesElementsY()))
> +	       updateSnapDataFromStyle(style, size);
> +	   else
> +	       updateSnapDataFromStyleAndParentElement(style, parent, size);
> +    }
> +}

The point of ScrollableArea is to abstract out differences between different
scrollable things, so you should re-write this in terms of ScrollableArea, and
not have different function for FrameView and overflow.

> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:154
> +    LayoutUnit parentOffset = m_axis == ScrollEventAxis::Horizontal ?
parent->renderBox()->offsetLeft() : parent->renderBox()->offsetTop();

We try not to use offset* in our own code. It usually indicates code that
failed to consider CSS transforms. I don' think you need parentOffset at all
here.

> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:160
> +    while (child && lastPotentialSnapPosition < maxScrollOffset) {

This walking of the nodes under the "elements" scroller could be very
expensive. I think it would be better to somehow register RenderObjects with
scrollSnapCoordinates() with their enclosing scroller.

> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:164
> +	       LayoutUnit childOffset = (m_axis == ScrollEventAxis::Horizontal
? childBox->offsetLeft() : childBox->offsetTop()) - parentOffset;

This should use localToContainerPoint() or similar. You should test content
with CSS transforms between the scrolling thing and the target elements.

> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:178
> +    // Always put a snap point on the maximum scroll offset.

Is that behavior defined in the spec?

> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:183
> +void ScrollSnapManager::updateSnapDataFromStyle(const RenderStyle& style,
LayoutUnit size)

It's unclear what the "size" param refers to.

> Source/WebCore/page/scrolling/ScrollSnapManager.cpp:202
> +    LayoutUnit curSnapPositionShift = -destinationOffset;

Not clear why.

> Source/WebCore/page/scrolling/ScrollSnapManager.h:42
> +#include <wtf/Vector.h>
> +#include <wtf/text/CString.h>
> +#include <wtf/text/WTFString.h>

I don't think you need these.

> Source/WebCore/page/scrolling/ScrollSnapManager.h:58
> +class ScrollSnapManager {

http://blog.codinghorror.com/i-shall-call-it-somethingmanager/

> Source/WebCore/page/scrolling/ScrollSnapManager.h:66
> +    LayoutUnit maxScrollOffset;

m_maxScrollOffset

> Source/WebCore/page/scrolling/ScrollSnapManager.h:67
> +    SnapPositionList snapPositions;

m_snapPositions.

I think both these should be private.