[Webkit-unassigned] [Bug 134301] Implement parsing for CSS scroll snap points
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Jun 26 16:38:00 PDT 2014
https://bugs.webkit.org/show_bug.cgi?id=134301
Dean Jackson <dino at apple.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #233930|review? |review-
Flag| |
--- Comment #17 from Dean Jackson <dino at apple.com> 2014-06-26 16:38:16 PST ---
(From update of attachment 233930)
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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list