Bug 86308

Summary: <video> won't load when URL ends with .php
Product: WebKit Reporter: Eric Carlson <eric.carlson>
Component: MediaAssignee: Eric Carlson <eric.carlson>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, beidson, dglazkov, feature-media-reviews, webkit-bug-importer, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=221695
Attachments:
Description Flags
Proposed patch
webkit-ews: commit-queue-
Updated patch - this time with not as many uninitialized variables. darin: review+

Description Eric Carlson 2012-05-12 17:33:16 PDT
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.
Comment 1 Eric Carlson 2012-05-12 17:33:50 PDT
<rdar://problem/10904486>
Comment 2 Radar WebKit Bug Importer 2012-05-12 17:34:08 PDT
<rdar://problem/11441432>
Comment 3 Eric Carlson 2012-05-12 18:06:00 PDT
Created attachment 141590 [details]
Proposed patch
Comment 4 Early Warning System Bot 2012-05-12 18:10:45 PDT
Comment on attachment 141590 [details]
Proposed patch

Attachment 141590 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12679123
Comment 5 Early Warning System Bot 2012-05-12 18:10:58 PDT
Comment on attachment 141590 [details]
Proposed patch

Attachment 141590 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12678503
Comment 6 Build Bot 2012-05-12 18:27:26 PDT
Comment on attachment 141590 [details]
Proposed patch

Attachment 141590 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12666923
Comment 7 Eric Carlson 2012-05-12 23:04:39 PDT
Created attachment 141600 [details]
Updated patch - this time with not as many uninitialized variables.
Comment 8 Darin Adler 2012-05-13 16:19:26 PDT
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.
Comment 9 Eric Carlson 2012-05-14 10:25:07 PDT
(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.
Comment 10 Eric Carlson 2012-05-14 10:25:20 PDT
http://trac.webkit.org/changeset/116960