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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Jun 29 19:28:13 PDT 2014

Previous message (by thread): [Webkit-unassigned] [Bug 134301] Implement parsing for CSS scroll snap points
Next message (by thread): [Webkit-unassigned] [Bug 134301] Implement parsing for CSS scroll snap points
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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


Sam Weinig <sam at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #234048|review?                     |review-
               Flag|                            |




--- Comment #28 from Sam Weinig <sam at webkit.org>  2014-06-29 19:28:28 PST ---
(From update of attachment 234048)
View in context: https://bugs.webkit.org/attachment.cgi?id=234048&action=review

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1100
> +        snapPointsValue.get().append(cssValuePool().createValue(point.value, type));

You can get rid of the .get() if you change snapPointsValue from auto to Ref<CSSValueList>.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1105
> +        repeatStrBuilder.append("repeat(", 7);

This should use StringBuilder::appendLiteral(...)

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1106
> +        repeatStrBuilder.append(String::numberToStringFixedWidth((double) repeatPoint.value, 0));

If the cast to double is necessary, you should use a static_cast.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1108
> +            repeatStrBuilder.append("%%", 1);

This should use StringBuilder::appendLiteral(...)

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1110
> +            repeatStrBuilder.append("px", 2);

This should use StringBuilder::appendLiteral(...)

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1111
> +        repeatStrBuilder.append(")", 1);

This should use StringBuilder::appendLiteral(...)

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1118
> +static PassRef<CSSValueList> getScrollSnapCoordinates(const Vector<SnapPoint>& coords)

Can we pass this in / store these as a Vector<std::pair<SnapPoint, SnapPoint>>.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1126
> +        currentCoord.get().append(cssValuePool().createValue(point.value, type));

You can get rid of the .get() if you change currentCoord from auto to Ref<CSSValueList>.

> Source/WebCore/css/CSSParser.cpp:3115
> +        RefPtr<CSSValue> parsedValue;
> +        if (val->unit == CSSPrimitiveValue::CSS_PERCENTAGE 
> +            || val->unit == CSSPrimitiveValue::CSS_NUMBER
> +            || val->unit == CSSPrimitiveValue::CSS_PX) {
> +            parsedValue = CSSPrimitiveValue::create(val->fValue, static_cast<CSSPrimitiveValue::UnitTypes>(val->unit));
> +            values->append(parsedValue.release());

The spec says this should support <length>. What is the reason for supporting a reduced set.

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2320
> +    static void applyInheritValue(CSSPropertyID, StyleResolver* styleResolver)
> +    {
> +        UNUSED_PARAM(styleResolver);

There is no reason to use UNUSED_PARAM() here. Just don't declare the parameter name in the signature.

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2337
> +        else {
> +            renderStyle->setScrollSnapUsesElementsX(false);

You should assert that the CSSValue is a CSSValueList here.

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2339
> +            Vector<SnapPoint> points = Vector<SnapPoint>();

This can be written as Vector<SnapPoint> points;

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

This should probably use a CSSValueListIterator

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2353
> +                else if (primitiveValue->isString() && primitiveValue->getStringValue() == "repeat")

It seems quite odd that this is a string compare.  We should instead store the repeat-ness in a CSSValue keyword.

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2370
> +    static void applyInheritValue(CSSPropertyID, StyleResolver* styleResolver)
> +    {
> +        UNUSED_PARAM(styleResolver);

There is no reason to use UNUSED_PARAM() here. Just don't declare the parameter name in the signature.

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2420
> +    static void applyInheritValue(CSSPropertyID, StyleResolver* styleResolver)
> +    {
> +        UNUSED_PARAM(styleResolver);

There is no reason to use UNUSED_PARAM() here. Just don't declare the parameter name in the signature.

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2452
> +    static void applyInheritValue(CSSPropertyID, StyleResolver* styleResolver)
> +    {
> +        UNUSED_PARAM(styleResolver);

There is no reason to use UNUSED_PARAM() here. Just don't declare the parameter name in the signature.

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2457
> +    static void applyInitialValue(CSSPropertyID, StyleResolver* styleResolver)
> +    {
> +        UNUSED_PARAM(styleResolver);

There is no reason to use UNUSED_PARAM() here. Just don't declare the parameter name in the signature.

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2467
> +        // Every x must be followed by a y.
> +        if (pointCount & 0x1)
> +            return;

This seems like an overly complicated way to say % 2.

> Source/WebCore/rendering/style/RenderStyle.h:1080
> +    Vector<SnapPoint> scrollSnapPointsX() const { return rareNonInheritedData->m_scrollSnapPoints->pointsX; }
> +    Vector<SnapPoint> scrollSnapPointsY() const { return rareNonInheritedData->m_scrollSnapPoints->pointsY; }

Do we really want to copy the Vector here?  If not, this should return a const Vector<SnapPoint>&.

> Source/WebCore/rendering/style/RenderStyle.h:1087
> +    Vector<SnapPoint> scrollSnapCoordinates() const { return rareNonInheritedData->m_scrollSnapPoints->coords; }

Do we really want to copy the Vector here?  If not, this should return a const Vector<SnapPoint>&.

> Source/WebCore/rendering/style/RenderStyleConstants.h:557
> +enum EScrollSnapType { ScrollSnapTypeNone = 0, ScrollSnapTypeProximity = 1, ScrollSnapTypeMandatory = 2};

The E prefix is unnecessary and this should be an enum class.

> Source/WebCore/rendering/style/StyleScrollSnapPoints.cpp:19
> +/*
> + * Copyright (C) 2014 Apple Inc. All rights reserved.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Library General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Library General Public License for more details.
> + *
> + * You should have received a copy of the GNU Library General Public License
> + * along with this library; see the file COPYING.LIB.  If not, write to
> + * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> + * Boston, MA 02110-1301, USA.
> + *
> + */

Please use the BSD license.

> Source/WebCore/rendering/style/StyleScrollSnapPoints.cpp:26
> +#include <wtf/text/WTFString.h>

This is already included in the header.

> Source/WebCore/rendering/style/StyleScrollSnapPoints.cpp:41
> +    , pointsX(Vector<SnapPoint>())
> +    , pointsY(Vector<SnapPoint>())
> +    , coords(Vector<SnapPoint>())

These don't require explicit initialization.

> Source/WebCore/rendering/style/StyleScrollSnapPoints.cpp:46
> +    : RefCounted<StyleScrollSnapPoints>()

This shouldn't require explicit initialization.

> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:19
> +/*
> + * Copyright (C) 2014 Apple Inc. All rights reserved.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Library General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Library General Public License for more details.
> + *
> + * You should have received a copy of the GNU Library General Public License
> + * along with this library; see the file COPYING.LIB.  If not, write to
> + * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> + * Boston, MA 02110-1301, USA.
> + *
> + */

Please use the BSD license.

> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:36
> +enum SnapPointConstant {
> +    DefaultRepeat,
> +    DefaultDestination,
> +};

This should be an enum class.

> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:38
> +struct SnapPoint {

I'm not too familiar with the terminology, but we usually think of a point as something with at least an X and Y.  Is this something the spec names?  If not, perhaps we can rename this to something more suitable (SnapValue? SnapLength?).

> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:43
> +    SnapPoint(float val, bool isPct)
> +        : value(val)
> +        , isPercentage(isPct)
> +    {
> +    }

The spec indicates that a these values should of type <length>. Why are we limiting it?

> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:44
> +    SnapPoint(SnapPointConstant snapConstant)

This should be an explicit constructor.

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

Previous message (by thread): [Webkit-unassigned] [Bug 134301] Implement parsing for CSS scroll snap points
Next message (by thread): [Webkit-unassigned] [Bug 134301] Implement parsing for CSS scroll snap points
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

More information about the webkit-unassigned mailing list