[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