[webkit-reviews] review denied: [Bug 134301] Implement parsing for CSS scroll snap points : [Attachment 233930] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has denied Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 134301: Implement parsing for CSS scroll snap points
https://bugs.webkit.org/show_bug.cgi?id=134301

Attachment 233930: Patch
https://bugs.webkit.org/attachment.cgi?id=233930&action=review

------- Additional Comments from Dean Jackson <dino at apple.com>
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.