Summary: | [BlackBerry] Removing String operator += uses in BlackBerry code | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sean Wang <xuewen.ok> | ||||||||||
Component: | WebKit BlackBerry | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | CLOSED INVALID | ||||||||||||
Severity: | Normal | CC: | andersca, charles.wei, mifenton, rwlbuis, staikos, tonikitoo, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Sean Wang
2012-09-06 22:02:01 PDT
Created attachment 162670 [details]
Patch
This patch is replace all WTF::String::operator +=() in all blackberry codes.
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 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 on attachment 162670 [details]
Patch
r-
Created attachment 162685 [details]
Patch
Rebase the last patch on the top of upstream.
Comment on attachment 162685 [details]
Patch
Why can't you use StringBuilder for these?
(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. 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?
(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 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. 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.
(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 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 on attachment 163081 [details]
patch
GEt this out of the review queue until the new patch arrives.
|