[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