Bug 107505 – Abstract the logic for appending a UChar32 onto StringBuilder

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Martin Robinson]

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.

[Martin Robinson]

Created attachment 184244 [details]
Patch for landing

[Martin Robinson]

Comment on attachment 184244 [details]
Patch for landing

Sorry, apparently I hit a bug in webkit-patch. This is for another bug.

[Martin Robinson]

Created attachment 184246 [details]
Patch

[Darin Adler]

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.

[Benjamin Poulain]

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.

[Benjamin Poulain]

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.

[Martin Robinson]

(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.

[Martin Robinson]

Created attachment 184393 [details]
Patch for final review and landing

[Build Bot]

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

[Benjamin Poulain]

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.

[Build Bot]

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

[Build Bot]

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

[Martin Robinson]

Created attachment 184484 [details]
Patch for final review and landing

[WebKit Review Bot]

Comment on attachment 184484 [details]
Patch for final review and landing

Clearing flags on attachment: 184484

Committed r140731: <http://trac.webkit.org/changeset/140731>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.