Bug 86308 - <video> won't load when URL ends with .php
Summary: <video> won't load when URL ends with .php
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-05-12 17:33 PDT by Eric Carlson
Modified: 2012-05-15 14:25 PDT (History)
6 users (show)

See Also:


Attachments
Proposed patch (6.98 KB, patch)
2012-05-12 18:06 PDT, Eric Carlson
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Updated patch - this time with not as many uninitialized variables. (7.09 KB, patch)
2012-05-12 23:04 PDT, 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 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