[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





--- Comment #18 from Wenson Hsieh <wenson_hsieh at apple.com>  2014-06-26 18:38:00 PST ---
(From update of attachment 233930)
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.

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