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.
Created attachment 184244 [details] Patch for landing
Comment on attachment 184244 [details] Patch for landing Sorry, apparently I hit a bug in webkit-patch. This is for another bug.
Created attachment 184246 [details] Patch
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 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 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.
(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.
Created attachment 184393 [details] Patch for final review and landing
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 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 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 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
Created attachment 184484 [details] Patch for final review and landing
Comment on attachment 184484 [details] Patch for final review and landing Clearing flags on attachment: 184484 Committed r140731: <http://trac.webkit.org/changeset/140731>
All reviewed patches have been landed. Closing bug.