Bug 227464 – Encoding specified in the 'charset' attribute should have precedence over 'content' attribute

• Component: New Bugs
• Status: RESOLVED FIXED

[Qiaosong Zhou]

Encoding specified in the 'charset' attribute should have precedence over 'content' attribute

[Alex Christensen]

*** Bug 227463 has been marked as a duplicate of this bug. ***

[Qiaosong Zhou]

Created attachment 432428 [details]
Patch

[Alex Christensen]

Comment on attachment 432428 [details]
Patch

Hooray!  You successfully uploaded a patch.

[Qiaosong Zhou]

Created attachment 432444 [details]
Patch

[Geoffrey Garen]

Comment on attachment 432444 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=432444&action=review

r=me

> Source/WebCore/ChangeLog:9
> +        (WebCore::HTMLMetaCharsetParser::encodingFromMetaAttributes):

It's nice to write a little explanation here about what made the previous logic incorrect, and how the new logic corrects it. Even if the case is pretty simple like this one.

[Chris Dumez]

Comment on attachment 432444 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=432444&action=review

>> Source/WebCore/ChangeLog:9
>> +        (WebCore::HTMLMetaCharsetParser::encodingFromMetaAttributes):
> 
> It's nice to write a little explanation here about what made the previous logic incorrect, and how the new logic corrects it. Even if the case is pretty simple like this one.

Also, for compatibility changes such as this one, please make sure to test the behavior in Chrome and Firefox and provide the results in the ChangeLog. It is important as fixing tests may break compatibility with other browsers in some cases (because other browsers also fail said test).

[Chris Dumez]

(In reply to Chris Dumez from comment #6)
> Comment on attachment 432444 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=432444&action=review
> 
> >> Source/WebCore/ChangeLog:9
> >> +        (WebCore::HTMLMetaCharsetParser::encodingFromMetaAttributes):
> > 
> > It's nice to write a little explanation here about what made the previous logic incorrect, and how the new logic corrects it. Even if the case is pretty simple like this one.
> 
> Also, for compatibility changes such as this one, please make sure to test
> the behavior in Chrome and Firefox and provide the results in the ChangeLog.
> It is important as fixing tests may break compatibility with other browsers
> in some cases (because other browsers also fail said test).

For this particular test, I have verified via https://wpt.live/html/syntax/parsing/meta-inhead-insertion-mode.html that it passes on both Firefox and Chrome.

[Alex Christensen]

Comment on attachment 432444 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=432444&action=review

> Source/WebCore/html/parser/HTMLMetaCharsetParser.cpp:115
>                  mode = Charset;

We could also increase performance a little by putting "break;" after this line instead of checking if mode != Charset for the rest of the attributes.

[Qiaosong Zhou]

Created attachment 432516 [details]
Patch

[Chris Dumez]

Comment on attachment 432516 [details]
Patch

Where are the layout test changes?

[Qiaosong Zhou]

Created attachment 432522 [details]
Patch

[Chris Dumez]

Comment on attachment 432522 [details]
Patch

Again this patch does not contain any test or test rebaseline.

[Qiaosong Zhou]

Created attachment 432523 [details]
Patch

[EWS]

Committed r279391 (239256@main): <https://commits.webkit.org/239256@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 432523 [details].