[webkit-reviews] review denied: [Bug 96066] [BlackBerry] Removing String operator += uses in BlackBerry code : [Attachment 162728] patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Sep 7 08:15:55 PDT 2012
Joe Mason <jmason at rim.com> has denied review:
Bug 96066: [BlackBerry] Removing String operator += uses in BlackBerry code
https://bugs.webkit.org/show_bug.cgi?id=96066
Attachment 162728: patch
https://bugs.webkit.org/attachment.cgi?id=162728&action=review
------- Additional Comments from Joe Mason <jmason at rim.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=162728&action=review
> Source/WebCore/platform/graphics/blackberry/ImageBlackBerry.cpp:60
> + WTF::StringBuilder fullPath;
Minor, but you could just use + for this ('String fullPath = RESOURCE_PATH +
name + ".png"').
StringBuilder's only really needed when there are substrings that may or may
not be included in the full string.
I'm not sure if operator+ or StringBuilder is faster, but webkit-dev, they said
they'd spent a lot of work optimizing operator+. It's just slightly easier to
read than StringBuilder, IMHO. But not enough to insist on.
> Source/WebCore/platform/network/blackberry/NetworkJob.cpp:132
> + m_contentDisposition = m_contentDisposition +
request.getSuggestedSaveName().c_str();
This isn't any improvement over +=. What you want is just
'm_contentDisposition = "filename=" + request.getSuggestedSaveName().c_str();'.
Or StringBuilder.
> Source/WebKit/blackberry/WebCoreSupport/JavaScriptDebuggerBlackBerry.cpp:92
> + sourceString = sourceString + (":" + lineNumber);
This should be "String sourceString = String(url, urlLength) + ":" +
lineNumber;" Or use StringBuilder.
(Actually, I think my version isn't any faster than yours, because of the
explicit String(url, urlLength), but it's no slower, and better to keep with
one style: "Don't use foo += bar or foo = foo + bar, but foo = bar + baz is ok"
is an easy rule to remember.)
> Source/WebKit/blackberry/WebKitSupport/AboutData.cpp:86
> + page.append("</td></tr>");
Can you put a blank line after each <tr>...</tr> block, or use
"page.append("<tr>..."); page.append(...); page.append("</tr>");" to put them
all on the same line, or something? (I like putting them all on the same line,
but it may be against the coding style. I'm not sure.) It's hard to read where
one cell stops and the next starts now.
> Source/WebKit/blackberry/WebKitSupport/AboutData.cpp:245
> + tableHTML + tableHTML + numberToHTMLTr(iter->first, iter->second);
BUG!
Everything else I pointed out is just a style complaint, but this is actually
wrong. You have "+" instead of "=". This use should definitely use
StringBuilder, and then assign "tableHTML = tableBuilder.toString()" when done.
> Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:359
> + return "";
I think s.toString() might be more clear here - I assume with an empty
StringBuilder, that creates an empty string. (I'm not clear why that used
s.utf8().data() on an empty string, instead of just "return s"...)
> Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:499
> + data = data + dumpBackForwardListForWebView();
Should use StringBuilder.
More information about the webkit-reviews
mailing list