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

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> 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 233850: Patch
https://bugs.webkit.org/attachment.cgi?id=233850&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=233850&action=review


> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1086
> +

No blank line at the start of the function.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1087
> +    PassRef<CSSValueList> dst = CSSValueList::createSpaceSeparated();

This should be RefPtr<>, but you can also use "auto". Please don't use
abbreviated variable names like "dst". Maybe snapDestination or
snapDestinationValue.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1098
> +
> +    PassRef<CSSValueList> pointList = CSSValueList::createSpaceSeparated();

Ditto.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1108
> +	   snprintf(rs, 7, "%f", repeatPoint.value);

No snprintf! Use String/StringBuilder functions.

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

const Vector<>&

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1124
> +    PassRef<CSSValueList> allCoordinates =
CSSValueList::createCommaSeparated();

RefPtr (or auto).

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

No need for the local variable.

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2475
> +	   // every x must be followed by a y

Comments should have sentence case and a period at the end.

> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2479
> +	   if (!(pointCount & 0x1))
> +	       pointCount /= 2;
> +	   else
> +	       return;

If (pointCount & 0x1)
  return;

> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:71
> +    bool elementsX;
> +    bool elementsY;

The name is confusing; "elementsX" doesn't say why this is boolean.

> LayoutTests/platform/mac/css3/scroll-snap/parse-scroll-snap.html:33
> +	   <script type="text/javascript">
> +	       function testComputedStyleProperty(elemName, property, expected)
{

This should be a "dumpAsText()" test. I think you should use js-pre/js-post
like other similar tests.