[webkit-reviews] review denied: [Bug 135268] Compute and store snap point positions : [Attachment 236310] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Aug 8 16:21:37 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 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.
More information about the webkit-reviews
mailing list