Bug 107505

Summary: Abstract the logic for appending a UChar32 onto StringBuilder
Product: WebKit Reporter: Martin Robinson <mrobinson>
Component: Web Template FrameworkAssignee: Martin Robinson <mrobinson>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, buildbot, cmarcelo, d-r, fmalita, macpherson, menard, ojan.autocc, pdr, rniwa, schenney, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch for landing
none
Patch
none
Patch for final review and landing
none
Patch for final review and landing none

Description Martin Robinson 2013-01-21 23:20:18 PST
Several StringBuilder clients in WebCore want to a append a UChar32 onto a StringBuilder. Right now the logic for doing this is repeated at each place.
Comment 1 Martin Robinson 2013-01-23 08:41:35 PST
Created attachment 184244 [details]
Patch for landing
Comment 2 Martin Robinson 2013-01-23 08:43:19 PST
Comment on attachment 184244 [details]
Patch for landing

Sorry, apparently I hit a bug in webkit-patch. This is for another bug.
Comment 3 Martin Robinson 2013-01-23 08:45:33 PST
Created attachment 184246 [details]
Patch
Comment 4 Darin Adler 2013-01-23 09:35:27 PST
Comment on attachment 184246 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184246&action=review

Nice improvement at every call site!

> Source/WTF/wtf/text/StringBuilder.h:149
> +            UChar character = static_cast<UChar>(c);
> +            ASSERT(character == c);
> +            append(character);

Tiny nit: This seems like overkill to me. I would follow the CSSOMUtils.cpp version and just do this.

    append(static_cast<UChar>(c));

We can trust that U_IS_BMP works properly and don’t need to restate what it does in the form of an assertion.
Comment 5 Benjamin Poulain 2013-01-23 13:54:30 PST
Comment on attachment 184246 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=184246&action=review

> Source/WebCore/ChangeLog:9
> +        No new tests. This is just refactor, so it's covered by
> +        existing tests.

Actually, WTF has a growing corpus of C++ tests. They already caught some bugs in the past.

I would appreciate if you could spend 5 minutes extending the tests of StringBuilder of this use case.
Comment 6 Benjamin Poulain 2013-01-23 14:00:41 PST
Comment on attachment 184246 [details]
Patch

Actually, I am gonna cq-. I agree with Darin's r+ but I would like this to have its test.
Comment 7 Martin Robinson 2013-01-23 14:16:00 PST
(In reply to comment #6)
> (From update of attachment 184246 [details])
> Actually, I am gonna cq-. I agree with Darin's r+ but I would like this to have its test.

I don't mind adding a test, I'll do that and have you do a quick review before landing.
Comment 8 Martin Robinson 2013-01-23 21:08:51 PST
Created attachment 184393 [details]
Patch for final review and landing
Comment 9 Build Bot 2013-01-23 22:34:29 PST
Comment on attachment 184393 [details]
Patch for final review and landing

Attachment 184393 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16096002
Comment 10 Benjamin Poulain 2013-01-23 23:04:03 PST
Comment on attachment 184393 [details]
Patch for final review and landing

View in context: https://bugs.webkit.org/attachment.cgi?id=184393&action=review

Thank you for adding the test. Some comments bellow:

> Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:97
> +
> +    StringBuilder builder3;
> +    UChar32 frakturAChar = 0x1D504;
> +    builder3.append(frakturAChar); // The fraktur A is not in the BMP, so it's two UTF-16 code units long.
> +    ASSERT_EQ(2, builder3.length());
> +    builder3.append(static_cast<UChar32>('A'));
> +    ASSERT_EQ(3, builder3.length());
> +    ASSERT_EQ(builder3[0], U16_LEAD(frakturAChar));
> +    ASSERT_EQ(builder3[1], U16_TRAIL(frakturAChar));
> +    ASSERT_EQ(builder3[2], 'A');

Please add a little comment saying this is testing the append(UChar32).

Instead of ASSERT_EQ(builder3[0], U16_LEAD(frakturAChar));
Please to builder3.toString()  -> then test the characters of the string.
This way, we test append() end to end and make sure the builder does its job at building strings.
Comment 11 Build Bot 2013-01-23 23:09:47 PST
Comment on attachment 184393 [details]
Patch for final review and landing

Attachment 184393 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16034031
Comment 12 Build Bot 2013-01-24 00:16:59 PST
Comment on attachment 184393 [details]
Patch for final review and landing

Attachment 184393 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16073652
Comment 13 Martin Robinson 2013-01-24 06:35:40 PST
Created attachment 184484 [details]
Patch for final review and landing
Comment 14 WebKit Review Bot 2013-01-24 15:02:37 PST
Comment on attachment 184484 [details]
Patch for final review and landing

Clearing flags on attachment: 184484

Committed r140731: <http://trac.webkit.org/changeset/140731>
Comment 15 WebKit Review Bot 2013-01-24 15:02:43 PST
All reviewed patches have been landed.  Closing bug.