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

[bugzilla-daemon at webkit.org]

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





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

Thanks for reviewing my patch! I'll put up a new patch up soon.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1121
>> +    for (int i = 0; i < (int)coords.size() / 2; i++) {
> 
> I think this should be size_t, no? (and in other places too)

Fixed -- good catch!

>> Source/WebCore/css/CSSParser.cpp:3180
>> +        CSSParserValue* xparsed = m_valueList->current();
> 
> These names aren't camel case (and I'm not sure they're the best names for the job).

Perhaps parsedValueX or parsedXValue work better?

>> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2331
>> +        styleResolver->style()->setScrollSnapHasRepeatX(false);
> 
> styleResolver->style() over and over; keep it in a local?

Good point -- fixed.

>> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2387
>> +            for (int i = 0; i < (int)valueList.length(); i++) {
> 
> size_t again.

Fixed.

>> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:85
>> +    bool hasRepeatX;
> 
> I'm surprised smfr didn't complain about member packing here :D In any case, I think this could be packed better (though I don't think there will be many of these in the common case?).

Right -- the snap scroll style objects should be relatively rare. The 4 bool flags could probably be condensed into single bits right off the bat.

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