Bug 97752

Summary: Implement reviewer feedback that I missed on bug 95930.
Product: WebKit Reporter: Luke Macpherson <macpherson>
Component: New BugsAssignee: Luke Macpherson <macpherson>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, macpherson, menard, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 95930    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch for landing none

Luke Macpherson
Reported 2012-09-26 19:27:57 PDT
Implement reviewer feedback that I missed on bug 95930.
Attachments
Patch (7.60 KB, patch)
2012-09-26 19:29 PDT, Luke Macpherson 		no flags
DetailsFormatted DiffDiff
Patch for landing (9.49 KB, patch)
2012-09-27 18:49 PDT, Luke Macpherson 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Luke Macpherson
Comment 1 2012-09-26 19:29:48 PDT
Created attachment 165918 [details] Patch
Darin Adler
Comment 2 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.
Tony Chang
Comment 3 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.
Tony Chang
Comment 4 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.
Luke Macpherson
Comment 5 2012-09-27 18:49:05 PDT
Created attachment 166111 [details] Patch for landing
WebKit Review Bot
Comment 6 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>
WebKit Review Bot
Comment 7 2012-09-27 19:16:25 PDT
All reviewed patches have been landed. Closing bug.
Tony Chang
Comment 8 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.
Note You need to log in before you can comment on or make changes to this bug.