Bug 47664 - Replace lots of String::format() usages by StringConcatenate
Summary: Replace lots of String::format() usages by StringConcatenate
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords:
Depends on:
Blocks: 30342 18994
  Show dependency treegraph
 
Reported: 2010-10-14 05:03 PDT by Nikolas Zimmermann
Modified: 2019-02-03 11:11 PST (History)
6 users (show)

See Also:


Attachments
Patch (55.05 KB, patch)
2010-10-14 08:37 PDT, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch v2 (55.32 KB, patch)
2010-10-14 09:23 PDT, Nikolas Zimmermann
barraclough: review-
Details | Formatted Diff | Diff
Patch v3 (55.33 KB, patch)
2010-10-14 11:41 PDT, Nikolas Zimmermann
barraclough: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 2010-10-14 05:03:26 PDT
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).
Comment 1 Nikolas Zimmermann 2010-10-14 08:37:51 PDT
Created attachment 70740 [details]
Patch

Attention, this needs a careful review, as it touches a lot of different places.
Comment 2 Nikolas Zimmermann 2010-10-14 08:38:27 PDT
I'm not sure whether the changed xssAuditor tests are fine, CC'ing Adam to comment.
Comment 3 WebKit Review Bot 2010-10-14 08:40:32 PDT
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.
Comment 4 Early Warning System Bot 2010-10-14 09:11:37 PDT
Attachment 70740 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4456037
Comment 5 Nikolas Zimmermann 2010-10-14 09:23:00 PDT
Created attachment 70744 [details]
Patch v2

Fix style and Qt build.
Comment 6 Gavin Barraclough 2010-10-14 11:16:48 PDT
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.
Comment 7 Nikolas Zimmermann 2010-10-14 11:23:43 PDT
(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> ?
Comment 8 Nikolas Zimmermann 2010-10-14 11:41:21 PDT
Created attachment 70761 [details]
Patch v3

Fixed Gavins suggestions. I hope Adam can comment first though, regarding the changed xssAuditor tests.
Comment 9 Nikolas Zimmermann 2010-10-14 11:42:32 PDT
(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 10 Gavin Barraclough 2010-10-14 11:59:00 PDT
Comment on attachment 70761 [details]
Patch v3

r+, contingent on an okay on the results change.
Comment 11 Darin Adler 2010-10-14 12:15:06 PDT
(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.
Comment 12 Nikolas Zimmermann 2010-10-14 12:27:47 PDT
(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 13 Adam Barth 2010-10-14 12:41:58 PDT
Comment on attachment 70761 [details]
Patch v3

The XSS auditor rebaselines are fine.  Thanks for giving me a chance to check,
Comment 14 Nikolas Zimmermann 2010-10-14 12:44:39 PDT
(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 :-)
Comment 15 Nikolas Zimmermann 2010-10-14 12:49:40 PDT
Committed r69798: <http://trac.webkit.org/changeset/69798>