Bug 150436

Summary: Enable MediaSource::isTypeSupported() to handle the upper-cased MIME type & Codec
Product: WebKit Reporter: Hyunduk Kim <hyunduk.kim>
Component: MediaAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, eric.carlson, jer.noble
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews112 for mac-yosemite
none
Patch
none
Archive of layout-test-results from ews113 for mac-yosemite
none
Patch
none
Patch none

Description Hyunduk Kim 2015-10-21 23:23:29 PDT
The MIME type should not be case-sensitive.

So MediaSource::isTypeSupported() should handle the same.
Comment 1 Hyunduk Kim 2015-10-21 23:24:16 PDT
I'm working on the same.
Comment 2 Hyunduk Kim 2015-10-21 23:32:17 PDT
I meant not only MIME type but also codec.
For example, 
AUDIO/WEBM;CODECS="vorbis"
should work properly.
Comment 3 Hyunduk Kim 2015-10-22 02:20:34 PDT
Created attachment 263810 [details]
Patch
Comment 4 Build Bot 2015-10-22 03:09:14 PDT
Comment on attachment 263810 [details]
Patch

Attachment 263810 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/320924

New failing tests:
http/tests/media/media-source/mediasource-is-type-supported.html
Comment 5 Build Bot 2015-10-22 03:09:16 PDT
Created attachment 263813 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 6 Hyunduk Kim 2015-10-22 03:11:35 PDT
Created attachment 263814 [details]
Patch
Comment 7 Build Bot 2015-10-22 03:59:04 PDT
Comment on attachment 263814 [details]
Patch

Attachment 263814 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/321062

New failing tests:
http/tests/media/media-source/mediasource-is-type-supported.html
Comment 8 Build Bot 2015-10-22 03:59:06 PDT
Created attachment 263818 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 9 Hyunduk Kim 2015-10-22 04:24:35 PDT
Created attachment 263819 [details]
Patch
Comment 10 Hyunduk Kim 2015-10-22 23:51:20 PDT
Newly added TC is passed in the Chrome Browser ( Version 46.0.2490.71 m ).
Comment 11 Eric Carlson 2015-10-23 09:18:23 PDT
Comment on attachment 263819 [details]
Patch

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

> LayoutTests/http/tests/media/media-source/mediasource-is-type-supported-expected.txt:30
> -PASS Test valid WebM type "audio/webm;codecs="vorbis"" 
> +PASS Test valid WebM type "audio/webm;codecs="vorbis""
> +PASS Test valid WebM type "AUDIO/WEBM;CODECS="vorbis""
>  PASS Test valid MP4 type "video/mp4;codecs="avc1.4d001e"" 
>  PASS Test valid MP4 type "video/mp4;codecs="avc1.42001e"" 

This should be tested on all ports, so you should also have an upper/mixed case type that is supported by ports that don't support webm/vorbis.
Comment 12 Hyunduk Kim 2015-10-25 18:49:58 PDT
Created attachment 264027 [details]
Patch
Comment 13 Hyunduk Kim 2015-10-25 21:12:47 PDT
I added upper/mixed case for both webm and mp4.
Comment 14 Darin Adler 2015-10-31 10:33:07 PDT
Comment on attachment 264027 [details]
Patch

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

> Source/WebCore/Modules/mediasource/MediaSource.cpp:732
>      if (type.isNull() || type.isEmpty())

Not part of your patch, but this line of code is slightly wasteful. The isNull check isn’t needed because all null strings are empty strings too.

> Source/WebCore/Modules/mediasource/MediaSource.cpp:735
> +    ContentType contentType(type.lower());

There are two issues here.

When we want to fold case for something like this, we want to fold only ASCII case, not full Unicode case. That means we should use functions like convertToASCIILowercase() rather than lower().

I am not sure that this call site is the right level to fold case. We should study the other clients of ContentType and see why they don’t need lowercasing. If they do, then convertToASCIILowercase should be done in the ContentType constructor, not here. There’s even another use of ContentType in the same file that might need the case folding. The problem with that is that changing ContentType will affect a lot more code paths and we’ll have to do more testing, and possibly remove some now-unneeded code that does additional lowercasing in other cases that use ContentType.

I am going to say review+ but we should do better on both of those issues if we can.
Comment 15 Hyunduk Kim 2015-11-02 03:01:36 PST
Thank you for your comment, Darin.

I'll check what you mentioned, too.
Comment 16 WebKit Commit Bot 2015-11-02 08:12:37 PST
Comment on attachment 264027 [details]
Patch

Clearing flags on attachment: 264027

Committed r191888: <http://trac.webkit.org/changeset/191888>
Comment 17 WebKit Commit Bot 2015-11-02 08:12:42 PST
All reviewed patches have been landed.  Closing bug.