Bug 150436 – Enable MediaSource::isTypeSupported() to handle the upper-cased MIME type & Codec

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Hyunduk Kim]

The MIME type should not be case-sensitive.

So MediaSource::isTypeSupported() should handle the same.

[Hyunduk Kim]

I'm working on the same.

[Hyunduk Kim]

I meant not only MIME type but also codec.
For example, 
AUDIO/WEBM;CODECS="vorbis"
should work properly.

[Hyunduk Kim]

Created attachment 263810 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Hyunduk Kim]

Created attachment 263814 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Hyunduk Kim]

Created attachment 263819 [details]
Patch

[Hyunduk Kim]

Newly added TC is passed in the Chrome Browser ( Version 46.0.2490.71 m ).

[Eric Carlson]

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.

[Hyunduk Kim]

Created attachment 264027 [details]
Patch

[Hyunduk Kim]

I added upper/mixed case for both webm and mp4.

[Darin Adler]

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.

[Hyunduk Kim]

Thank you for your comment, Darin.

I'll check what you mentioned, too.

[WebKit Commit Bot]

Comment on attachment 264027 [details]
Patch

Clearing flags on attachment: 264027

Committed r191888: <http://trac.webkit.org/changeset/191888>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.