Bug 24903 – Text files which have sub MIME types are treated as not displayable

• Status: REOPENED
• Component: WebCore Misc.

[Christian Dywan]

WebKit has an idea of text, image and plugin types it can display. Unfortunately it doesn't recognize sub types such as text/x-c and similar types. WebCore actually double checks in the "use" policy delegate so applications can't even override this.

I'm looking what the correct fix is right now.

[Christian Dywan]

Created attachment 29033 [details]
Recognize sub types of text/plain

Since we have a fixed list of supported types, and text/plain is among the supported types, I think the correct fix is regarding any subtype of text/plain as valid. This patch does that.

I'm wondering if we can use this for images as well, ie. png images with unusual MIME types, but I don't have examples to test this right now.

[Christian Dywan]

Okay, I actually found an image, of type image/x-png, but it doesn't work like that it seems.

[Darin Adler]

Comment on attachment 29033 [details]
Recognize sub types of text/plain

The issue here seems to be a shortcoming in MIMETypeRegistry::isSupportedNonImageMIMEType, and should be fixed there.

[Christian Dywan]

Created attachment 29084 [details]
Recognize any text/* type

This is a different approach, now isSupportedNonImageMIMEType actually reports any type starting with "text/" as supported.

[Darin Adler]

Comment on attachment 29084 [details]
Recognize any text/* type

Needs a change log.

Change looks good. I don't understand why these functions are case sensitive, but they are, so the new code matches what's already here.

Besides "text/", you should remove all the "text/*" MIME types from initializeSupportedNonImageMimeTypes because none of them do us any good. That includes "text/vnd.wap.wml" as well as the others that are not inside an ifdef.

Can you write a regression test too? There should be a way to test this in our HTTP tests if not the plain text tests.

review- because of lack of a change log and the other issues mentioned above.

[Darin Adler]

(In reply to comment #5)
> Can you write a regression test too? There should be a way to test this in our
> HTTP tests if not the plain text tests.

I meant "in our HTTP tests if not the local file tests".

[Christian Dywan]

Created attachment 30550 [details]
Recognize any text/* type #2

I updated the patch to remove all text/ types from the list and added a ChangeLog entry.

I didn't add a test because I'm rather clueless about adding new tests

[Darin Adler]

This would be *much* better with a test.

[Christian Dywan]

2009-05-21  Christian Dywan  <[e-mail]>

        Reviewed by Darin Adler.

        Text files which have sub MIME types are treated as not displayable
        http://bugs.webkit.org/show_bug.cgi?id=24903

        * platform/MIMETypeRegistry.cpp:
        (WebCore::initializeSupportedNonImageMimeTypes):
        (WebCore::MIMETypeRegistry::isSupportedNonImageMIMEType): Regard any
        MIME type beginning with "text/" as supported and remove all "text/"
        types from the list.

[Christian Dywan]

Comment on attachment 30550 [details]
Recognize any text/* type #2

I committed the patch.

Gustavo seems to have a test for this, so I'm leaving this open until the test is done.

[Gustavo Noronha (kov)]

Created attachment 30556 [details]
Test for acceptance of text/* types

 LayoutTests/ChangeLog                              |    9 ++++++
 .../tests/mime/accept-all-text-types-expected.txt  |    4 +++
 .../http/tests/mime/accept-all-text-types.html     |   27 ++++++++++++++++++++
 3 files changed, 40 insertions(+), 0 deletions(-)

[Gustavo Noronha (kov)]

Comment on attachment 30556 [details]
Test for acceptance of text/* types

Here's the test case, along with expected results. This is my first layout test, so please advise =).

[Gustavo Noronha (kov)]

Landed the test. I forgot to add the cgi script on the first go, though, so added in a second commit.

Rolled out this patch in r44000 because it caused bug 25947

[Brady Eidson]

This broke all layout tests - run them before landing patches!!!  =D

Darin had a comment:
"Besides "text/", you should remove all the "text/*" MIME types from
initializeSupportedNonImageMimeTypes because none of them do us any good. That
includes "text/vnd.wap.wml" as well as the others that are not inside an ifdef."

This was incorrect.  In addition to answering the question "is this a supported non image mimetype?", MIMETypeRegistry also has the role of delivering a collection of all known non-image MIME types via the ::getSupportedNonImageMIMETypes() method.

That method is exposed to the WebKit layer of the various platforms who do various things with it.  This completely hosed the Mac platform as it no longer registered WebHTMLView as the view class for "text/html", since "text/html" never showed up in the list of supported MIME types. 

The other half of the change - having isSupportedNonImageMIMEType() return true if the passed in type starts with "text/" might be valid.  We might need further changes to the design here such that we can do fuzzy matching of subtypes.  But the full types we explicitly list in the registry must remaining explicitly listed!

[Mark Rowe (bdash)]

To be fair, it only broke all layout tests on Mac and Windows.  It didn't break GTK.

[Alexey Proskuryakov]

See also: bug 24618.

[Jan Alonzo]

Comment on attachment 30550 [details]
Recognize any text/* type #2

Clearing r+ as this patch was landed then rolled-out.

[Gustavo Noronha (kov)]

Created attachment 32143 [details]
Treat all text/ subtypes as supported

 WebCore/ChangeLog                     |   19 +++++++++++++++++++
 WebCore/platform/MIMETypeRegistry.cpp |    2 ++
 2 files changed, 21 insertions(+), 0 deletions(-)

[Evan Martin]

+abarth for mime changes

[Mark Rowe (bdash)]

Comment on attachment 32143 [details]
Treat all text/ subtypes as supported

This leaves an inconsistency between getSupportedNonImageMIMETypes and what isSupportedNonImageMIMEType will return.  I suspect this will again cause subtle problems for the Mac, as it uses both of these functions within WebKit/mac.

[Adam Barth]

Can we please add a test that "text/foobar" is doesn't render HTML tags?  I'm pretty sure it won't, but testing is better than guessing.  :)