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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Jul 17 10:59:55 PDT 2014


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





--- Comment #35 from Simon Fraser (smfr) <simon.fraser at apple.com>  2014-07-17 11:00:06 PST ---
(From update of attachment 234998)
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

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