Bug 51195 - MediaPlayer should look for MIME type in data: URL
Summary: MediaPlayer should look for MIME type in data: URL
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-12-16 10:16 PST by Eric Carlson
Modified: 2010-12-23 01:26 PST (History)
6 users (show)

See Also:


Attachments
Proposed patch (1.75 KB, patch)
2010-12-16 10:52 PST, Eric Carlson
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2010-12-16 10:16:09 PST
MediaPlayer::load should try to get the MIME type from the url when passed a data: url.
Comment 1 Eric Carlson 2010-12-16 10:52:20 PST
Created attachment 76786 [details]
Proposed patch
Comment 2 Darin Adler 2010-12-16 10:59:06 PST
Comment on attachment 76786 [details]
Proposed patch

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

Can we make tests for this?

> WebCore/platform/graphics/MediaPlayer.cpp:306
> +    // If the MIME type is missing or is not meaningful, try to figure it out from the url.
> +    if (type.isEmpty() && protocolIs(url, "data"))

The comment says “is not meaningful”, but the code only checks for empty strings. Aren’t there other non-meaningful MIME types?

I suggest capitalizing URL in the comment.

I suggest we add protocolIsData to KURL.h. We already have it for actual KURL objects and I’d like to have it for strings. It’s not good to have the actual "data" string in the code. We should also adopt protocolIsData in KURL.cpp, KURLGoogle.cpp, DataURL.cpp, ResourceHandleSoup.cpp. None of that needs to be done right away or by you.
Comment 3 Eric Carlson 2010-12-16 11:09:30 PST
(In reply to comment #2)
> 
> Can we make tests for this?
> 
  The MIME type is only used internally within MediaPlayer so I couldn't think of a way to create a test.

> > WebCore/platform/graphics/MediaPlayer.cpp:306
> > +    // If the MIME type is missing or is not meaningful, try to figure it out from the url.
> > +    if (type.isEmpty() && protocolIs(url, "data"))
> 
> The comment says “is not meaningful”, but the code only checks for empty strings. Aren’t there other non-meaningful MIME types?
> 
  Good point, we try to infer the MIME type from the file extension when passed application/octetstream or text/plain, so we should look for type in a data url when passed those types as well.
Comment 4 Eric Carlson 2010-12-22 14:13:38 PST
http://trac.webkit.org/changeset/74504
Comment 5 Jessie Berlin 2010-12-22 15:28:39 PST
(In reply to comment #4)
> http://trac.webkit.org/changeset/74504

I think this change broke media/audio-data-url.html in the Windows 7 Release Tests:

http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r74504%20(7657)/media/audio-data-url-pretty-diff.html
Comment 6 WebKit Review Bot 2010-12-22 15:32:42 PST
http://trac.webkit.org/changeset/74504 might have broken GTK Linux 64-bit Debug
The following tests are not passing:
media/audio-data-url.html
Comment 7 Jessie Berlin 2010-12-22 20:02:49 PST
Bug for the failing Windows 7 Release Tests

https://bugs.webkit.org/show_bug.cgi?id=51518
Comment 8 Philippe Normand 2010-12-23 01:26:05 PST
(In reply to comment #6)
> http://trac.webkit.org/changeset/74504 might have broken GTK Linux 64-bit Debug
> The following tests are not passing:
> media/audio-data-url.html

https://bugs.webkit.org/show_bug.cgi?id=51525 ...