A lot of String::format() calls can now be replaced by using wtf/text/StringConcatenate. As first step, I only chose to eliminate the easy cases, those that don't involve floating-point (%..f)/ hex number formattings, and those which don't dump pointers (%p).
Created attachment 70740 [details] Patch Attention, this needs a careful review, as it touches a lot of different places.
I'm not sure whether the changed xssAuditor tests are fine, CC'ing Adam to comment.
Attachment 70740 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/platform/graphics/mac/GraphicsLayerCA.mm:251: Line contains tab character. [whitespace/tab] [5] Total errors found: 1 in 45 files If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 70740 [details] did not build on qt: Build output: http://queues.webkit.org/results/4456037
Created attachment 70744 [details] Patch v2 Fix style and Qt build.
Comment on attachment 70744 [details] Patch v2 Looks great, but one tiny problem. Our default char->UChar conversion is simple latin1, which is to say zero-extend. You should change 'char m_buffer;' to 'unsigned char m_buffer;' in the adapter class. Do you know what accounted for the change in layout test results? - might this be related? Otherwise all looks good. At some point in the future it might be nice rename makeString() to String::cat() – I'm not sure, but I think I like the idea of tying it to the class to implicitly indicate that the return type is String – but sticking with the same name from JSC is a fine decision for now, and that would be an easy find & replace later if we do want to do so.
(In reply to comment #6) > (From update of attachment 70744 [details]) > Looks great, but one tiny problem. > Our default char->UChar conversion is simple latin1, which is to say zero-extend. You should change 'char m_buffer;' to 'unsigned char m_buffer;' in the adapter class. > Do you know what accounted for the change in layout test results? - might this be related? I will test it soon. Are you saying only m_buffer should be changed? Not changing the adapter from <char> to -> <unsigned char> ?
Created attachment 70761 [details] Patch v3 Fixed Gavins suggestions. I hope Adam can comment first though, regarding the changed xssAuditor tests.
(In reply to comment #6) > (From update of attachment 70744 [details]) > Looks great, but one tiny problem. > Our default char->UChar conversion is simple latin1, which is to say zero-extend. You should change 'char m_buffer;' to 'unsigned char m_buffer;' in the adapter class. > Do you know what accounted for the change in layout test results? - might this be related? Note: This didn't affect the xssAuditor tests, still changed, let's wait for Adam to give ok.
Comment on attachment 70761 [details] Patch v3 r+, contingent on an okay on the results change.
(In reply to comment #9) > Note: This didn't affect the xssAuditor tests, still changed, let's wait for Adam to give ok. I can tell you exactly why the xssAuditor test results changed. The XSSAuditor::canLoadObject code converted a URL to UTF-8, then converted it back to Unicode, interpreting the UTF-8 bytes as Latin-1 when constructing UTF-16. The the console logging mechanism converts the now-incorrect round-tripped UTF-16 data to UTF-8. This process garbled the URL. The new code is correct, avoiding the incorrect round trip that garbles the URL. We can wait for Adam to weigh in if you like, but I don’t think we need to. It’s clear to me the old XSSAuditor::canLoadObject code was wrong and the new code is right. But since the XSSAuditor::canLoadObject change does fix a bug, while the rest of the patch is refactoring that should have no effect on behavior, if you’d like to be scrupulous you could just remove that XSSAuditor::canLoadObject change and land it separately along with the test result changes. But I think this can just go in as-is.
(In reply to comment #11) > (In reply to comment #9) > > Note: This didn't affect the xssAuditor tests, still changed, let's wait for Adam to give ok. > > I can tell you exactly why the xssAuditor test results changed. > > The XSSAuditor::canLoadObject code converted a URL to UTF-8, then converted it back to Unicode, interpreting the UTF-8 bytes as Latin-1 when constructing UTF-16. The the console logging mechanism converts the now-incorrect round-tripped UTF-16 data to UTF-8. This process garbled the URL. > > The new code is correct, avoiding the incorrect round trip that garbles the URL. Ah, thanks for the analysis, it's clear now. > > We can wait for Adam to weigh in if you like, but I don’t think we need to. It’s clear to me the old XSSAuditor::canLoadObject code was wrong and the new code is right. > > But since the XSSAuditor::canLoadObject change does fix a bug, while the rest of the patch is refactoring that should have no effect on behavior, if you’d like to be scrupulous you could just remove that XSSAuditor::canLoadObject change and land it separately along with the test result changes. > > But I think this can just go in as-is. Okay, thanks a lot, I'm landing.
Comment on attachment 70761 [details] Patch v3 The XSS auditor rebaselines are fine. Thanks for giving me a chance to check,
(In reply to comment #13) > (From update of attachment 70761 [details]) > The XSS auditor rebaselines are fine. Thanks for giving me a chance to check, Excellent, now invoking webkit-patch land.. (Note, I already mailed Adam before, asking to check these rebaselines, so it was fair to wait :-)
Committed r69798: <http://trac.webkit.org/changeset/69798>