Bug 47664 – Replace lots of String::format() usages by StringConcatenate

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

Created attachment 70740 [details]
Patch

Attention, this needs a careful review, as it touches a lot of different places.

[Nikolas Zimmermann]

I'm not sure whether the changed xssAuditor tests are fine, CC'ing Adam to comment.

[WebKit Review Bot]

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.

[Early Warning System Bot]

Attachment 70740 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4456037

[Nikolas Zimmermann]

Created attachment 70744 [details]
Patch v2

Fix style and Qt build.

[Gavin Barraclough]

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.

[Nikolas Zimmermann]

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

[Nikolas Zimmermann]

Created attachment 70761 [details]
Patch v3

Fixed Gavins suggestions. I hope Adam can comment first though, regarding the changed xssAuditor tests.

[Nikolas Zimmermann]

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

[Gavin Barraclough]

Comment on attachment 70761 [details]
Patch v3

r+, contingent on an okay on the results change.

[Darin Adler]

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

[Nikolas Zimmermann]

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

[Adam Barth]

Comment on attachment 70761 [details]
Patch v3

The XSS auditor rebaselines are fine.  Thanks for giving me a chance to check,

[Nikolas Zimmermann]

(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 :-)

[Nikolas Zimmermann]

Committed r69798: <http://trac.webkit.org/changeset/69798>