https://bugs.webkit.org/show_bug.cgi?id=134301
Simon Fraser (smfr) <simon.fraser at apple.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #233850|review? |review-
Flag| |
--- Comment #7 from Simon Fraser (smfr) <simon.fraser at apple.com> 2014-06-25 17:06:46 PST ---
(From update of attachment 233850)
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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.