Bug 134301 – Implement parsing for CSS scroll snap points

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Wenson Hsieh]

Snap point properties in CSS should be recognized and parsed, such that snap point information is stored in the RenderStyle. Does not cover changes in the behavior of elements. Relevant changes should be enclosed in #if ENABLE(CSS_SCROLL_SNAP).

This is the first sub-issue of https://bugs.webkit.org/show_bug.cgi?id=134283.

[Wenson Hsieh]

Created attachment 233827 [details]
Patch

[Wenson Hsieh]

Created attachment 233828 [details]
Patch

[Wenson Hsieh]

(In reply to comment #2)
> Created an attachment (id=233828) [details]
> Patch

It's a bit rough around the edges, but I'll most likely be refactoring parts of the code while I work on the next sub-issue anyways. In particular, there's some repeated code in DeprecatedStyleBuilder.cpp that I could probably fix by using templates.

[Wenson Hsieh]

Created attachment 233835 [details]
Patch

[Wenson Hsieh]

Created attachment 233847 [details]
Patch

[Wenson Hsieh]

Created attachment 233850 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 233850 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=233850&action=review

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1086
> +

No blank line at the start of the function.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1087
> +    PassRef<CSSValueList> dst = CSSValueList::createSpaceSeparated();

This should be RefPtr<>, but you can also use "auto". Please don't use abbreviated variable names like "dst". Maybe snapDestination or snapDestinationValue.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1098
> +
> +    PassRef<CSSValueList> pointList = CSSValueList::createSpaceSeparated();

Ditto.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1108
> +        snprintf(rs, 7, "%f", repeatPoint.value);

No snprintf! Use String/StringBuilder functions.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1122
> +static PassRef<CSSValueList> getScrollSnapCoordinates(Vector<SnapPoint> coords)

const Vector<>&

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1124
> +    PassRef<CSSValueList> allCoordinates = CSSValueList::createCommaSeparated();

RefPtr (or auto).

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3097
> +            bool hasRepeat = style->scrollSnapHasRepeatX();

No need for the local variable.

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2475
> +        // every x must be followed by a y

Comments should have sentence case and a period at the end.

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2479
> +        if (!(pointCount & 0x1))
> +            pointCount /= 2;
> +        else
> +            return;

If (pointCount & 0x1)
  return;

> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:71
> +    bool elementsX;
> +    bool elementsY;

The name is confusing; "elementsX" doesn't say why this is boolean.

> LayoutTests/platform/mac/css3/scroll-snap/parse-scroll-snap.html:33
> +        <script type="text/javascript">
> +            function testComputedStyleProperty(elemName, property, expected) {

This should be a "dumpAsText()" test. I think you should use js-pre/js-post like other similar tests.

[Wenson Hsieh]

Created attachment 233885 [details]
Patch

[Wenson Hsieh]

Comment on attachment 233850 [details]
Patch

Thanks again for the review! I've worked to address these issues on the new patch.

[Wenson Hsieh]

Created attachment 233902 [details]
Patch

[zalan]

Comment on attachment 233902 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=233902&action=review

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3084
> +            SnapPoint x = style->scrollSnapDestX();
> +            SnapPoint y = style->scrollSnapDestY();

no need for these local variables. function names clearly define what they are.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3097
> +            SnapPoint repeatPoint = style->scrollSnapRepeatY();
> +            bool hasRepeat = style->scrollSnapHasRepeatY();

same here.

> Source/WebCore/css/CSSParser.cpp:3129
> +        bool doAppend = false;
> +        RefPtr<CSSValue> parsedValue;
> +        if (val->unit == CSSPrimitiveValue::CSS_PERCENTAGE) {
> +            parsedValue = CSSPrimitiveValue::create(val->fValue, CSSPrimitiveValue::CSS_PERCENTAGE);
> +            doAppend = true;
> +        } else if (val->unit == CSSPrimitiveValue::CSS_NUMBER) {
> +            parsedValue = CSSPrimitiveValue::create(val->fValue, CSSPrimitiveValue::CSS_NUMBER);
> +            doAppend = true;
> +        } else if (val->unit == CSSPrimitiveValue::CSS_PX) {
> +            parsedValue = CSSPrimitiveValue::create(val->fValue, CSSPrimitiveValue::CSS_PX);
> +            doAppend = true;
> +        }
> +        if (doAppend)

there's no way to figure out by looking at the parsedValue whether it has a value that needs to be appended? using doAppends is highly error-prone.

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2378
> +        styleResolver->style()->setScrollSnapRepeatY(SnapPoint(100, true));

Can't we use some enums instead of these magic values? What's 100? px? %?

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2434
> +        styleResolver->style()->setScrollSnapDestX(SnapPoint(0, true));
> +        styleResolver->style()->setScrollSnapDestY(SnapPoint(0, true));

same here.

> Source/WebCore/rendering/style/StyleScrollSnapPoints.cpp:22
> +#include "StyleScrollSnapPoints.h"

missing #if ENABLE(CSS_SCROLL_SNAP)

> Source/WebCore/rendering/style/StyleScrollSnapPoints.cpp:36
> +    , destX(SnapPoint(0, false))
> +    , destY(SnapPoint(0, false))

please use destination*

> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:22
> +#define StyleScrollSnapPoints_h

missing #if ENABLE(CSS_SCROLL_SNAP)

> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:33
> +    bool isRelative;

members are preferred to be at the bottom.

> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:36
> +        , isRelative(isRel)

isPercentage? ("relative" has multiple meanings when it is associated with coordinates. it might be better to use isPercentage unless you follow some css standard coding style here.)

> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:47
> +};

keep in mind that this float value needs to be converted (and most likely pixel snapped) to LayoutUnit when you do the actual scroll pos snapping.

[Wenson Hsieh]

Comment on attachment 233902 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=233902&action=review

Thanks for the review!

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3084
>> +            SnapPoint y = style->scrollSnapDestY();
> 
> no need for these local variables. function names clearly define what they are.

Fixed -- thanks!

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3097
>> +            bool hasRepeat = style->scrollSnapHasRepeatY();
> 
> same here.

Fixed.

>> Source/WebCore/css/CSSParser.cpp:3129
>> +        if (doAppend)
> 
> there's no way to figure out by looking at the parsedValue whether it has a value that needs to be appended? using doAppends is highly error-prone.

Good point -- fixed.

>> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2378
>> +        styleResolver->style()->setScrollSnapRepeatY(SnapPoint(100, true));
> 
> Can't we use some enums instead of these magic values? What's 100? px? %?

Fixed -- SnapPoint constructor will accept enums indicating default values.

>> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2434
>> +        styleResolver->style()->setScrollSnapDestY(SnapPoint(0, true));
> 
> same here.

Fixed -- thanks!

>> Source/WebCore/rendering/style/StyleScrollSnapPoints.cpp:22
>> +#include "StyleScrollSnapPoints.h"
> 
> missing #if ENABLE(CSS_SCROLL_SNAP)

Wrapped header in #if -- thanks.

>> Source/WebCore/rendering/style/StyleScrollSnapPoints.cpp:36
>> +    , destY(SnapPoint(0, false))
> 
> please use destination*

Renamed variables -- thanks

>> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:22
>> +#define StyleScrollSnapPoints_h
> 
> missing #if ENABLE(CSS_SCROLL_SNAP)

Fixed.

>> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:33
>> +    bool isRelative;
> 
> members are preferred to be at the bottom.

Moved members below methods

>> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:36
>> +        , isRelative(isRel)
> 
> isPercentage? ("relative" has multiple meanings when it is associated with coordinates. it might be better to use isPercentage unless you follow some css standard coding style here.)

Renamed to isPercentage

>> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:47
>> +};
> 
> keep in mind that this float value needs to be converted (and most likely pixel snapped) to LayoutUnit when you do the actual scroll pos snapping.

Thanks for the tip -- I think I'll keep it as a float for now, but I might change this when I implement the actual snapping behavior.

[Wenson Hsieh]

Created attachment 233911 [details]
Patch

[Build Bot]

Comment on attachment 233911 [details]
Patch

Attachment 233911 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6268282512343040

New failing tests:
media/W3C/video/networkState/networkState_during_loadstart.html

[Build Bot]

Created attachment 233921 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Wenson Hsieh]

Created attachment 233930 [details]
Patch

[Dean Jackson]

Comment on attachment 233930 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=233930&action=review

You should add the new keywords to Source/WebInspectorUI/UserInterface/Models/CSSKeywordCompletions.js

Technically you're not testing the parsing - you're testing the parsing and the computed style. I think they should be separate tests.
See LayoutTests/css3/filters/script-tests/filter-property-parsing.js for a parsing test (the HTML is in the parent dir)
and filter-property-parsing-invalid.js and filter-property-computed-style.js

Otherwise it is very close.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1099
> +    for (int i = 0; i < (int)points.size(); i++) {
> +        SnapPoint point = points.at(i);

I believe this can be a C++11 style loop, since you never use "i" again.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1121
> +        auto curCoordinate = CSSValueList::createSpaceSeparated();

Call this currentCoordinate. (or currentCoord, since you use the coord shortening elsewhere)

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3084
> +        case CSSPropertyWebkitScrollSnapDestination: {
> +            return getScrollSnapDestination(style->scrollSnapDestinationX(), style->scrollSnapDestinationY());
> +        }

No need for braces.

> Source/WebCore/css/CSSParser.cpp:1026
> +    case CSSPropertyWebkitScrollSnapPointsX: // elements | <length>

What about "| <length>? repeat(<length>)" in the comment?

> Source/WebCore/css/CSSParser.cpp:3126
> +        if (val->unit == CSSPrimitiveValue::CSS_PERCENTAGE) {
> +            parsedValue = CSSPrimitiveValue::create(val->fValue, CSSPrimitiveValue::CSS_PERCENTAGE);
> +            values->append(parsedValue.release());
> +        } else if (val->unit == CSSPrimitiveValue::CSS_NUMBER) {
> +            parsedValue = CSSPrimitiveValue::create(val->fValue, CSSPrimitiveValue::CSS_NUMBER);
> +            values->append(parsedValue.release());
> +        } else if (val->unit == CSSPrimitiveValue::CSS_PX) {
> +            parsedValue = CSSPrimitiveValue::create(val->fValue, CSSPrimitiveValue::CSS_PX);
> +            values->append(parsedValue.release());

All three of these are identical, so combine them into one if clause.
parsedValue = CSSPrimitiveValue::create(val->fValue, val->unit);
values->append(parsedValue.release());

> Source/WebCore/css/CSSParser.cpp:3138
> +                if (parserVal->unit == CSSPrimitiveValue::CSS_PERCENTAGE)
> +                    parsedValue = CSSPrimitiveValue::create(parserVal->fValue, CSSPrimitiveValue::CSS_PERCENTAGE);
> +                else if (parserVal->unit == CSSPrimitiveValue::CSS_NUMBER)
> +                    parsedValue = CSSPrimitiveValue::create(parserVal->fValue, CSSPrimitiveValue::CSS_NUMBER);
> +                else if (parserVal->unit == CSSPrimitiveValue::CSS_PX)
> +                    parsedValue = CSSPrimitiveValue::create(parserVal->fValue, CSSPrimitiveValue::CSS_PX);

Same here.

> Source/WebCore/css/CSSParser.cpp:3142
> +                RefPtr<CSSValue> repeatFlag = CSSPrimitiveValue::create("repeat", CSSPrimitiveValue::CSS_STRING);
> +                values->append(repeatFlag.release());

No need to create the local variable, just call CSSPrimitiveValue::create as a parameter.

> Source/WebCore/css/CSSParser.cpp:3175
> +        if (curParserVal && curParserVal->unit == CSSPrimitiveValue::CSS_PERCENTAGE) {
> +            float x = curParserVal->fValue;
> +            xpos = CSSPrimitiveValue::create(x, CSSPrimitiveValue::CSS_PERCENTAGE);
> +        } else if (curParserVal && curParserVal->unit == CSSPrimitiveValue::CSS_NUMBER) {
> +            float x = curParserVal->fValue;
> +            xpos = CSSPrimitiveValue::create(x, CSSPrimitiveValue::CSS_NUMBER);
> +        } else if (curParserVal && curParserVal->unit == CSSPrimitiveValue::CSS_PX) {
> +            float x = curParserVal->fValue;
> +            xpos = CSSPrimitiveValue::create(x, CSSPrimitiveValue::CSS_PX);

And again here.

> Source/WebCore/css/CSSParser.cpp:3188
> +        if (curParserVal && curParserVal->unit == CSSPrimitiveValue::CSS_PERCENTAGE) {
> +            float y = curParserVal->fValue;
> +            ypos = CSSPrimitiveValue::create(y, CSSPrimitiveValue::CSS_PERCENTAGE);
> +        } else if (curParserVal && curParserVal->unit == CSSPrimitiveValue::CSS_NUMBER) {
> +            float y = curParserVal->fValue;
> +            ypos = CSSPrimitiveValue::create(y, CSSPrimitiveValue::CSS_NUMBER);
> +        } else if (curParserVal && curParserVal->unit == CSSPrimitiveValue::CSS_PX) {
> +            float y = curParserVal->fValue;
> +            ypos = CSSPrimitiveValue::create(y, CSSPrimitiveValue::CSS_PX);

And here.

> Source/WebCore/css/CSSParser.cpp:3210
> +        // don't accept odd-length lists of coords

Nit: We always start comments with a capital letter, and end with a period.

> Source/WebCore/css/CSSParser.cpp:3237
> +        if (xparsed->unit == CSSPrimitiveValue::CSS_PERCENTAGE) {
> +            xval = xparsed->fValue;
> +            xpos = CSSPrimitiveValue::create(xval, CSSPrimitiveValue::CSS_PERCENTAGE);
> +            positions->append(xpos);
> +        } else if (xparsed->unit == CSSPrimitiveValue::CSS_NUMBER) {
> +            xval = xparsed->fValue;
> +            xpos = CSSPrimitiveValue::create(xval, CSSPrimitiveValue::CSS_NUMBER);
> +            positions->append(xpos);
> +        } else if (xparsed->unit == CSSPrimitiveValue::CSS_PX) {
> +            xval = xparsed->fValue;
> +            xpos = CSSPrimitiveValue::create(xval, CSSPrimitiveValue::CSS_PX);
> +            positions->append(xpos);
> +        } else { return false; }
> +        if (yparsed->unit == CSSPrimitiveValue::CSS_PERCENTAGE) {
> +            yval = yparsed->fValue;
> +            ypos = CSSPrimitiveValue::create(yval, CSSPrimitiveValue::CSS_PERCENTAGE);
> +            positions->append(ypos);
> +        } else if (yparsed->unit == CSSPrimitiveValue::CSS_NUMBER) {
> +            yval = yparsed->fValue;
> +            ypos = CSSPrimitiveValue::create(yval, CSSPrimitiveValue::CSS_NUMBER);
> +            positions->append(ypos);
> +        } else if (yparsed->unit == CSSPrimitiveValue::CSS_PX) {
> +            yval = yparsed->fValue;
> +            ypos = CSSPrimitiveValue::create(yval, CSSPrimitiveValue::CSS_PX);
> +            positions->append(ypos);

And again :)

Also, your else clause should be on a separate line without the braces.

else
    return false;

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2333
> +            // handle "elements"

Nit: Comment style is wrong, but I think you can just remove this completely.

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2341
> +            for (int i = 0; i < (int)valueList.length(); i++) {
> +                RefPtr<CSSValue> rawValue = valueList.item(i);

Another case where you can use C++11 for (a : listA) style.

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2382
> +            // handle "elements"

Ditto on the comment.

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2390
> +            for (int i = 0; i < (int)valueList.length(); i++) {
> +                RefPtr<CSSValue> rawValue = valueList.item(i);

And the loop.

> Source/WebCore/rendering/style/StyleScrollSnapPoints.cpp:34
> +    , repeatPointX(SnapPoint(100, true)) // repeat(100%)
> +    , repeatPointY(SnapPoint(100, true)) // repeat(100%)

No need for these comments.

[Wenson Hsieh]

Comment on attachment 233930 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=233930&action=review

Thank you for reviewing my code! I've fixed the smaller issues, and I'll have new tests (separate ones for parsing/computed style) up soon.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1099
>> +        SnapPoint point = points.at(i);
> 
> I believe this can be a C++11 style loop, since you never use "i" again.

forgot I could do that :) this makes the syntax much cleaner. Thanks!

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1121
>> +        auto curCoordinate = CSSValueList::createSpaceSeparated();
> 
> Call this currentCoordinate. (or currentCoord, since you use the coord shortening elsewhere)

Fixed.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3084
>> +        }
> 
> No need for braces.

Fixed.

>> Source/WebCore/css/CSSParser.cpp:1026
>> +    case CSSPropertyWebkitScrollSnapPointsX: // elements | <length>
> 
> What about "| <length>? repeat(<length>)" in the comment?

Ah, just realized -- I actually don't need these 2 case statements here, since I handle SnapPointsX/Y elsewhere.

>> Source/WebCore/css/CSSParser.cpp:3126
>> +            values->append(parsedValue.release());
> 
> All three of these are identical, so combine them into one if clause.
> parsedValue = CSSPrimitiveValue::create(val->fValue, val->unit);
> values->append(parsedValue.release());

Ohh, just realized I need to cast val->unit to CSSPrimitiveValue::UnitTypes. I originally had CSSPrimitiveValue::create(val->fValue, val->unit), but got errors.

>> Source/WebCore/css/CSSParser.cpp:3138
>> +                    parsedValue = CSSPrimitiveValue::create(parserVal->fValue, CSSPrimitiveValue::CSS_PX);
> 
> Same here.

Fixed!

>> Source/WebCore/css/CSSParser.cpp:3142
>> +                values->append(repeatFlag.release());
> 
> No need to create the local variable, just call CSSPrimitiveValue::create as a parameter.

Fixed.

>> Source/WebCore/css/CSSParser.cpp:3175
>> +            xpos = CSSPrimitiveValue::create(x, CSSPrimitiveValue::CSS_PX);
> 
> And again here.

Fixed.

>> Source/WebCore/css/CSSParser.cpp:3188
>> +            ypos = CSSPrimitiveValue::create(y, CSSPrimitiveValue::CSS_PX);
> 
> And here.

Fixed.

>> Source/WebCore/css/CSSParser.cpp:3210
>> +        // don't accept odd-length lists of coords
> 
> Nit: We always start comments with a capital letter, and end with a period.

Got it -- fixed!

>> Source/WebCore/css/CSSParser.cpp:3237
>> +            positions->append(ypos);
> 
> And again :)
> 
> Also, your else clause should be on a separate line without the braces.
> 
> else
>     return false;

Fixed.

>> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2333
>> +            // handle "elements"
> 
> Nit: Comment style is wrong, but I think you can just remove this completely.

Fixed.

>> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2341
>> +                RefPtr<CSSValue> rawValue = valueList.item(i);
> 
> Another case where you can use C++11 for (a : listA) style.

Hmm..I got a compiler error using that syntax. I'm not sure CSSValueList supports range expressions

>> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2382
>> +            // handle "elements"
> 
> Ditto on the comment.

Fixed.

>> Source/WebCore/rendering/style/StyleScrollSnapPoints.cpp:34
>> +    , repeatPointY(SnapPoint(100, true)) // repeat(100%)
> 
> No need for these comments.

Fixed.

[Wenson Hsieh]

Created attachment 233995 [details]
Patch

[Wenson Hsieh]

Created attachment 233998 [details]
Patch

[Build Bot]

Comment on attachment 233998 [details]
Patch

Attachment 233998 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5596848395911168

New failing tests:
platform/mac/css3/scroll-snap/scroll-snap-property-computed-style.html

[Build Bot]

Created attachment 234007 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Wenson Hsieh]

Created attachment 234011 [details]
Patch

[Wenson Hsieh]

Created attachment 234032 [details]
Patch

[Tim Horton]

Comment on attachment 234032 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=234032&action=review

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1121
> +    for (int i = 0; i < (int)coords.size() / 2; i++) {

I think this should be size_t, no? (and in other places too)

> Source/WebCore/css/CSSParser.cpp:3180
> +        CSSParserValue* xparsed = m_valueList->current();

These names aren't camel case (and I'm not sure they're the best names for the job).

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2331
> +        styleResolver->style()->setScrollSnapHasRepeatX(false);

styleResolver->style() over and over; keep it in a local?

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2387
> +            for (int i = 0; i < (int)valueList.length(); i++) {

size_t again.

> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:85
> +    bool hasRepeatX;

I'm surprised smfr didn't complain about member packing here :D In any case, I think this could be packed better (though I don't think there will be many of these in the common case?).

[Wenson Hsieh]

Comment on attachment 234032 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=234032&action=review

Thanks for reviewing my patch! I'll put up a new patch up soon.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1121
>> +    for (int i = 0; i < (int)coords.size() / 2; i++) {
> 
> I think this should be size_t, no? (and in other places too)

Fixed -- good catch!

>> Source/WebCore/css/CSSParser.cpp:3180
>> +        CSSParserValue* xparsed = m_valueList->current();
> 
> These names aren't camel case (and I'm not sure they're the best names for the job).

Perhaps parsedValueX or parsedXValue work better?

>> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2331
>> +        styleResolver->style()->setScrollSnapHasRepeatX(false);
> 
> styleResolver->style() over and over; keep it in a local?

Good point -- fixed.

>> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2387
>> +            for (int i = 0; i < (int)valueList.length(); i++) {
> 
> size_t again.

Fixed.

>> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:85
>> +    bool hasRepeatX;
> 
> I'm surprised smfr didn't complain about member packing here :D In any case, I think this could be packed better (though I don't think there will be many of these in the common case?).

Right -- the snap scroll style objects should be relatively rare. The 4 bool flags could probably be condensed into single bits right off the bat.

[Wenson Hsieh]

Created attachment 234048 [details]
Patch

[Sam Weinig]

Comment on attachment 234048 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=234048&action=review

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1100
> +        snapPointsValue.get().append(cssValuePool().createValue(point.value, type));

You can get rid of the .get() if you change snapPointsValue from auto to Ref<CSSValueList>.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1105
> +        repeatStrBuilder.append("repeat(", 7);

This should use StringBuilder::appendLiteral(...)

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1106
> +        repeatStrBuilder.append(String::numberToStringFixedWidth((double) repeatPoint.value, 0));

If the cast to double is necessary, you should use a static_cast.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1108
> +            repeatStrBuilder.append("%%", 1);

This should use StringBuilder::appendLiteral(...)

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1110
> +            repeatStrBuilder.append("px", 2);

This should use StringBuilder::appendLiteral(...)

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1111
> +        repeatStrBuilder.append(")", 1);

This should use StringBuilder::appendLiteral(...)

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1118
> +static PassRef<CSSValueList> getScrollSnapCoordinates(const Vector<SnapPoint>& coords)

Can we pass this in / store these as a Vector<std::pair<SnapPoint, SnapPoint>>.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1126
> +        currentCoord.get().append(cssValuePool().createValue(point.value, type));

You can get rid of the .get() if you change currentCoord from auto to Ref<CSSValueList>.

> Source/WebCore/css/CSSParser.cpp:3115
> +        RefPtr<CSSValue> parsedValue;
> +        if (val->unit == CSSPrimitiveValue::CSS_PERCENTAGE 
> +            || val->unit == CSSPrimitiveValue::CSS_NUMBER
> +            || val->unit == CSSPrimitiveValue::CSS_PX) {
> +            parsedValue = CSSPrimitiveValue::create(val->fValue, static_cast<CSSPrimitiveValue::UnitTypes>(val->unit));
> +            values->append(parsedValue.release());

The spec says this should support <length>. What is the reason for supporting a reduced set.

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2320
> +    static void applyInheritValue(CSSPropertyID, StyleResolver* styleResolver)
> +    {
> +        UNUSED_PARAM(styleResolver);

There is no reason to use UNUSED_PARAM() here. Just don't declare the parameter name in the signature.

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2337
> +        else {
> +            renderStyle->setScrollSnapUsesElementsX(false);

You should assert that the CSSValue is a CSSValueList here.

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2339
> +            Vector<SnapPoint> points = Vector<SnapPoint>();

This can be written as Vector<SnapPoint> points;

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2341
> +            for (size_t i = 0; i < valueList.length(); i++) {

This should probably use a CSSValueListIterator

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2353
> +                else if (primitiveValue->isString() && primitiveValue->getStringValue() == "repeat")

It seems quite odd that this is a string compare.  We should instead store the repeat-ness in a CSSValue keyword.

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2370
> +    static void applyInheritValue(CSSPropertyID, StyleResolver* styleResolver)
> +    {
> +        UNUSED_PARAM(styleResolver);

There is no reason to use UNUSED_PARAM() here. Just don't declare the parameter name in the signature.

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2420
> +    static void applyInheritValue(CSSPropertyID, StyleResolver* styleResolver)
> +    {
> +        UNUSED_PARAM(styleResolver);

There is no reason to use UNUSED_PARAM() here. Just don't declare the parameter name in the signature.

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2452
> +    static void applyInheritValue(CSSPropertyID, StyleResolver* styleResolver)
> +    {
> +        UNUSED_PARAM(styleResolver);

There is no reason to use UNUSED_PARAM() here. Just don't declare the parameter name in the signature.

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2457
> +    static void applyInitialValue(CSSPropertyID, StyleResolver* styleResolver)
> +    {
> +        UNUSED_PARAM(styleResolver);

There is no reason to use UNUSED_PARAM() here. Just don't declare the parameter name in the signature.

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2467
> +        // Every x must be followed by a y.
> +        if (pointCount & 0x1)
> +            return;

This seems like an overly complicated way to say % 2.

> Source/WebCore/rendering/style/RenderStyle.h:1080
> +    Vector<SnapPoint> scrollSnapPointsX() const { return rareNonInheritedData->m_scrollSnapPoints->pointsX; }
> +    Vector<SnapPoint> scrollSnapPointsY() const { return rareNonInheritedData->m_scrollSnapPoints->pointsY; }

Do we really want to copy the Vector here?  If not, this should return a const Vector<SnapPoint>&.

> Source/WebCore/rendering/style/RenderStyle.h:1087
> +    Vector<SnapPoint> scrollSnapCoordinates() const { return rareNonInheritedData->m_scrollSnapPoints->coords; }

Do we really want to copy the Vector here?  If not, this should return a const Vector<SnapPoint>&.

> Source/WebCore/rendering/style/RenderStyleConstants.h:557
> +enum EScrollSnapType { ScrollSnapTypeNone = 0, ScrollSnapTypeProximity = 1, ScrollSnapTypeMandatory = 2};

The E prefix is unnecessary and this should be an enum class.

> Source/WebCore/rendering/style/StyleScrollSnapPoints.cpp:19
> +/*
> + * Copyright (C) 2014 Apple Inc. All rights reserved.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Library General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Library General Public License for more details.
> + *
> + * You should have received a copy of the GNU Library General Public License
> + * along with this library; see the file COPYING.LIB.  If not, write to
> + * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> + * Boston, MA 02110-1301, USA.
> + *
> + */

Please use the BSD license.

> Source/WebCore/rendering/style/StyleScrollSnapPoints.cpp:26
> +#include <wtf/text/WTFString.h>

This is already included in the header.

> Source/WebCore/rendering/style/StyleScrollSnapPoints.cpp:41
> +    , pointsX(Vector<SnapPoint>())
> +    , pointsY(Vector<SnapPoint>())
> +    , coords(Vector<SnapPoint>())

These don't require explicit initialization.

> Source/WebCore/rendering/style/StyleScrollSnapPoints.cpp:46
> +    : RefCounted<StyleScrollSnapPoints>()

This shouldn't require explicit initialization.

> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:19
> +/*
> + * Copyright (C) 2014 Apple Inc. All rights reserved.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Library General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Library General Public License for more details.
> + *
> + * You should have received a copy of the GNU Library General Public License
> + * along with this library; see the file COPYING.LIB.  If not, write to
> + * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> + * Boston, MA 02110-1301, USA.
> + *
> + */

Please use the BSD license.

> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:36
> +enum SnapPointConstant {
> +    DefaultRepeat,
> +    DefaultDestination,
> +};

This should be an enum class.

> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:38
> +struct SnapPoint {

I'm not too familiar with the terminology, but we usually think of a point as something with at least an X and Y.  Is this something the spec names?  If not, perhaps we can rename this to something more suitable (SnapValue? SnapLength?).

> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:43
> +    SnapPoint(float val, bool isPct)
> +        : value(val)
> +        , isPercentage(isPct)
> +    {
> +    }

The spec indicates that a these values should of type <length>. Why are we limiting it?

> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:44
> +    SnapPoint(SnapPointConstant snapConstant)

This should be an explicit constructor.

[Wenson Hsieh]

Comment on attachment 234048 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=234048&action=review

Thanks for the review! I fixed most of the smaller issues, and I'll put up a new version of the patch as soon as I add support for the different types of <length>s.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1105
>> +        repeatStrBuilder.append("repeat(", 7);
> 
> This should use StringBuilder::appendLiteral(...)

Got it -- fixed!

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1106
>> +        repeatStrBuilder.append(String::numberToStringFixedWidth((double) repeatPoint.value, 0));
> 
> If the cast to double is necessary, you should use a static_cast.

Removed double cast, good catch.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1108
>> +            repeatStrBuilder.append("%%", 1);
> 
> This should use StringBuilder::appendLiteral(...)

Fixed.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1110
>> +            repeatStrBuilder.append("px", 2);
> 
> This should use StringBuilder::appendLiteral(...)

Fixed.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1111
>> +        repeatStrBuilder.append(")", 1);
> 
> This should use StringBuilder::appendLiteral(...)

Fixed.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1118
>> +static PassRef<CSSValueList> getScrollSnapCoordinates(const Vector<SnapPoint>& coords)
> 
> Can we pass this in / store these as a Vector<std::pair<SnapPoint, SnapPoint>>.

Good point -- this makes a lot more sense than storing them in a big list. Fixed.

>> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2320
>> +        UNUSED_PARAM(styleResolver);
> 
> There is no reason to use UNUSED_PARAM() here. Just don't declare the parameter name in the signature.

Fixed.

>> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2337
>> +            renderStyle->setScrollSnapUsesElementsX(false);
> 
> You should assert that the CSSValue is a CSSValueList here.

Added ASSERT check.

>> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2339
>> +            Vector<SnapPoint> points = Vector<SnapPoint>();
> 
> This can be written as Vector<SnapPoint> points;

Fixed -- thanks!

>> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2341
>> +            for (size_t i = 0; i < valueList.length(); i++) {
> 
> This should probably use a CSSValueListIterator

Fixed -- thanks!

>> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2370
>> +        UNUSED_PARAM(styleResolver);
> 
> There is no reason to use UNUSED_PARAM() here. Just don't declare the parameter name in the signature.

Fixed -- thanks!

>> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2420
>> +        UNUSED_PARAM(styleResolver);
> 
> There is no reason to use UNUSED_PARAM() here. Just don't declare the parameter name in the signature.

Fixed -- thanks!

>> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2452
>> +        UNUSED_PARAM(styleResolver);
> 
> There is no reason to use UNUSED_PARAM() here. Just don't declare the parameter name in the signature.

Fixed -- thanks!

>> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2457
>> +        UNUSED_PARAM(styleResolver);
> 
> There is no reason to use UNUSED_PARAM() here. Just don't declare the parameter name in the signature.

Fixed -- thanks!

>> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2467
>> +            return;
> 
> This seems like an overly complicated way to say % 2.

Good catch -- changed to % 2.

>> Source/WebCore/rendering/style/RenderStyle.h:1080
>> +    Vector<SnapPoint> scrollSnapPointsY() const { return rareNonInheritedData->m_scrollSnapPoints->pointsY; }
> 
> Do we really want to copy the Vector here?  If not, this should return a const Vector<SnapPoint>&.

Fixed -- thanks!

>> Source/WebCore/rendering/style/RenderStyle.h:1087
>> +    Vector<SnapPoint> scrollSnapCoordinates() const { return rareNonInheritedData->m_scrollSnapPoints->coords; }
> 
> Do we really want to copy the Vector here?  If not, this should return a const Vector<SnapPoint>&.

Fixed -- thanks!

>> Source/WebCore/rendering/style/RenderStyleConstants.h:557
>> +enum EScrollSnapType { ScrollSnapTypeNone = 0, ScrollSnapTypeProximity = 1, ScrollSnapTypeMandatory = 2};
> 
> The E prefix is unnecessary and this should be an enum class.

Changed to use enum class

>> Source/WebCore/rendering/style/StyleScrollSnapPoints.cpp:19
>> + */
> 
> Please use the BSD license.

Fixed with BSD license from http://opensource.org/licenses/BSD-3-Clause (hopefully this is the right one?)

>> Source/WebCore/rendering/style/StyleScrollSnapPoints.cpp:26
>> +#include <wtf/text/WTFString.h>
> 
> This is already included in the header.

Removed -- thanks!

>> Source/WebCore/rendering/style/StyleScrollSnapPoints.cpp:41
>> +    , coords(Vector<SnapPoint>())
> 
> These don't require explicit initialization.

Fixed -- thanks!

>> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:19
>> + */
> 
> Please use the BSD license.

Fixed with BSD license from http://opensource.org/licenses/BSD-3-Clause (hopefully this is the right one?)

>> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:36
>> +};
> 
> This should be an enum class.

Fixed -- thanks!

>> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:38
>> +struct SnapPoint {
> 
> I'm not too familiar with the terminology, but we usually think of a point as something with at least an X and Y.  Is this something the spec names?  If not, perhaps we can rename this to something more suitable (SnapValue? SnapLength?).

The spec doesn't explicitly say that a snap point is defined by both an x and a y, although in CSS having snapping behavior in one direction only requires scroll-snap-points-x or scroll-snap-points-y -- I suppose the other point is defined implicitly as repeat(100%). I agree, though; in either case, SnapLength is probably a better name.

>> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:44
>> +    SnapPoint(SnapPointConstant snapConstant)
> 
> This should be an explicit constructor.

Fixed.

[Simon Fraser (smfr)]

> > I'm not too familiar with the terminology, but we usually think of a point as something with at least an X and Y.  Is this something the spec names?  If not, perhaps we can rename this to something more suitable (SnapValue? SnapLength?).
> 
> The spec doesn't explicitly say that a snap point is defined by both an x and a y, although in CSS having snapping behavior in one direction only requires scroll-snap-points-x or scroll-snap-points-y -- I suppose the other point is defined implicitly as repeat(100%). I agree, though; in either case, SnapLength is probably a better name.

SnapLength makes it sound like a CSS Length. How about SnapLocation, or SnapOffset?

[Sam Weinig]

Also, instead of using DeprecatedStyleBuilder, the style building should use the old switch in StyleResolver.

[Wenson Hsieh]

Created attachment 234106 [details]
Patch

[Wenson Hsieh]

Created attachment 234119 [details]
Patch

[Wenson Hsieh]

Created attachment 234998 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 234998 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=234998&action=review

> Source/WebCore/ChangeLog:8
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Test: platform/mac/css3/scroll-snap/parse-scroll-snap.html

Please give a summary of your changes here with  reference to the spec.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:6251
> +		F47A5E3E195B8C8A00483100 /* StyleScrollSnapPoints.h in Headers */ = {isa = PBXBuildFile; fileRef = F47A5E3B195B8C8A00483100 /* StyleScrollSnapPoints.h */; settings = {ATTRIBUTES = (Private, ); }; };

Is there a reason this has to be a private header?

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1092
> +    if (y.isPercentNotCalculated())

Blank line above please.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1096
> +    return snapDestinationValue;

Ditto.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1102
> +    for (Length point : points) {

maybe auto& to avoid a copy.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1112
> +        RefPtr<CSSPrimitiveValue> primitiveValue = repeatPoint.isPercentNotCalculated() ? cssValuePool().createValue(repeatPoint.percent(), CSSPrimitiveValue::CSS_PERCENTAGE) : zoomAdjustedPixelValue((int)valueForLength(repeatPoint, 0), style);

You are throwing away fractional pixels here with the (int) cast. This seems wrong.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1127
> +    for (SnapCoordinate coordinate : coordinates) {

const auto& ?

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1133
> +            currentCoordinate.get().append(zoomAdjustedPixelValue((int)valueForLength(point, 0), style));

Another int cast

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1138
> +            currentCoordinate.get().append(zoomAdjustedPixelValue((int)valueForLength(point, 0), style));

And another.

> Source/WebCore/css/CSSParser.cpp:3060
> +    case CSSPropertyWebkitScrollSnapPointsX: // elements | <length>+ | (<length>+)? repeat(<length>)

I don't think the comments help here.

> Source/WebCore/css/CSSParser.cpp:3069
> +    case CSSPropertyWebkitScrollSnapDestination: // <length>{2}
> +        return parseScrollSnapDestination(propId, important);
> +    case CSSPropertyWebkitScrollSnapCoordinate: // (<length>{2}+)?

Ditto.

> Source/WebCore/css/CSSParser.cpp:3183
> +    RefPtr<CSSValueList> values = CSSValueList::createSpaceSeparated();
> +    while (CSSParserValue* val = m_valueList->current()) {

You're not handling "elements" in this function? If not you should add a comment to say it needs to be added.

> Source/WebCore/css/CSSParser.cpp:3195
> +                values->append(RefPtr<CSSValue>(CSSPrimitiveValue::create("repeat", CSSPrimitiveValue::CSS_STRING)).release());

Is the RefPtr<CSSValue>() required?

> Source/WebCore/css/CSSParser.cpp:3218
> +    if (m_valueList->size() == 2) {

You should reverse the check and early return.

> Source/WebCore/css/CSSParser.cpp:3223
> +        if (validUnit(curParserVal, FPercent | FLength))
> +            cssValueX = createPrimitiveNumericValue(curParserVal);
> +        else
> +            return false;

Better as if (!validUnit()) return false.

> Source/WebCore/css/CSSParser.cpp:3226
> +        if (validUnit(curParserVal, FPercent | FLength))

Same

> Source/WebCore/css/CSSParser.cpp:3246
> +        CSSParserValue* parsedValueX = m_valueList->current();
> +        m_valueList->next();
> +        CSSParserValue* parsedValueY = m_valueList->current();
> +        m_valueList->next();

I think it's a bad to call current() and next() without checking to see if you're at the end of the list.

> Source/WebCore/css/CSSParser.cpp:3250
> +        if (validUnit(parsedValueX, FPercent | FLength))

if (!validUnit)

> Source/WebCore/css/CSSPrimitiveValue.cpp:55
> +#include "StyleScrollSnapPoints.h"

Why do you need this #include?

> Source/WebCore/css/StyleResolver.cpp:2848
> +        CSSValueID scrollSnapId = primitiveValue->getValueID();
> +        if (scrollSnapId == CSSValueProximity)
> +            state.style()->setScrollSnapType(ScrollSnapType::Proximity);
> +        else if (scrollSnapId == CSSValueMandatory)
> +            state.style()->setScrollSnapType(ScrollSnapType::Mandatory);
> +        else
> +            state.style()->setScrollSnapType(ScrollSnapType::None);

Isn't this what CSSPrimitiveValue::operator ScrollSnapType() is for?

> Source/WebCore/css/StyleResolver.cpp:2855
> +        if (primitiveValue)
> +            renderStyle->setScrollSnapUsesElementsX(true);

Shouldn't you check that the value represents "elements"?

> Source/WebCore/css/StyleResolver.cpp:2864
> +                CSSPrimitiveValue* primitiveItemValue = toCSSPrimitiveValue(rawItemValue.get());
> +                if (primitiveItemValue->isString() && primitiveItemValue->getStringValue() == "repeat")

You don't need to compare strings here.  toCSSPrimitiveValue(value)->getValueID() == CSSValueRepeat

> Source/WebCore/css/StyleResolver.cpp:2896
> +            renderStyle->setScrollSnapUsesElementsY(false);
> +            bool expectRepeat = false;
> +            Vector<Length> points;
> +            for (CSSValueListIterator i(value); i.hasMore() && !renderStyle->scrollSnapHasRepeatY(); i.advance()) {
> +                RefPtr<CSSValue> rawValue = i.value();
> +                CSSPrimitiveValue* primitiveValue = toCSSPrimitiveValue(rawValue.get());
> +                if (primitiveValue->isString() && primitiveValue->getStringValue() == "repeat")
> +                    expectRepeat = true;
> +                else if (expectRepeat) {
> +                    renderStyle->setScrollSnapRepeatY(primitiveValue->convertToLength<FixedIntegerConversion | PercentConversion | FractionConversion | AutoConversion>(state.cssToLengthConversionData()));
> +                    renderStyle->setScrollSnapHasRepeatY(true);
> +                } else
> +                    points.append(primitiveValue->convertToLength<FixedIntegerConversion | PercentConversion | FractionConversion | AutoConversion>(state.cssToLengthConversionData()));
> +            }

You should share this code between X and Y.

> Source/WebCore/css/StyleResolver.cpp:2920
> +        for (size_t idx = 0; idx < pointCount; idx++) {

idx -> i

> Source/WebCore/rendering/style/RenderStyle.h:1096
> +    ScrollSnapType scrollSnapType() const { return static_cast<ScrollSnapType>(rareNonInheritedData->m_scrollSnapType); }
> +    Vector<Length> scrollSnapPointsX() const { return rareNonInheritedData->m_scrollSnapPoints->pointsX; }
> +    Vector<Length> scrollSnapPointsY() const { return rareNonInheritedData->m_scrollSnapPoints->pointsY; }
> +    Length scrollSnapRepeatX() const { return rareNonInheritedData->m_scrollSnapPoints->repeatPointX; }
> +    Length scrollSnapRepeatY() const { return rareNonInheritedData->m_scrollSnapPoints->repeatPointY; }
> +    bool scrollSnapHasRepeatX() const { return rareNonInheritedData->m_scrollSnapPoints->hasRepeatX; }
> +    bool scrollSnapHasRepeatY() const { return rareNonInheritedData->m_scrollSnapPoints->hasRepeatY; }
> +    Length scrollSnapDestinationX() const { return rareNonInheritedData->m_scrollSnapPoints->destinationX; }
> +    Length scrollSnapDestinationY() const { return rareNonInheritedData->m_scrollSnapPoints->destinationY; }
> +    Vector<SnapCoordinate> scrollSnapCoordinates() const { return rareNonInheritedData->m_scrollSnapPoints->coordinates; }
> +    bool scrollSnapUsesElementsX() const { return rareNonInheritedData->m_scrollSnapPoints->usesElementsX; }
> +    bool scrollSnapUsesElementsY() const { return rareNonInheritedData->m_scrollSnapPoints->usesElementsY; }

I wonder if we should make a class which you can share between X and Y

[Simon Fraser (smfr)]

Partial review; will finish later.

[Simon Fraser (smfr)]

Comment on attachment 234998 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=234998&action=review

> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:58
> +    static Length defaultRepeatPoint()
> +    {
> +        return Length(100, Percent);
> +    }
> +    
> +    static Length defaultDestinationPoint()
> +    {
> +        return Length(0, Fixed);
> +    }

Not sure I like the names having "point" since these don't use IntPoint/FloatPoint.

> LayoutTests/platform/mac/css3/scroll-snap/scroll-snap-property-computed-style-expected.txt:98
> +pixel/pixel destination : 10px 50px
> +PASS window.getComputedStyle(document.body).getPropertyValue('-webkit-scroll-snap-destination') is '10px 50px'

You should test fractional values here as well (10.5px, 12.234px)

[Wenson Hsieh]

Comment on attachment 234998 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=234998&action=review

Thank you for the review Simon! I've fixed all but one of the issues (parsing repeat as it's own separate unit, which I plan to get to a little later after I check in my second patch).

>> Source/WebCore/WebCore.xcodeproj/project.pbxproj:6251
>> +		F47A5E3E195B8C8A00483100 /* StyleScrollSnapPoints.h in Headers */ = {isa = PBXBuildFile; fileRef = F47A5E3B195B8C8A00483100 /* StyleScrollSnapPoints.h */; settings = {ATTRIBUTES = (Private, ); }; };
> 
> Is there a reason this has to be a private header?

Good point -- changed to project.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1092
>> +    if (y.isPercentNotCalculated())
> 
> Blank line above please.

Fixed. Definitely looks cleaner with the newline.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1096
>> +    return snapDestinationValue;
> 
> Ditto.

Fixed.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1102
>> +    for (Length point : points) {
> 
> maybe auto& to avoid a copy.

Fixed.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1112
>> +        RefPtr<CSSPrimitiveValue> primitiveValue = repeatPoint.isPercentNotCalculated() ? cssValuePool().createValue(repeatPoint.percent(), CSSPrimitiveValue::CSS_PERCENTAGE) : zoomAdjustedPixelValue((int)valueForLength(repeatPoint, 0), style);
> 
> You are throwing away fractional pixels here with the (int) cast. This seems wrong.

Good catch -- fixed.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1127
>> +    for (SnapCoordinate coordinate : coordinates) {
> 
> const auto& ?

Fixed.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1133
>> +            currentCoordinate.get().append(zoomAdjustedPixelValue((int)valueForLength(point, 0), style));
> 
> Another int cast

Fixed.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1138
>> +            currentCoordinate.get().append(zoomAdjustedPixelValue((int)valueForLength(point, 0), style));
> 
> And another.

Fixed.

>> Source/WebCore/css/CSSParser.cpp:3060
>> +    case CSSPropertyWebkitScrollSnapPointsX: // elements | <length>+ | (<length>+)? repeat(<length>)
> 
> I don't think the comments help here.

Removed comment.

>> Source/WebCore/css/CSSParser.cpp:3069
>> +    case CSSPropertyWebkitScrollSnapCoordinate: // (<length>{2}+)?
> 
> Ditto.

Removed comment.

>> Source/WebCore/css/CSSParser.cpp:3183
>> +    while (CSSParserValue* val = m_valueList->current()) {
> 
> You're not handling "elements" in this function? If not you should add a comment to say it needs to be added.

Changed the function name to "parseScrollSnapPointsWithoutElements," since the "elements" CSS value is actually being handled in applyProperty, before this function is called, in the same way other value identifiers are handled (it just sets validPrimitive to true and breaks out of the switch case in parseValue)

>> Source/WebCore/css/CSSParser.cpp:3195
>> +                values->append(RefPtr<CSSValue>(CSSPrimitiveValue::create("repeat", CSSPrimitiveValue::CSS_STRING)).release());
> 
> Is the RefPtr<CSSValue>() required?

Oops, it's not -- good catch!

>> Source/WebCore/css/CSSParser.cpp:3218
>> +    if (m_valueList->size() == 2) {
> 
> You should reverse the check and early return.

Fixed.

>> Source/WebCore/css/CSSParser.cpp:3223
>> +            return false;
> 
> Better as if (!validUnit()) return false.

Fixed.

>> Source/WebCore/css/CSSParser.cpp:3226
>> +        if (validUnit(curParserVal, FPercent | FLength))
> 
> Same

Fixed.

>> Source/WebCore/css/CSSParser.cpp:3246
>> +        m_valueList->next();
> 
> I think it's a bad to call current() and next() without checking to see if you're at the end of the list.

Fixed. I now return false if m_valueList has no next() after parsing an odd value.

>> Source/WebCore/css/CSSParser.cpp:3250
>> +        if (validUnit(parsedValueX, FPercent | FLength))
> 
> if (!validUnit)

Fixed.

>> Source/WebCore/css/CSSPrimitiveValue.cpp:55
>> +#include "StyleScrollSnapPoints.h"
> 
> Why do you need this #include?

Good point -- forgot I don't actually need this anymore. Removed.

>> Source/WebCore/css/StyleResolver.cpp:2848
>> +            state.style()->setScrollSnapType(ScrollSnapType::None);
> 
> Isn't this what CSSPrimitiveValue::operator ScrollSnapType() is for?

Indeed -- fixed.

>> Source/WebCore/css/StyleResolver.cpp:2855
>> +            renderStyle->setScrollSnapUsesElementsX(true);
> 
> Shouldn't you check that the value represents "elements"?

Added check that ->getValueID() == CSSValueElements

>> Source/WebCore/css/StyleResolver.cpp:2864
>> +                if (primitiveItemValue->isString() && primitiveItemValue->getStringValue() == "repeat")
> 
> You don't need to compare strings here.  toCSSPrimitiveValue(value)->getValueID() == CSSValueRepeat

Unfortunately, repeat(<length>) isn't a CSS value :( It should actually be a unit, similar to a counter. Since it doesn't make a big difference in the behavior, I'm using a string token, but I'll fix this soon after I get some basic snapping behaviors into the system. For now, I'm leaving a FIXME indicating that repeat() needs to be added as a CSS unit type.

>> Source/WebCore/css/StyleResolver.cpp:2896
>> +            }
> 
> You should share this code between X and Y.

Fixed.

>> Source/WebCore/css/StyleResolver.cpp:2920
>> +        for (size_t idx = 0; idx < pointCount; idx++) {
> 
> idx -> i

Fixed.

>> Source/WebCore/rendering/style/RenderStyle.h:1096
>> +    bool scrollSnapUsesElementsY() const { return rareNonInheritedData->m_scrollSnapPoints->usesElementsY; }
> 
> I wonder if we should make a class which you can share between X and Y

Do you mean instead of having an X and Y version of each field in my StyleScrollSnapPoints object, just having two objects, each of which contains the X data xor the Y data? I was thinking about that, but I wasn't sure how to split scrollSnapCoordinates, since there's no X/Y-specific data for coordinates. Maybe have it in a separate field, or store the x-values of the coordinates in the X object and the y-values in the other? This would mean we'd need both objects to know exactly where the coordinates are.

>> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:58
>> +    }
> 
> Not sure I like the names having "point" since these don't use IntPoint/FloatPoint.

Indeed. "offset" is more fitting :)

>> LayoutTests/platform/mac/css3/scroll-snap/scroll-snap-property-computed-style-expected.txt:98
>> +PASS window.getComputedStyle(document.body).getPropertyValue('-webkit-scroll-snap-destination') is '10px 50px'
> 
> You should test fractional values here as well (10.5px, 12.234px)

Got it -- fractional tests to come soon.

[Wenson Hsieh]

Created attachment 235334 [details]
Patch

[Wenson Hsieh]

Created attachment 235336 [details]
Patch

[Wenson Hsieh]

Created attachment 235340 [details]
Patch

[Wenson Hsieh]

Created attachment 235341 [details]
Patch

[Wenson Hsieh]

Comment on attachment 235341 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=235341&action=review

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:6252
> +		F47A5E3E195B8C8A00483100 /* StyleScrollSnapPoints.h in Headers */ = {isa = PBXBuildFile; fileRef = F47A5E3B195B8C8A00483100 /* StyleScrollSnapPoints.h */; settings = {ATTRIBUTES = (Private, ); }; };

I tried making this Project, but (as with before) it broke when attempting to compile WebKitLegacy. Maybe there's something else I need to avoid making this Private?

[Simon Fraser (smfr)]

(In reply to comment #43)
> (From update of attachment 235341 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235341&action=review
> 
> > Source/WebCore/WebCore.xcodeproj/project.pbxproj:6252
> > +		F47A5E3E195B8C8A00483100 /* StyleScrollSnapPoints.h in Headers */ = {isa = PBXBuildFile; fileRef = F47A5E3B195B8C8A00483100 /* StyleScrollSnapPoints.h */; settings = {ATTRIBUTES = (Private, ); }; };
> 
> I tried making this Project, but (as with before) it broke when attempting to compile WebKitLegacy. Maybe there's something else I need to avoid making this Private?

It's because RenderStyle.h includes it, which I don't think you can easily avoid. It's OK to make it Private.

[Wenson Hsieh]

Created attachment 235582 [details]
Patch

[WebKit Commit Bot]

Attachment 235582 [details] did not pass style-queue:


ERROR: Source/WebCore/css/CSSPrimitiveValue.h:112:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Wenson Hsieh]

Comment on attachment 235582 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=235582&action=review

> Source/WebCore/css/CSSCalculationValue.cpp:143
> +    case CSSPrimitiveValue::CSS_REPEAT:

Hmm...this line is causing a style error: "enum members should use InterCaps with an initial capital letter". While that's accurate, the other CSSPrimitiveValue enums are also all caps and don't seem to trigger it. Perhaps this is a false positive?

[Simon Fraser (smfr)]

Comment on attachment 235582 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=235582&action=review

Let's do one final round.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1088
> +static PassRef<CSSValueList> getScrollSnapDestination(RenderStyle* style, Length x, Length y)

Drop the "get", so just scrollSnapDestination()

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1104
> +static PassRef<CSSValueList> getScrollSnapPoints(RenderStyle* style, const Vector<Length>& points, Length repeatPoint, bool hasRepeat)

scrollSnapPoints()

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1114
> +static PassRef<CSSValueList> getScrollSnapCoordinates(RenderStyle* style, const Vector<SnapCoordinate>& coordinates)

scrollSnapCoordinates()

> Source/WebCore/css/CSSParser.cpp:3186
> +bool CSSParser::parseScrollSnapPointsWithoutElements(CSSPropertyID propId, bool important)

This name is odd. Once you add elements will that be handled here or in another function?

> Source/WebCore/css/CSSParser.cpp:3205
> +            if (validUnit(parserVal, FLength | FPercent)) {
> +                values->append(cssValuePool().createValue(Repeat::create(createPrimitiveNumericValue(parserVal))));
> +                m_valueList->next();
> +                if (m_valueList->current())
> +                    return false;
> +                break;
> +            }

Does this need to do anything to handle calc()?

> Source/WebCore/css/CSSParser.cpp:3221
> +    RefPtr<CSSValueList> position = CSSValueList::createSpaceSeparated();
> +    RefPtr<CSSValue> cssValueX, cssValueY;

Declare on first use.

> Source/WebCore/css/CSSPrimitiveValue.h:112
> +        CSS_REPEAT = 34,

Please call this CSS_LENGTH_REPEAT

> Source/WebCore/css/CSSPrimitiveValue.h:192
> +    bool isRepeat() const { return m_primitiveUnitType == CSS_REPEAT; }

isLengthRepeat()

> Source/WebCore/css/CSSPrimitiveValue.h:318
> +    Repeat* getRepeatValue(ExceptionCode&) const;
> +    Repeat* getRepeatValue() const { return m_primitiveUnitType != CSS_REPEAT ? 0 : m_value.repeat; }

getLengthRepeat etc.

> Source/WebCore/css/Repeat.h:2
> +/*
> + * Copyright (C) 2014 Apple Inc. All rights reserved.

Please rename the file to LengthRepeat.h

> Source/WebCore/css/Repeat.h:18
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Library General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Library General Public License for more details.
> + *
> + * You should have received a copy of the GNU Library General Public License
> + * along with this library; see the file COPYING.LIB.  If not, write to
> + * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> + * Boston, MA 02110-1301, USA.
> + */

Wrong license. This should have the 2-clause Apple license (e.g. from ScrollingTreeIOS.h).

> Source/WebCore/css/Repeat.h:31
> +class Repeat : public RefCounted<Repeat> {

LengthRepeat

> Source/WebCore/css/Repeat.h:32
> +

No blank line here.

> Source/WebCore/css/Repeat.h:34
> +

No blank line here.

> Source/WebCore/css/Repeat.h:54
> +

Remove.

> Source/WebCore/css/Repeat.h:60
> +    RefPtr<CSSPrimitiveValue> m_interval;

Isn't this just a Length value?

> Source/WebCore/css/Repeat.h:61
> +

Remove

> Source/WebCore/css/StyleResolver.cpp:2913
> +        Vector<SnapCoordinate> coordinates;

You could reserveCapacity() on this vector since you know the final size.

> Source/WebCore/rendering/style/StyleScrollSnapPoints.cpp:18
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are met:
> + * 
> + * 1. Redistributions of source code must retain the above copyright notice, this
> + * list of conditions and the following disclaimer.
> + * 
> + * 2. Redistributions in binary form must reproduce the above copyright notice,
> + * this list of conditions and the following disclaimer in the documentation and/
> + * or other materials provided with the distribution.
> + * 
> + * 3. Neither the name of the copyright holder nor the names of its contributors
> + * may be used to endorse or promote products derived from this software without
> + * specific prior written permission.
> + * 

Wrong license. This should have the 2-clause Apple license (e.g. from ScrollingTreeIOS.h).

> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:18
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are met:
> + * 
> + * 1. Redistributions of source code must retain the above copyright notice, this
> + * list of conditions and the following disclaimer.
> + * 
> + * 2. Redistributions in binary form must reproduce the above copyright notice,
> + * this list of conditions and the following disclaimer in the documentation and/
> + * or other materials provided with the distribution.
> + * 
> + * 3. Neither the name of the copyright holder nor the names of its contributors
> + * may be used to endorse or promote products derived from this software without
> + * specific prior written permission.
> + * 

Ditto.

[Wenson Hsieh]

Comment on attachment 235582 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=235582&action=review

Thanks again for taking a look at this! I've addressed all the issues -- hopefully we can land the final version this Monday!

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1088
>> +static PassRef<CSSValueList> getScrollSnapDestination(RenderStyle* style, Length x, Length y)
> 
> Drop the "get", so just scrollSnapDestination()

Got it -- fixed.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1104
>> +static PassRef<CSSValueList> getScrollSnapPoints(RenderStyle* style, const Vector<Length>& points, Length repeatPoint, bool hasRepeat)
> 
> scrollSnapPoints()

Fixed.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1114
>> +static PassRef<CSSValueList> getScrollSnapCoordinates(RenderStyle* style, const Vector<SnapCoordinate>& coordinates)
> 
> scrollSnapCoordinates()

Fixed.

>> Source/WebCore/css/CSSParser.cpp:3186
>> +bool CSSParser::parseScrollSnapPointsWithoutElements(CSSPropertyID propId, bool important)
> 
> This name is odd. Once you add elements will that be handled here or in another function?

Renaming to parseNonElementSnapPoints.

>> Source/WebCore/css/CSSParser.cpp:3205
>> +            }
> 
> Does this need to do anything to handle calc()?

I haven't thought about that yet -- I'll add in calc( ) support after this patch as an issue.

>> Source/WebCore/css/CSSParser.cpp:3221
>> +    RefPtr<CSSValue> cssValueX, cssValueY;
> 
> Declare on first use.

Fixed.

>> Source/WebCore/css/CSSPrimitiveValue.h:112
>> +        CSS_REPEAT = 34,
> 
> Please call this CSS_LENGTH_REPEAT

Renamed. Thanks for the suggestion!

>> Source/WebCore/css/CSSPrimitiveValue.h:192
>> +    bool isRepeat() const { return m_primitiveUnitType == CSS_REPEAT; }
> 
> isLengthRepeat()

Renamed.

>> Source/WebCore/css/CSSPrimitiveValue.h:318
>> +    Repeat* getRepeatValue() const { return m_primitiveUnitType != CSS_REPEAT ? 0 : m_value.repeat; }
> 
> getLengthRepeat etc.

Renamed.

>> Source/WebCore/css/Repeat.h:2
>> + * Copyright (C) 2014 Apple Inc. All rights reserved.
> 
> Please rename the file to LengthRepeat.h

Renamed.

>> Source/WebCore/css/Repeat.h:31
>> +class Repeat : public RefCounted<Repeat> {
> 
> LengthRepeat

Renamed.

>> Source/WebCore/css/Repeat.h:32
>> +
> 
> No blank line here.

Fixed.

>> Source/WebCore/css/Repeat.h:34
>> +
> 
> No blank line here.

Removed line.

>> Source/WebCore/css/Repeat.h:54
>> +
> 
> Remove.

Fixed.

>> Source/WebCore/css/Repeat.h:60
>> +    RefPtr<CSSPrimitiveValue> m_interval;
> 
> Isn't this just a Length value?

I think for the purposes of generating css text, it should be a CSSPrimitiveValue (I took cues from Rect.h and RGBColor.h, which both specify their data via CSSPrimitiveValues).

>> Source/WebCore/css/Repeat.h:61
>> +
> 
> Remove

Fixed.

>> Source/WebCore/css/StyleResolver.cpp:2913
>> +        Vector<SnapCoordinate> coordinates;
> 
> You could reserveCapacity() on this vector since you know the final size.

Fixed.

>> Source/WebCore/rendering/style/StyleScrollSnapPoints.cpp:18
>> + * 
> 
> Wrong license. This should have the 2-clause Apple license (e.g. from ScrollingTreeIOS.h).

Fixed.

>> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:18
>> + * 
> 
> Ditto.

Fixed.

[Wenson Hsieh]

Created attachment 235953 [details]
Patch

[WebKit Commit Bot]

Attachment 235953 [details] did not pass style-queue:


ERROR: Source/WebCore/css/CSSPrimitiveValue.h:112:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Wenson Hsieh]

Created attachment 235954 [details]
Patch

[WebKit Commit Bot]

Attachment 235954 [details] did not pass style-queue:


ERROR: Source/WebCore/css/CSSPrimitiveValue.h:112:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 235954 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=235954&action=review

> Source/WebCore/css/StyleResolver.cpp:2869
> +        } else {

You could return before this line to make the logic easier to follow.

> Source/WebCore/css/StyleResolver.cpp:2877
> +            for (CSSValueListIterator i(value); i.hasMore(); i.advance()) {

i -> it

> Source/WebCore/rendering/style/RenderStyle.h:1627
> +    void setScrollSnapType(ScrollSnapType t) { SET_VAR(rareNonInheritedData, m_scrollSnapType, t); }
> +    void setScrollSnapOffsetsX(Vector<Length>& t) { SET_VAR(rareNonInheritedData.access()->m_scrollSnapPoints, offsetsX, t); }
> +    void setScrollSnapOffsetsY(Vector<Length>& t) { SET_VAR(rareNonInheritedData.access()->m_scrollSnapPoints, offsetsY, t); }
> +    void setScrollSnapRepeatOffsetX(Length t) { SET_VAR(rareNonInheritedData.access()->m_scrollSnapPoints, repeatOffsetX, t); }
> +    void setScrollSnapRepeatOffsetY(Length t) { SET_VAR(rareNonInheritedData.access()->m_scrollSnapPoints, repeatOffsetY, t); }
> +    void setScrollSnapHasRepeatX(bool t) { SET_VAR(rareNonInheritedData.access()->m_scrollSnapPoints, hasRepeatX, t); }
> +    void setScrollSnapHasRepeatY(bool t) { SET_VAR(rareNonInheritedData.access()->m_scrollSnapPoints, hasRepeatY, t); }
> +    void setScrollSnapDestinationX(Length t) { SET_VAR(rareNonInheritedData.access()->m_scrollSnapPoints, destinationX, t); }
> +    void setScrollSnapDestinationY(Length t) { SET_VAR(rareNonInheritedData.access()->m_scrollSnapPoints, destinationY, t); }
> +    void setScrollSnapCoordinates(Vector<SnapCoordinate>& t) { SET_VAR(rareNonInheritedData.access()->m_scrollSnapPoints, coordinates, t); }
> +    void setScrollSnapUsesElementsX(bool t) { SET_VAR(rareNonInheritedData.access()->m_scrollSnapPoints, usesElementsX, t); }
> +    void setScrollSnapUsesElementsY(bool t) { SET_VAR(rareNonInheritedData.access()->m_scrollSnapPoints, usesElementsY, t); }

Please use readable parameters names, rather than 't'.

[Dean Jackson]

Comment on attachment 235954 [details]
Patch

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.

[Wenson Hsieh]

Comment on attachment 235954 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=235954&action=review

Thank you for the reviews, Simon and Dean! I'll have a new version up very soon -- let's hope it's the final one :)

>> Source/WebCore/ChangeLog:18
>> +        * 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"

Got it, I'll keep that in mind.

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

Added it. Good catch.

>> Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters:9917
>> +    </ClInclude>
> 
> What about LengthRepeat.h?

Added LengthRepeat.h

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1100
>> +
> 
> 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.

Good point -- updated to remove get() call.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1108
>> +        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.

Fixed.

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

Fixed.

>>> Source/WebCore/css/StyleResolver.cpp:2869
>>> +        } else {
>> 
>> You could return before this line to make the logic easier to follow.
> 
> Not important, but maybe you could early return at the end of the if, and then avoid the else block.

Added early return.

>> Source/WebCore/css/StyleResolver.cpp:2874
>> +                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.

Removed. Good point.

>> Source/WebCore/css/StyleResolver.cpp:2877
>> +            for (CSSValueListIterator i(value); i.hasMore(); i.advance()) {
> 
> i -> it

Fixed.

>> Source/WebCore/css/StyleResolver.cpp:2898
>> +                renderStyle->setScrollSnapOffsetsY(points);
> 
> I think this should go up before the loop over CSSValueList.

Hmm...I think it needs to go after the loop, since the setter in RenderStyle copies the offsets by value.

>> Source/WebCore/rendering/style/RenderStyle.h:1627
>> +    void setScrollSnapUsesElementsY(bool t) { SET_VAR(rareNonInheritedData.access()->m_scrollSnapPoints, usesElementsY, t); }
> 
> Please use readable parameters names, rather than 't'.

Replaced 1-letter parameters with more descriptive names.

>> LayoutTests/platform/mac/css3/scroll-snap/scroll-snap-property-computed-style.html:4
>> +<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?

So far, the CSS_SCROLL_SNAP feature flag is only turned on for platform Mac (it will be on for iOS in the near future as well) so this test is specific to mac wk1 and wk2.

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

Good catch -- removed these variables.

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

Fixed.

[Wenson Hsieh]

Created attachment 236142 [details]
Patch

[Tim Horton]

(In reply to comment #56)
> (From update of attachment 235954 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235954&action=review
> 
> >> LayoutTests/platform/mac/css3/scroll-snap/scroll-snap-property-computed-style.html:4
> >> +<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?
> 
> So far, the CSS_SCROLL_SNAP feature flag is only turned on for platform Mac (it will be on for iOS in the near future as well) so this test is specific to mac wk1 and wk2.

But it's not specific to Mac, just only enabled there. You should put the tests in generic directories and skip them on the platforms that don't implement this feature yet.

[WebKit Commit Bot]

Attachment 236142 [details] did not pass style-queue:


ERROR: Source/WebCore/css/CSSPrimitiveValue.h:112:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Wenson Hsieh]

Created attachment 236146 [details]
Patch

[WebKit Commit Bot]

Attachment 236146 [details] did not pass style-queue:


ERROR: Source/WebCore/css/CSSPrimitiveValue.h:112:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Wenson Hsieh]

Created attachment 236147 [details]
Patch

[WebKit Commit Bot]

Attachment 236147 [details] did not pass style-queue:


ERROR: Source/WebCore/css/CSSPrimitiveValue.h:112:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 236147 [details]
Patch

Clearing flags on attachment: 236147

Committed r172192: <http://trac.webkit.org/changeset/172192>