Bug 47538

Summary:

Unify JSC::StringBuilder & WebCore::StringBuilder

Product:

WebKit

Reporter:

Nikolas Zimmermann <zimmermann>

Component:

Web Template Framework

Assignee:

Nikolas Zimmermann <zimmermann>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ap, barraclough, darin, dglazkov, ggaren, simon.fraser, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

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+

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.