Bug 47538

Summary: Unify JSC::StringBuilder & WebCore::StringBuilder
Product: WebKit Reporter: Nikolas Zimmermann <zimmermann>
Component: Web Template FrameworkAssignee: Nikolas Zimmermann <zimmermann>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, barraclough, darin, dglazkov, ggaren, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 18994    
Attachments:
Description Flags
Patch
none
Patch v2
none
Patch v3
none
Patch v4 barraclough: review+

Nikolas Zimmermann
Reported 2010-10-12 02:35:18 PDT
JSC::StringBuilder operates on a Vector<UChar, 64> and allows to efficiently build a string based on a stream of characters. WebCore::StringBuilder operates on a Vector<String, 16> and allows to concat a set of arbitary Strings, it's not very efficient for single characters, as a String is built for each of them. When evaluating the usages of WebCore::StringBuilder, all clients of this code, use it to combine Strings _and_ single characters (grep -RI StringBuilder * in WebCore). My proposal is to use the JSC::StringBuilder throughout WebCore & JSC, as it's more efficient, and reduces the duplicated code mess. Move it to wtf/text/StringBuilder, add a new UStringBuilder class to JSC/runtime that extends the StringBuilder to support appending/creating UStrings as well. This class can go away, once we've completed unifying WTFString & UString.
Attachments
Patch (82.89 KB, patch)
2010-10-12 03:30 PDT, Nikolas Zimmermann 		no flags
DetailsFormatted DiffDiff
Patch v2 (86.36 KB, patch)
2010-10-12 05:46 PDT, Nikolas Zimmermann 		no flags
DetailsFormatted DiffDiff
Patch v3 (90.64 KB, patch)
2010-10-12 05:58 PDT, Nikolas Zimmermann 		no flags
DetailsFormatted DiffDiff
Patch v4 (91.21 KB, patch)
2010-10-12 10:09 PDT, Nikolas Zimmermann 		barraclough: review+
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Nikolas Zimmermann
Comment 1 2010-10-12 03:30:54 PDT
Created attachment 70520 [details] Patch
WebKit Review Bot
Comment 2 2010-10-12 03:35:01 PDT
Attachment 70520 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeWin.cpp:49: Alphabetical sorting problem. [build/include_order] [4] WebCore/platform/graphics/win/MediaPlayerPrivateQuickTimeVisualContext.cpp:56: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 36 files If any of these errors are false positives, please file a bug against check-webkit-style.
Nikolas Zimmermann
Comment 3 2010-10-12 03:35:23 PDT
The WebCore/ChangeLog is still missing some information, only marked it for review, to get EWS results.
Nikolas Zimmermann
Comment 4 2010-10-12 05:46:53 PDT
Created attachment 70523 [details] Patch v2 Fixed a regression, now all tests pass as expected, also simplifies Node.h a bit.
WebKit Review Bot
Comment 5 2010-10-12 05:47:16 PDT
Attachment 70520 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4334033
Nikolas Zimmermann
Comment 6 2010-10-12 05:48:33 PDT
(In reply to comment #5) > Attachment 70520 [details] did not build on chromium: > Build output: http://queues.webkit.org/results/4334033 Fixing...
Nikolas Zimmermann
Comment 7 2010-10-12 05:58:40 PDT
Created attachment 70524 [details] Patch v3 Chromium is using StringBuilder from WebKit/chromium/src - didn't know that, uploading new patch, which attempts to fix the Chromium build.
WebKit Review Bot
Comment 8 2010-10-12 08:41:59 PDT
Attachment 70524 [details] did not build on chromium: Build output: http://queues.webkit.org/results/4321039
Nikolas Zimmermann
Comment 9 2010-10-12 10:09:44 PDT
Created attachment 70540 [details] Patch v4 Hopefully chromium builds this time...
Gavin Barraclough
Comment 10 2010-10-12 12:09:17 PDT
Comment on attachment 70540 [details] Patch v4 Looks great.
Nikolas Zimmermann
Comment 11 2010-10-12 12:48:35 PDT
Committed r69594: <http://trac.webkit.org/changeset/69594>
Nikolas Zimmermann
Comment 12 2010-10-12 12:56:56 PDT
Landed in r69594.
Note You need to log in before you can comment on or make changes to this bug.