[Webkit-unassigned] [Bug 96066] [BlackBerry] Removing String operator += uses in BlackBerry code

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=96066


Joe Mason <jmason at rim.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #162728|review+                     |review-
               Flag|                            |




--- Comment #10 from Joe Mason <jmason at rim.com>  2012-09-07 08:16:11 PST ---
(From update of attachment 162728)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.