[webkit-reviews] review denied: [Bug 134301] Implement parsing for CSS scroll snap points : [Attachment 234048] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Wenson Hsieh <wenson_hsieh at apple.com>'s
request for review:
Bug 134301: Implement parsing for CSS scroll snap points
https://bugs.webkit.org/show_bug.cgi?id=134301

Attachment 234048: Patch
https://bugs.webkit.org/attachment.cgi?id=234048&action=review

------- Additional Comments from Sam Weinig <sam at webkit.org>
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.