Summary: | Way too many encoding aliases are treated as valid | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexey Proskuryakov <ap> | ||||
Component: | DOM | Assignee: | Alexey Proskuryakov <ap> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | abarth, darin, eric, webkit.review.bot | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | All | ||||||
OS: | All | ||||||
Attachments: |
|
Description
Alexey Proskuryakov
2010-08-05 05:17:25 PDT
Created attachment 63585 [details]
proposed patch
Attachment 63585 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/text/TextEncodingRegistry.cpp:162: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
Total errors found: 1 in 11 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Is this going to cause problem with WebKit-specific content that relies on this behavior that we've had for the past 7 or so years? Comment on attachment 63585 [details] proposed patch > +// If passed any non-ASCII characters, depends on the behavior of isalnum -- > +// if that returns false as it does on OS X, then it will properly skip those > +// characters too. This comment is not true, and should be removed. > struct TextEncodingNameHash { With the changes you have made, is TextEncodingNameHash different from CaseFoldingHash? Can we just use CaseFoldingHash instead? > + if (0 == strcmp(alias, "8859_1")) > + return true; Would be good to add a comment explaining why we don't want this alias. Generally speaking this seems like a good patch, but I am slightly worried about backward compatibility. Comment on attachment 63585 [details] proposed patch If you haven't landed this, there is a typo in the ChangeLog: > Index: WebCore/ChangeLog > =================================================================== > + * platform/text/TextEncoding.cpp: (WebCore::Latin1Encoding): > + "Latin-1" is not a real encoding name, it's not known ot Firefox or IE. TYPO: "not known ot Firefox" => "not known to Firefox" > With the changes you have made, is TextEncodingNameHash different from CaseFoldingHash? > Can we just use CaseFoldingHash instead? I'm not sure - the code is different, as it's ASCII-only, but maybe the difference in behavior/performance is negligible. > Is this going to cause problem with WebKit-specific content that relies on this behavior > that we've had for the past 7 or so years? That seems likely, yes. But I think that the extent of forgiveness has changed semi-recently, with the introduction of TextEncodingRegistry. Maybe my memory is wrong. Committed <http://trac.webkit.org/changeset/64817>. http://trac.webkit.org/changeset/64817 might have broken GTK Linux 64-bit Debug Follow up fix in <http://trac.webkit.org/changeset/64820> - added "shift-jis". More follow-up fixing in r64831 - don't register ISO8859-16, which was causing crashes on platforms with older ICU, and which isn't needed (at least, not yet). |