[webkit-reviews] review granted: [Bug 24904] Disentangle standards mode CSS type check from content sniffing : [Attachment 29323] git format-patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Adam Barth
<abarth at webkit.org>'s request for review:
Bug 24904: Disentangle standards mode CSS type check from content sniffing
https://bugs.webkit.org/show_bug.cgi?id=24904

Attachment 29323: git format-patch
https://bugs.webkit.org/attachment.cgi?id=29323&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
Testing with Safari on Mac OS X 10.5.6, this patch actually fixes a regression
on the included test - since its extension is .cgi, the resource is sniffed as
text/plain, so the stylesheet isn't applied in ToT.

I'd love to also have tests for a stylesheet with .css extension but no
Content-Type, and for text/plain content type. Since an earlier version of this
patch broke stylesheets with charset in Content-Type, we definitely need a test
verifying that those work.

In failure case, Web Inspector shows a warning such as "Resource interpreted as
stylesheet but transferred with MIME type text/plain." That's a lie - it should
be an error, saying that that the stylesheet wasn't applied because of this
issue. It might be appropriate to fix this now.

+	 <...>	This test
+	 isn't perfect: style.cgi sends "Content-Type: " instead of
+	 not sending the header at all <..>

That's not necessarily bad - at least one of the sites you gave as a reference
in comment 17 sends exactly that.

+    layoutTestController.waitUntilDone();

There shouldn't be any need to do this - onload fires before a test finishes by
default in DRT.

+    // For protocols, like file:// and ftp:// that don't have a Content-Type

There is an extra comma after "protocols". Also, the comment doesn't match what
the code does - even though file:// and ftp:// were likely the reason to permit
an empty content type, we permit them for all protocols.

r=me with only these minor issues fixed if you'd like to be done with this bug
ASAP, but please consider making more tests, and maybe fixing the Web Inspector
problem.