Bug 96066 - [BlackBerry] Removing String operator += uses in BlackBerry code
Summary: [BlackBerry] Removing String operator += uses in BlackBerry code
Status: CLOSED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit BlackBerry (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-06 22:02 PDT by Sean Wang
Modified: 2015-11-04 18:17 PST (History)
7 users (show)

See Also:


Attachments
Patch (45.33 KB, patch)
2012-09-06 22:20 PDT, Sean Wang
charles.wei: review-
charles.wei: commit-queue-
Details | Formatted Diff | Diff
Patch (45.47 KB, patch)
2012-09-06 23:47 PDT, Sean Wang
xuewen.ok: review-
xuewen.ok: commit-queue-
Details | Formatted Diff | Diff
patch (59.63 KB, patch)
2012-09-07 03:35 PDT, Sean Wang
joenotcharles: review-
staikos: commit-queue-
Details | Formatted Diff | Diff
patch (61.39 KB, patch)
2012-09-10 03:50 PDT, Sean Wang
rwlbuis: review-
xuewen.ok: commit-queue?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sean Wang 2012-09-06 22:02:01 PDT
Removing String operator += uses in BlackBerry code since it is being deprecated.
Comment 1 Sean Wang 2012-09-06 22:20:59 PDT
Created attachment 162670 [details]
Patch

This patch is replace all WTF::String::operator +=() in all blackberry codes.
Comment 2 Charles Wei 2012-09-06 23:01:25 PDT
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.
Comment 3 Charles Wei 2012-09-06 23:29:43 PDT
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.
Comment 4 Charles Wei 2012-09-06 23:30:17 PDT
Comment on attachment 162670 [details]
Patch

r-
Comment 5 Sean Wang 2012-09-06 23:47:40 PDT
Created attachment 162685 [details]
Patch

Rebase the last patch on the top of upstream.
Comment 6 George Staikos 2012-09-07 00:44:41 PDT
Comment on attachment 162685 [details]
Patch

Why can't you use StringBuilder for these?
Comment 7 Sean Wang 2012-09-07 01:23:00 PDT
(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.
Comment 8 Sean Wang 2012-09-07 03:35:55 PDT
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?
Comment 9 George Staikos 2012-09-07 04:13:27 PDT
(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.
Comment 10 Joe Mason 2012-09-07 08:16:11 PDT
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.
Comment 11 Sean Wang 2012-09-10 03:50:01 PDT
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.
Comment 12 Sean Wang 2012-09-10 03:53:33 PDT
(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.
Comment 13 Rob Buis 2012-09-17 07:10:27 PDT
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.
Comment 14 Rob Buis 2012-09-17 07:12:12 PDT
Comment on attachment 163081 [details]
patch

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