WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
95508
Remove all-but-one use of WTF::String::operator+= from WebCore
https://bugs.webkit.org/show_bug.cgi?id=95508
Summary
Remove all-but-one use of WTF::String::operator+= from WebCore
Adam Barth
Reported
2012-08-30 16:11:49 PDT
Remove all-but-one use of WTF::String::operator+= from WebCore
Attachments
Patch
(18.62 KB, patch)
2012-08-30 16:20 PDT
,
Adam Barth
no flags
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Adam Barth
Comment 1
2012-08-30 16:20:08 PDT
Created
attachment 161579
[details]
Patch
Eric Seidel (no email)
Comment 2
2012-08-30 16:47:27 PDT
Comment on
attachment 161579
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=161579&action=review
> Source/WebCore/inspector/InspectorStyleTextEditor.cpp:107 > + textToSet.append(formatLineFeed);
It seems this function wants a StringBuilder. Maybe needs a fixme.
> Source/WebCore/inspector/NetworkResourcesData.cpp:116 > m_content = m_decoder->decode(m_dataBuffer->data(), m_dataBuffer->size()); > - m_content += m_decoder->flush(); > + m_content.append(m_decoder->flush());
It seems decoder wants a decodeAndFlush command. :) Or a ShouldFlush enum (like I think it used to ahve).
Adam Barth
Comment 3
2012-08-30 16:52:56 PDT
(In reply to
comment #2
)
> (From update of
attachment 161579
[details]
) > View in context:
https://bugs.webkit.org/attachment.cgi?id=161579&action=review
> > > Source/WebCore/inspector/InspectorStyleTextEditor.cpp:107 > > + textToSet.append(formatLineFeed); > > It seems this function wants a StringBuilder. Maybe needs a fixme.
Yeah, the problem is that it's doing a bunch of complex insert operations as well. I can add a FIXME.
Benjamin Poulain
Comment 4
2012-08-30 18:32:20 PDT
Comment on
attachment 161579
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=161579&action=review
I am only half convinced we should have String::append() at all. This is an improvement in any case.
> Source/WebCore/platform/chromium/support/WebHTTPLoadInfo.cpp:109 > - result.iterator->second += "\n" + String(value); > + result.iterator->second.append("\n" + String(value));
One of those ugly case where the verbose way is better: result.iterator->second.append = result.iterator->second.append + '\n' + String(value);
> Source/WebCore/platform/chromium/support/WebURLResponse.cpp:264 > - result.iterator->second += ", " + valueStr; > + result.iterator->second.append(", " + valueStr);
The ugly case.
> Source/WebCore/xml/XMLHttpRequest.cpp:922 > - result.iterator->second += ", " + value; > + result.iterator->second.append(", " + value);
Another case for result.iterator->second.append = result.iterator->second.append + ", " + value);
> Source/WebCore/xml/XPathFunctions.cpp:-565 > - // FIXME: Building a String a character at a time is quite slow. > for (unsigned i1 = 0; i1 < s1.length(); ++i1) { > UChar ch = s1[i1]; > size_t i2 = s2.find(ch); > > if (i2 == notFound) > - newString += String(&ch, 1); > - else if (i2 < s3.length()) { > - UChar c2 = s3[i2]; > - newString += String(&c2, 1); > - }
:)
Adam Barth
Comment 5
2012-08-30 18:37:05 PDT
Thanks for the review. I will uglify the code before landing.
Adam Barth
Comment 6
2012-08-30 18:37:13 PDT
:)
Darin Adler
Comment 7
2012-08-31 11:57:25 PDT
Comment on
attachment 161579
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=161579&action=review
>> Source/WebCore/inspector/NetworkResourcesData.cpp:116 >> + m_content.append(m_decoder->flush()); > > It seems decoder wants a decodeAndFlush command. :) Or a ShouldFlush enum (like I think it used to ahve).
It would be good to have a “decode all at once” function that does the decode plus the flush. Simplistic uses of the decoder all want that. I don’t think adding an argument to decode is the way to do it, though. I’d prefer the new function. Since flush almost always returns an empty string, there is not a significant performance issue caused by using append in the implementation, whether at these call sites or inside the helper function. Basically, the flush function will only return characters if there is a trailing partial character sequence. Not really that common.
>> Source/WebCore/platform/chromium/support/WebHTTPLoadInfo.cpp:109 >> + result.iterator->second.append("\n" + String(value)); > > One of those ugly case where the verbose way is better: > result.iterator->second.append = result.iterator->second.append + '\n' + String(value);
With a some work, we could make the append function call style get the same performance as the verbose way. More template meta-programming work, but not too terribly difficult.
Adam Barth
Comment 8
2012-09-01 01:18:00 PDT
Ok. I'm going to land with the prettier idiom for now because these are fairly rare cases.
Adam Barth
Comment 9
2012-09-01 01:19:27 PDT
Comment on
attachment 161579
[details]
Patch Clearing flags on attachment: 161579 Committed
r127366
: <
http://trac.webkit.org/changeset/127366
>
Adam Barth
Comment 10
2012-09-01 01:19:30 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug