[Webkit-unassigned] [Bug 134301] Implement parsing for CSS scroll snap points

[bugzilla-daemon at webkit.org]

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


Dean Jackson <dino at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #235954|commit-queue-               |commit-queue?
               Flag|                            |




--- Comment #55 from Dean Jackson <dino at apple.com>  2014-08-06 13:49:13 PST ---
(From update of attachment 235954)
View in context: https://bugs.webkit.org/attachment.cgi?id=235954&action=review

This looks good. If you upload a patch with the changes I'll mark for cq.

> Source/WebCore/ChangeLog:18
> +        * CMakeLists.txt: Added StyleScrollSnapPoints.h, StyleScrollSnapPoints.cpp
> +        * Configurations/FeatureDefines.xcconfig: Added ENABLE_CSS_SCROLL_SNAP
> +        * WebCore.vcxproj/WebCore.vcxproj: Added StyleScrollSnapPoints.h, StyleScrollSnapPoints.cpp
> +        * WebCore.vcxproj/WebCore.vcxproj.filters: Added StyleScrollSnapPoints.h, StyleScrollSnapPoints.cpp
> +        * WebCore.xcodeproj/project.pbxproj: Added StyleScrollSnapPoints.h, StyleScrollSnapPoints.cpp, LengthRepeat.h

No need to change this now, but in the future I think it is ok to just say "Added new files"

> Source/WebCore/WebCore.vcxproj/WebCore.vcxproj:20386
> +    <ClInclude Include="..\rendering\style\StyleScrollSnapPoints.h" />

What about LengthRepeat.h?

> Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters:9917
> +    <ClInclude Include="..\rendering\style\StyleScrollSnapPoints.h">
> +      <Filter>rendering\style</Filter>
> +    </ClInclude>

What about LengthRepeat.h?

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1100
> +    auto snapDestinationValue = CSSValueList::createSpaceSeparated();
> +    if (x.isPercentNotCalculated())
> +        snapDestinationValue.get().append(cssValuePool().createValue(x.percent(), CSSPrimitiveValue::CSS_PERCENTAGE));
> +    else
> +        snapDestinationValue.get().append(zoomAdjustedPixelValue(valueForLength(x, 0), style));
> +
> +    if (y.isPercentNotCalculated())
> +        snapDestinationValue.get().append(cssValuePool().createValue(y.percent(), CSSPrimitiveValue::CSS_PERCENTAGE));
> +    else
> +        snapDestinationValue.get().append(zoomAdjustedPixelValue(valueForLength(y, 0), style));
> +

I could be the odd one out here, but you can replace all the snapDestinationValue.get().append calls with just snapDestinationValue->append, which looks cleaner.... but maybe only if you use RefPtr<CSSValueList> rather than "auto".

The same goes for snapPointsValue and snapCoordinatesValue in the other new functions.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1108
> +    for (auto& point : points)
> +        snapPointsValue.get().append(point.isPercentNotCalculated() ? cssValuePool().createValue(point.percent(), CSSPrimitiveValue::CSS_PERCENTAGE) : zoomAdjustedPixelValue(valueForLength(point, 0), style));

In scrollSnapDestination/scrollSnapCoordinates you do this with an if/else statement rather than a ternary operator, which I think is easier to read.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1110
> +        snapPointsValue.get().append(cssValuePool().createValue(LengthRepeat::create(repeatPoint.isPercentNotCalculated() ? cssValuePool().createValue(repeatPoint.percent(), CSSPrimitiveValue::CSS_PERCENTAGE) : zoomAdjustedPixelValue(valueForLength(repeatPoint, 0), style))));

Ditto.

> Source/WebCore/css/StyleResolver.cpp:2869
> +        if (primitiveValue && primitiveValue->getValueID() == CSSValueElements) {
> +            if (id == CSSPropertyWebkitScrollSnapPointsX)
> +                renderStyle->setScrollSnapUsesElementsX(true);
> +            else
> +                renderStyle->setScrollSnapUsesElementsY(true);
> +        } else {

Not important, but maybe you could early return at the end of the if, and then avoid the else block.

> Source/WebCore/css/StyleResolver.cpp:2874
> +            if (id == CSSPropertyWebkitScrollSnapPointsX)
> +                renderStyle->setScrollSnapUsesElementsX(false);
> +            else
> +                renderStyle->setScrollSnapUsesElementsY(false);

Should this test be here? I think you've added it accidentally. This code is only called if we're not CSSValueElements, and down below you're testing for scroll snap points.

> Source/WebCore/css/StyleResolver.cpp:2898
> +            if (id == CSSPropertyWebkitScrollSnapPointsX)
> +                renderStyle->setScrollSnapOffsetsX(points);
> +            else
> +                renderStyle->setScrollSnapOffsetsY(points);

I think this should go up before the loop over CSSValueList.

> LayoutTests/platform/mac/css3/scroll-snap/scroll-snap-property-computed-style.html:4
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">
> +<html>
> +<head>
> +<script src="../../../../resources/js-test-pre.js"></script>

Is there a reason these tests are in platform/mac?

Can't we make the test content platform neutral?

> LayoutTests/platform/mac/css3/scroll-snap/scroll-snap-property-computed-style.js:4
> +var stylesheet, filterStyle, subRule;

I don't think you use filterStyle or subRule.

> LayoutTests/platform/mac/css3/scroll-snap/scroll-snap-property-parsing.js:4
> +var stylesheet, cssRule, declaration, filterRule, subRule;

Same here. filterRule and subRule are not used.

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