Bug 24903 - Text files which have sub MIME types are treated as not displayable
Summary: Text files which have sub MIME types are treated as not displayable
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-03-27 15:06 PDT by Christian Dywan
Modified: 2010-06-10 17:44 PDT (History)
3 users (show)

See Also:


Attachments
Recognize sub types of text/plain (1.46 KB, patch)
2009-03-27 17:04 PDT, Christian Dywan
darin: review-
Details | Formatted Diff | Diff
Recognize any text/* type (805 bytes, patch)
2009-03-30 13:41 PDT, Christian Dywan
darin: review-
Details | Formatted Diff | Diff
Recognize any text/* type #2 (1.66 KB, patch)
2009-05-21 12:46 PDT, Christian Dywan
no flags Details | Formatted Diff | Diff
Test for acceptance of text/* types (2.32 KB, patch)
2009-05-21 14:27 PDT, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff
Treat all text/ subtypes as supported (1.58 KB, patch)
2009-07-01 14:39 PDT, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Christian Dywan 2009-03-27 15:06:09 PDT
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.
Comment 1 Christian Dywan 2009-03-27 17:04:00 PDT
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.
Comment 2 Christian Dywan 2009-03-27 17:10:12 PDT
Okay, I actually found an image, of type image/x-png, but it doesn't work like that it seems.
Comment 3 Darin Adler 2009-03-30 09:49:14 PDT
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.
Comment 4 Christian Dywan 2009-03-30 13:41:55 PDT
Created attachment 29084 [details]
Recognize any text/* type

This is a different approach, now isSupportedNonImageMIMEType actually reports any type starting with "text/" as supported.
Comment 5 Darin Adler 2009-03-30 14:03:28 PDT
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.
Comment 6 Darin Adler 2009-03-30 14:06:02 PDT
(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".
Comment 7 Christian Dywan 2009-05-21 12:46:54 PDT
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
Comment 8 Darin Adler 2009-05-21 12:48:11 PDT
This would be *much* better with a test.
Comment 9 Christian Dywan 2009-05-21 13:59:13 PDT
2009-05-21  Christian Dywan  <christian@twotoasts.de>

        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.
Comment 10 Christian Dywan 2009-05-21 14:00:17 PDT
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.
Comment 11 Gustavo Noronha (kov) 2009-05-21 14:27:31 PDT
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(-)
Comment 12 Gustavo Noronha (kov) 2009-05-21 14:28:23 PDT
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 =).
Comment 13 Gustavo Noronha (kov) 2009-05-21 14:55:58 PDT
Landed the test. I forgot to add the cgi script on the first go, though, so added in a second commit.
Comment 14 mitz 2009-05-21 16:24:50 PDT
Rolled out this patch in r44000 because it caused bug 25947
Comment 15 Brady Eidson 2009-05-21 17:06:15 PDT
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!
Comment 16 Mark Rowe (bdash) 2009-05-21 17:24:42 PDT
To be fair, it only broke all layout tests on Mac and Windows.  It didn't break GTK.
Comment 17 Alexey Proskuryakov 2009-05-21 20:53:42 PDT
See also: bug 24618.
Comment 18 Jan Alonzo 2009-05-22 20:50:23 PDT
Comment on attachment 30550 [details]
Recognize any text/* type #2

Clearing r+ as this patch was landed then rolled-out.
Comment 19 Gustavo Noronha (kov) 2009-07-01 14:39:59 PDT
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(-)
Comment 20 Evan Martin 2009-07-01 15:08:20 PDT
+abarth for mime changes
Comment 21 Mark Rowe (bdash) 2009-07-01 15:13:09 PDT
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.
Comment 22 Adam Barth 2009-07-01 15:31:20 PDT
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.  :)