Bug 97752 - Implement reviewer feedback that I missed on bug 95930.
Summary: Implement reviewer feedback that I missed on bug 95930.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Luke Macpherson
URL:
Keywords:
Depends on: 95930
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-26 19:27 PDT by Luke Macpherson
Modified: 2012-09-28 10:39 PDT (History)
5 users (show)

See Also:


Attachments
Patch (7.60 KB, patch)
2012-09-26 19:29 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch for landing (9.49 KB, patch)
2012-09-27 18:49 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Luke Macpherson 2012-09-26 19:27:57 PDT
Implement reviewer feedback that I missed on bug 95930.
Comment 1 Luke Macpherson 2012-09-26 19:29:48 PDT
Created attachment 165918 [details]
Patch
Comment 2 Darin Adler 2012-09-27 10:47:39 PDT
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.
Comment 3 Tony Chang 2012-09-27 13:50:03 PDT
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.
Comment 4 Tony Chang 2012-09-27 13:50:30 PDT
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.
Comment 5 Luke Macpherson 2012-09-27 18:49:05 PDT
Created attachment 166111 [details]
Patch for landing
Comment 6 WebKit Review Bot 2012-09-27 19:16:22 PDT
Comment on attachment 166111 [details]
Patch for landing

Clearing flags on attachment: 166111

Committed r129831: <http://trac.webkit.org/changeset/129831>
Comment 7 WebKit Review Bot 2012-09-27 19:16:25 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Tony Chang 2012-09-28 10:39:37 PDT
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.