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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Jun 26 10:13:10 PDT 2014


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





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

Thanks for the review!

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3084
>> +            SnapPoint y = style->scrollSnapDestY();
> 
> no need for these local variables. function names clearly define what they are.

Fixed -- thanks!

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3097
>> +            bool hasRepeat = style->scrollSnapHasRepeatY();
> 
> same here.

Fixed.

>> Source/WebCore/css/CSSParser.cpp:3129
>> +        if (doAppend)
> 
> there's no way to figure out by looking at the parsedValue whether it has a value that needs to be appended? using doAppends is highly error-prone.

Good point -- fixed.

>> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2378
>> +        styleResolver->style()->setScrollSnapRepeatY(SnapPoint(100, true));
> 
> Can't we use some enums instead of these magic values? What's 100? px? %?

Fixed -- SnapPoint constructor will accept enums indicating default values.

>> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2434
>> +        styleResolver->style()->setScrollSnapDestY(SnapPoint(0, true));
> 
> same here.

Fixed -- thanks!

>> Source/WebCore/rendering/style/StyleScrollSnapPoints.cpp:22
>> +#include "StyleScrollSnapPoints.h"
> 
> missing #if ENABLE(CSS_SCROLL_SNAP)

Wrapped header in #if -- thanks.

>> Source/WebCore/rendering/style/StyleScrollSnapPoints.cpp:36
>> +    , destY(SnapPoint(0, false))
> 
> please use destination*

Renamed variables -- thanks

>> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:22
>> +#define StyleScrollSnapPoints_h
> 
> missing #if ENABLE(CSS_SCROLL_SNAP)

Fixed.

>> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:33
>> +    bool isRelative;
> 
> members are preferred to be at the bottom.

Moved members below methods

>> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:36
>> +        , isRelative(isRel)
> 
> isPercentage? ("relative" has multiple meanings when it is associated with coordinates. it might be better to use isPercentage unless you follow some css standard coding style here.)

Renamed to isPercentage

>> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:47
>> +};
> 
> keep in mind that this float value needs to be converted (and most likely pixel snapped) to LayoutUnit when you do the actual scroll pos snapping.

Thanks for the tip -- I think I'll keep it as a float for now, but I might change this when I implement the actual snapping behavior.

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