Bug 47538 - Unify JSC::StringBuilder & WebCore::StringBuilder
Summary: Unify JSC::StringBuilder & WebCore::StringBuilder
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on:
Blocks: 18994
  Show dependency treegraph
 
Reported: 2010-10-12 02:35 PDT by Nikolas Zimmermann
Modified: 2010-10-14 05:05 PDT (History)
7 users (show)

See Also:


Attachments
Patch (82.89 KB, patch)
2010-10-12 03:30 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v2 (86.36 KB, patch)
2010-10-12 05:46 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v3 (90.64 KB, patch)
2010-10-12 05:58 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v4 (91.21 KB, patch)
2010-10-12 10:09 PDT, Nikolas Zimmermann
barraclough: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 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.
Comment 1 Nikolas Zimmermann 2010-10-12 03:30:54 PDT
Created attachment 70520 [details]
Patch
Comment 2 WebKit Review Bot 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.
Comment 3 Nikolas Zimmermann 2010-10-12 03:35:23 PDT
The WebCore/ChangeLog is still missing some information, only marked it for review, to get EWS results.
Comment 4 Nikolas Zimmermann 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.
Comment 5 WebKit Review Bot 2010-10-12 05:47:16 PDT
Attachment 70520 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4334033
Comment 6 Nikolas Zimmermann 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...
Comment 7 Nikolas Zimmermann 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.
Comment 8 WebKit Review Bot 2010-10-12 08:41:59 PDT
Attachment 70524 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4321039
Comment 9 Nikolas Zimmermann 2010-10-12 10:09:44 PDT
Created attachment 70540 [details]
Patch v4

Hopefully chromium builds this time...
Comment 10 Gavin Barraclough 2010-10-12 12:09:17 PDT
Comment on attachment 70540 [details]
Patch v4

Looks great.
Comment 11 Nikolas Zimmermann 2010-10-12 12:48:35 PDT
Committed r69594: <http://trac.webkit.org/changeset/69594>
Comment 12 Nikolas Zimmermann 2010-10-12 12:56:56 PDT
Landed in r69594.