[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 #11 from Zalan Bujtas <zalan at apple.com>  2014-06-26 08:37:08 PST ---
(From update of attachment 233902)
View in context: https://bugs.webkit.org/attachment.cgi?id=233902&action=review

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3084
> +            SnapPoint x = style->scrollSnapDestX();
> +            SnapPoint y = style->scrollSnapDestY();

no need for these local variables. function names clearly define what they are.

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

same here.

> Source/WebCore/css/CSSParser.cpp:3129
> +        bool doAppend = false;
> +        RefPtr<CSSValue> parsedValue;
> +        if (val->unit == CSSPrimitiveValue::CSS_PERCENTAGE) {
> +            parsedValue = CSSPrimitiveValue::create(val->fValue, CSSPrimitiveValue::CSS_PERCENTAGE);
> +            doAppend = true;
> +        } else if (val->unit == CSSPrimitiveValue::CSS_NUMBER) {
> +            parsedValue = CSSPrimitiveValue::create(val->fValue, CSSPrimitiveValue::CSS_NUMBER);
> +            doAppend = true;
> +        } else if (val->unit == CSSPrimitiveValue::CSS_PX) {
> +            parsedValue = CSSPrimitiveValue::create(val->fValue, CSSPrimitiveValue::CSS_PX);
> +            doAppend = true;
> +        }
> +        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.

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

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

same here.

> Source/WebCore/rendering/style/StyleScrollSnapPoints.cpp:22
> +#include "StyleScrollSnapPoints.h"

missing #if ENABLE(CSS_SCROLL_SNAP)

> Source/WebCore/rendering/style/StyleScrollSnapPoints.cpp:36
> +    , destX(SnapPoint(0, false))
> +    , destY(SnapPoint(0, false))

please use destination*

> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:22
> +#define StyleScrollSnapPoints_h

missing #if ENABLE(CSS_SCROLL_SNAP)

> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:33
> +    bool isRelative;

members are preferred to be at the bottom.

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

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

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