[webkit-reviews] review granted: [Bug 101527] CSS charset parsing is too loose, doesn't match other browsers : [Attachment 173120] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Tab Atkins
<tabatkins at google.com>'s request for review:
Bug 101527: CSS charset parsing is too loose, doesn't match other browsers
https://bugs.webkit.org/show_bug.cgi?id=101527

Attachment 173120: Patch
https://bugs.webkit.org/attachment.cgi?id=173120&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=173120&action=review


Oh, css-charset-evil.html says that both Firefox and IE used to pass.
Especially with IE, the behavior of old versions is important.

I suggest going ahead with this one, but toning it down a little, so that
people would not be confused about our rationale in case actual content
regresses.

> Source/WebCore/ChangeLog:3
> +	   Charset parsing is too loose, doesn't match other browsers

Please update ChangeLog to match new bug title. It's way too scary as is.

> Source/WebCore/ChangeLog:9
> +	   Per
<https://www.w3.org/Bugs/Public/show_bug.cgi?id=19882#attach_1244>,
> +	   IE and FF are strict about CSS @charset parsing.

Please verify that the added tests all pass at least in Firefox, and preferably
in both browsers.

> Source/WebCore/ChangeLog:10
> +	   We should match them and the spec.

Should mention that this is a recent behavior change in IE and Firefox.

I disagree with this rationale. An accurate one would be something like "since
we think that @charset is very uncommon, we can as well match the new behavior
for platform consistency".

> Source/WebCore/ChangeLog:17
> +	   (WebCore):

prepare-ChangeLog generates these lines, but ChangeLogs are meant for human
consumption, and should be edited by hand for best readability.

> LayoutTests/ChangeLog:15
> +	   (#a1ä):

Somewhat more importantly, please remove these garbage lines.