Bug 86308 – <video> won't load when URL ends with .php

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

[Eric Carlson]

A media file that does not provide MIME a type in the markup, eg. does not use <source type=...>, will not load if the url has a non-media file extension for which MIMETypeRegistry::getMediaMIMETypeForExtension() returns a type. This is caused by the changes in http://trac.webkit.org/changeset/102800 in many cases because we now correctly extract the file extension from query urls.

[Eric Carlson]

<rdar://problem/10904486>

[Radar WebKit Bug Importer]

<rdar://problem/11441432>

[Eric Carlson]

Created attachment 141590 [details]
Proposed patch

[Early Warning System Bot]

Comment on attachment 141590 [details]
Proposed patch

Attachment 141590 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12679123

[Early Warning System Bot]

Comment on attachment 141590 [details]
Proposed patch

Attachment 141590 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12678503

[Build Bot]

Comment on attachment 141590 [details]
Proposed patch

Attachment 141590 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12666923

[Eric Carlson]

Created attachment 141600 [details]
Updated patch - this time with not as many uninitialized variables.

[Darin Adler]

Comment on attachment 141600 [details]
Updated patch - this time with not as many uninitialized variables.

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

> Source/WebCore/platform/graphics/MediaPlayer.cpp:328
> +    , m_typeInferredFromExtension(false)

This variable name sounds like it would be a type, not a boolean. m_typeWasInferredFromExtension should like a boolean. Or for additional clarity, m_contentMIMETypeWasInferredFromExtension.

> Source/WebCore/platform/graphics/MediaPlayer.cpp:354
> +    m_typeInferredFromExtension = false;

Seems strange that for m_url, m_contentMIMEType, and m_contentTypeCodecs we use local variables and set the data member only at the end, but for m_typeInferredFromExtension we set the data member directly. The code would be easier to read if all worked the same way.

> Source/WebCore/platform/graphics/MediaPlayer.cpp:369
>                  String mediaType = MIMETypeRegistry::getMediaMIMETypeForExtension(extension);
> -                if (!mediaType.isEmpty())
> +                if (!mediaType.isEmpty()) {
>                      type = mediaType;
> +                    m_typeInferredFromExtension = true;
> +                }

Seems like for clarity mediaType could be named inferredType.

[Eric Carlson]

(In reply to comment #8)
> (From update of attachment 141600 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141600&action=review
> 
> > Source/WebCore/platform/graphics/MediaPlayer.cpp:328
> > +    , m_typeInferredFromExtension(false)
> 
> This variable name sounds like it would be a type, not a boolean. m_typeWasInferredFromExtension should like a boolean. Or for additional clarity, m_contentMIMETypeWasInferredFromExtension.
> 

Done.

> > Source/WebCore/platform/graphics/MediaPlayer.cpp:354
> > +    m_typeInferredFromExtension = false;
> 
> Seems strange that for m_url, m_contentMIMEType, and m_contentTypeCodecs we use local variables and set the data member only at the end, but for m_typeInferredFromExtension we set the data member directly. The code would be easier to read if all worked the same way.
> 

Done.

> > Source/WebCore/platform/graphics/MediaPlayer.cpp:369
> >                  String mediaType = MIMETypeRegistry::getMediaMIMETypeForExtension(extension);
> > -                if (!mediaType.isEmpty())
> > +                if (!mediaType.isEmpty()) {
> >                      type = mediaType;
> > +                    m_typeInferredFromExtension = true;
> > +                }
> 
> Seems like for clarity mediaType could be named inferredType.

Excellent point, but I forgot to fix this before checking it in. I will clean this up the next time I modify this file.

[Eric Carlson]

http://trac.webkit.org/changeset/116960

[Brady Eidson]

(In reply to comment #10)
> http://trac.webkit.org/changeset/116960

The test checked in with this change fails on Lion.
http://build.webkit.org/results/Lion%20Debug%20(Tests)/r116960%20(6573)/http/tests/media/video-query-url-pretty-diff.html

https://bugs.webkit.org/show_bug.cgi?id=86527 tracks this.