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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Aug 6 15:18:22 PDT 2014


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





--- Comment #56 from Wenson Hsieh <wenson_hsieh at apple.com>  2014-08-06 15:18:27 PST ---
(From update of attachment 235954)
View in context: https://bugs.webkit.org/attachment.cgi?id=235954&action=review

Thank you for the reviews, Simon and Dean! I'll have a new version up very soon -- let's hope it's the final one :)

>> Source/WebCore/ChangeLog:18
>> +        * WebCore.xcodeproj/project.pbxproj: Added StyleScrollSnapPoints.h, StyleScrollSnapPoints.cpp, LengthRepeat.h
> 
> No need to change this now, but in the future I think it is ok to just say "Added new files"

Got it, I'll keep that in mind.

>> Source/WebCore/WebCore.vcxproj/WebCore.vcxproj:20386
>> +    <ClInclude Include="..\rendering\style\StyleScrollSnapPoints.h" />
> 
> What about LengthRepeat.h?

Added it. Good catch.

>> Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters:9917
>> +    </ClInclude>
> 
> What about LengthRepeat.h?

Added LengthRepeat.h

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1100
>> +
> 
> I could be the odd one out here, but you can replace all the snapDestinationValue.get().append calls with just snapDestinationValue->append, which looks cleaner.... but maybe only if you use RefPtr<CSSValueList> rather than "auto".
> 
> The same goes for snapPointsValue and snapCoordinatesValue in the other new functions.

Good point -- updated to remove get() call.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1108
>> +        snapPointsValue.get().append(point.isPercentNotCalculated() ? cssValuePool().createValue(point.percent(), CSSPrimitiveValue::CSS_PERCENTAGE) : zoomAdjustedPixelValue(valueForLength(point, 0), style));
> 
> In scrollSnapDestination/scrollSnapCoordinates you do this with an if/else statement rather than a ternary operator, which I think is easier to read.

Fixed.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1110
>> +        snapPointsValue.get().append(cssValuePool().createValue(LengthRepeat::create(repeatPoint.isPercentNotCalculated() ? cssValuePool().createValue(repeatPoint.percent(), CSSPrimitiveValue::CSS_PERCENTAGE) : zoomAdjustedPixelValue(valueForLength(repeatPoint, 0), style))));
> 
> Ditto.

Fixed.

>>> Source/WebCore/css/StyleResolver.cpp:2869
>>> +        } else {
>> 
>> You could return before this line to make the logic easier to follow.
> 
> Not important, but maybe you could early return at the end of the if, and then avoid the else block.

Added early return.

>> Source/WebCore/css/StyleResolver.cpp:2874
>> +                renderStyle->setScrollSnapUsesElementsY(false);
> 
> Should this test be here? I think you've added it accidentally. This code is only called if we're not CSSValueElements, and down below you're testing for scroll snap points.

Removed. Good point.

>> Source/WebCore/css/StyleResolver.cpp:2877
>> +            for (CSSValueListIterator i(value); i.hasMore(); i.advance()) {
> 
> i -> it

Fixed.

>> Source/WebCore/css/StyleResolver.cpp:2898
>> +                renderStyle->setScrollSnapOffsetsY(points);
> 
> I think this should go up before the loop over CSSValueList.

Hmm...I think it needs to go after the loop, since the setter in RenderStyle copies the offsets by value.

>> Source/WebCore/rendering/style/RenderStyle.h:1627
>> +    void setScrollSnapUsesElementsY(bool t) { SET_VAR(rareNonInheritedData.access()->m_scrollSnapPoints, usesElementsY, t); }
> 
> Please use readable parameters names, rather than 't'.

Replaced 1-letter parameters with more descriptive names.

>> LayoutTests/platform/mac/css3/scroll-snap/scroll-snap-property-computed-style.html:4
>> +<script src="../../../../resources/js-test-pre.js"></script>
> 
> Is there a reason these tests are in platform/mac?
> 
> Can't we make the test content platform neutral?

So far, the CSS_SCROLL_SNAP feature flag is only turned on for platform Mac (it will be on for iOS in the near future as well) so this test is specific to mac wk1 and wk2.

>> LayoutTests/platform/mac/css3/scroll-snap/scroll-snap-property-computed-style.js:4
>> +var stylesheet, filterStyle, subRule;
> 
> I don't think you use filterStyle or subRule.

Good catch -- removed these variables.

>> LayoutTests/platform/mac/css3/scroll-snap/scroll-snap-property-parsing.js:4
>> +var stylesheet, cssRule, declaration, filterRule, subRule;
> 
> Same here. filterRule and subRule are not used.

Fixed.

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