Bug 96066 – [BlackBerry] Removing String operator += uses in BlackBerry code

• Status: CLOSED
• Resolution: INVALID
• Component: WebKit BlackBerry

[Sean Wang]

Removing String operator += uses in BlackBerry code since it is being deprecated.

[Sean Wang]

Created attachment 162670 [details]
Patch

This patch is replace all WTF::String::operator +=() in all blackberry codes.

[Charles Wei]

Comment on attachment 162670 [details]
Patch

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

All others look good.

> Source/WebCore/ChangeLog:4
> +        Replacing all usages of String operator += because itis being deprecated. 

It is deprecated.

> Source/WebCore/ChangeLog:7
> +        PR 203054

remove this.

[Charles Wei]

Comment on attachment 162670 [details]
Patch

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

This is a patch generated in local repo and doesn't apply to upstream.

>> Source/WebCore/ChangeLog:4
>> +        Replacing all usages of String operator += because itis being deprecated. 
> 
> It is deprecated.

It is deprecated.

>> Source/WebCore/ChangeLog:7
>> +        PR 203054
> 
> remove this.

remove this.

[Charles Wei]

Comment on attachment 162670 [details]
Patch

r-

[Sean Wang]

Created attachment 162685 [details]
Patch

Rebase the last patch on the top of upstream.

[George Staikos]

Comment on attachment 162685 [details]
Patch

Why can't you use StringBuilder for these?

[Sean Wang]

(In reply to comment #6)
> (From update of attachment 162685 [details])
> Why can't you use StringBuilder for these?

OK, I will change to use StringBuilder.

[Sean Wang]

Created attachment 162728 [details]
patch

I changed to use StringBuilder instead of operator +() to replace the operator +=().
I do it on upstream repo directly. Since most of these file are not synchronous with blackberry trunk, I have not gotten a change to build and verify them.
Should I sync these file to blackberry trunk and have test before push it?

[George Staikos]

(In reply to comment #8)
> Created an attachment (id=162728) [details]
> patch
> 
> I changed to use StringBuilder instead of operator +() to replace the operator +=().
> I do it on upstream repo directly. Since most of these file are not synchronous with blackberry trunk, I have not gotten a change to build and verify them.
> Should I sync these file to blackberry trunk and have test before push it?

Yes.

[Joe Mason]

Comment on attachment 162728 [details]
patch

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.

[Sean Wang]

Created attachment 163081 [details]
patch

This patch processes the following files:
WebCore/platform/network/blackberry/rss/RSSAtomParser.cpp
WebCore/platform/network/blackberry/rss/RSSGenerator.cpp
WebCore/platform/network/blackberry/rss/RSSParserBase.cpp
WebKit/blackberry/WebCoreSupport/JavaScriptDebuggerBlackBerry.cpp
WebKit/blackberry/WebKitSupport/AboutData.cpp

These files have little differences with blackberry development trunk. So I fix these files first. Other files will be fixed in other patches.

It will be convenient to use if we could add an operator += for StringBuilder.

[Sean Wang]

(In reply to comment #10)
> > 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.

I add a blank line after each <tr>...</tr>, since place multiple sentences in one line is not following webkit styles.

[Rob Buis]

Comment on attachment 163081 [details]
patch

Unfortunately it seems we did double work :( Probably my mistake as you were earlier. Can you try to make a diff against ToT? I think the rss files are the only ones I did so far.

[Rob Buis]

Comment on attachment 163081 [details]
patch

GEt this out of the review queue until the new patch arrives.