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

Tue Jul 22 15:13:29 PDT 2014

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

--- Comment #38 from Wenson Hsieh <wenson_hsieh at apple.com>  2014-07-22 15:13:37 PST ---
(From update of attachment 234998)
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.

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