Summary: | Deploy StringBuilder in more places in WebKit2 | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jinwoo Song <jinwoo7.song> | ||||||||||||||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | benjamin, gtk-ews, gustavo, philn, ryuan.choi, webkit.review.bot, xan.lopez | ||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
Attachments: |
|
Description
Jinwoo Song
2012-09-05 19:00:11 PDT
Created attachment 162394 [details]
patch
Comment on attachment 162394 [details] patch Attachment 162394 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13772121 Comment on attachment 162394 [details] patch Attachment 162394 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13772125 Comment on attachment 162394 [details] patch Attachment 162394 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13774119 Created attachment 162398 [details]
patch
Comment on attachment 162398 [details] patch Attachment 162398 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13772137 Created attachment 162403 [details]
patch. build error fix.
Comment on attachment 162403 [details] patch. build error fix. Attachment 162403 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13768206 Comment on attachment 162403 [details] patch. build error fix. Attachment 162403 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13765458 Comment on attachment 162403 [details] patch. build error fix. View in context: https://bugs.webkit.org/attachment.cgi?id=162403&action=review > Source/WebKit2/Shared/WebMemorySampler.cpp:141 > + processDetails.appendLiteral("\n"); append('\n') looks better. > Source/WebKit2/Shared/WebMemorySampler.cpp:142 > + writeToFile(m_sampleLogFile, processDetails.toString().utf8().data(), processDetails.toString().utf8().length()); I am not sure, but isn't it better to keep String reference instead of using toString twice ? > Source/WebKit2/Shared/WebMemorySampler.cpp:153 > + header.appendLiteral("\n"); > + writeToFile(m_sampleLogFile, header.toString().utf8().data(), header.toString().utf8().length()); Ditto. > Source/WebKit2/Shared/WebMemorySampler.cpp:184 > + statString.appendLiteral("\n"); > + writeToFile(m_sampleLogFile, statString.toString().utf8().data(), statString.toString().utf8().length()); Ditto. > Source/WebKit2/WebProcess/Plugins/Netscape/NetscapeBrowserFuncs.cpp:127 > + etringBuilder result; Typo Comment on attachment 162403 [details] patch. build error fix. Attachment 162403 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13765470 Created attachment 162414 [details]
fixed typo. :(
Created attachment 162419 [details]
new patch. Applied ryuan's comments.
Created attachment 162421 [details]
patch.
(In reply to comment #10) > (From update of attachment 162403 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=162403&action=review > > > Source/WebKit2/Shared/WebMemorySampler.cpp:141 > > + processDetails.appendLiteral("\n"); > > append('\n') looks better. done. > > > Source/WebKit2/Shared/WebMemorySampler.cpp:142 > > + writeToFile(m_sampleLogFile, processDetails.toString().utf8().data(), processDetails.toString().utf8().length()); > > I am not sure, but isn't it better to keep String reference instead of using toString twice ? done. > > > Source/WebKit2/Shared/WebMemorySampler.cpp:153 > > + header.appendLiteral("\n"); > > + writeToFile(m_sampleLogFile, header.toString().utf8().data(), header.toString().utf8().length()); > > Ditto. done. > > > Source/WebKit2/Shared/WebMemorySampler.cpp:184 > > + statString.appendLiteral("\n"); > > + writeToFile(m_sampleLogFile, statString.toString().utf8().data(), statString.toString().utf8().length()); > > Ditto. done. > > > Source/WebKit2/WebProcess/Plugins/Netscape/NetscapeBrowserFuncs.cpp:127 > > + etringBuilder result; > > Typo done. Comment on attachment 162421 [details] patch. View in context: https://bugs.webkit.org/attachment.cgi?id=162421&action=review It looks almost fine to me. Below are some nit. > Source/WebKit2/ChangeLog:9 > + one space. > Source/WebKit2/Shared/WebMemorySampler.cpp:49 > + : m_separator(ASCIILiteral("\t")) remove spaces at end of lien. > Source/WebKit2/Shared/WebMemorySampler.cpp:151 > header.append(stats.keys[i].utf8().data()); stats.keys[i] looks enough. > Source/WebKit2/Shared/WebMemorySampler.cpp:176 > + StringBuilder statString; space. > Source/WebKit2/Shared/WebMemorySampler.cpp:187 > + statString.append('\n'); > + ditto. Comment on attachment 162421 [details] patch. View in context: https://bugs.webkit.org/attachment.cgi?id=162421&action=review All the changes are good ideas, but we can do even better. Some comments: >> Source/WebKit2/Shared/WebMemorySampler.cpp:49 >> - : m_separator(String("\t")) >> + : m_separator(ASCIILiteral("\t")) > > remove spaces at end of lien. Reading WebMemorySampler, m_separator is a bad idea, you should get rid of it. You can just have const char separator = '\t'; in the cpp file. > Source/WebKit2/Shared/WebMemorySampler.cpp:141 > - String processDetails = String("Process: "); > + StringBuilder processDetails; > + processDetails.appendLiteral("Process: "); > processDetails.append(processName()); > - processDetails.append(String("\n")); > - writeToFile(m_sampleLogFile, processDetails.utf8().data(), processDetails.utf8().length()); > + processDetails.append('\n'); Since there is no branch, it is more interesting to use the String Operators: String processDetails = "Process: " + processName() + '\n'; > Source/WebKit2/Shared/WebMemorySampler.cpp:186 > statString.append(String::number(memoryStats.values[i])); > } > } > - statString.append(String("\n")); > - writeToFile(m_sampleLogFile, statString.utf8().data(), statString.utf8().length()); > + statString.append('\n'); This one is a big win with StringBuilder. > Source/WebKit2/WebProcess/WebCoreSupport/WebContextMenuClient.cpp:87 > - String url("http://www.google.com/search?q="); > + StringBuilder url; > + url.appendLiteral("http://www.google.com/search?q="); > url.append(encoded); > - url.append("&ie=UTF-8&oe=UTF-8"); > + url.appendLiteral("&ie=UTF-8&oe=UTF-8"); This should use String Operators: url = "http://www.google.com/search?q=" + encoded + "&ie=UTF-8&oe=UTF-8"; Created attachment 162637 [details]
patch applied comments from Ryuan and Benjamin
Created attachment 162640 [details]
add ChangeLog
Applied comments by Ryuan and Benjamin. Thanks for scrupulous review!
Comment on attachment 162637 [details] patch applied comments from Ryuan and Benjamin View in context: https://bugs.webkit.org/attachment.cgi?id=162637&action=review I think that this patch can be landed? Why don't you add changelog and r? > Source/WebKit2/Shared/WebMemorySampler.cpp:39 > +const char separator = '\t'; nit. static const char? Created attachment 162649 [details]
patch.
> > Source/WebKit2/Shared/WebMemorySampler.cpp:39
> > +const char separator = '\t';
>
> nit. static const char?
Out of curiosity, why?
(In reply to comment #22) > > > Source/WebKit2/Shared/WebMemorySampler.cpp:39 > > > +const char separator = '\t'; > > > > nit. static const char? > > Out of curiosity, why? Just to clarify the scope. It will be same irrespective of 'static' Am I right? > > Out of curiosity, why?
>
> Just to clarify the scope.
> It will be same irrespective of 'static'
>
> Am I right?
That is right. I was thinking the value will likely end up in the __text segment anyway so why bother with static... But I see your point about making the scope explicit.
Comment on attachment 162649 [details] patch. Clearing flags on attachment: 162649 Committed r127828: <http://trac.webkit.org/changeset/127828> All reviewed patches have been landed. Closing bug. |