Bug 97752 – Implement reviewer feedback that I missed on bug 95930.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Luke Macpherson]

Implement reviewer feedback that I missed on bug 95930.

[Luke Macpherson]

Created attachment 165918 [details]
Patch

[Darin Adler]

Comment on attachment 165918 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165918&action=review

> Source/WebCore/css/CSSBasicShapes.cpp:153
> +    size_t length = 19 + (3 * (points.size() / 2));

This mystery equation ought to have a comment and maybe even some named constants. It seems it would be easy for this to get out of sync with the code if we changed the code below.

> Source/WebCore/css/Rect.h:128
> +        result.reserveCapacity(top.length() + right.length() + bottom.length() + left.length() + 3);

The “3” here needs a brief comment.

[Tony Chang]

Comment on attachment 165918 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165918&action=review

> Source/WebCore/css/CSSBasicShapes.cpp:43
> +    result.reserveCapacity(22 + x.length() + y.length() + width.length() + height.length() + radiusX.length() + radiusY.length());

I would move the string literals into const char foo[] variables and use sizeof instead of hard coding 22. You could use a similar trick for buildPolygonString's 19.

[Tony Chang]

Comment on attachment 165918 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165918&action=review

>> Source/WebCore/css/CSSBasicShapes.cpp:43
>> +    result.reserveCapacity(22 + x.length() + y.length() + width.length() + height.length() + radiusX.length() + radiusY.length());
> 
> I would move the string literals into const char foo[] variables and use sizeof instead of hard coding 22. You could use a similar trick for buildPolygonString's 19.

Err, sizeof - 1.

[Luke Macpherson]

Created attachment 166111 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 166111 [details]
Patch for landing

Clearing flags on attachment: 166111

Committed r129831: <http://trac.webkit.org/changeset/129831>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tony Chang]

Comment on attachment 166111 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=166111&action=review

> Source/WebCore/css/CSSBasicShapes.cpp:168
> +        // add length of two strings, plus one for the space separator.

Nit: Capitalize the first letter of sentences.

> Source/WebCore/css/Rect.h:128
> +        // reserve space for the four strings, plus three space separator characters.

Nit: Capitalize the first letter of sentences.