Bug 45364 – Fallback content should be rendered when an <object> doesn't specify a data, type or classid attribute.

• Component: New Bugs
• Status: RESOLVED FIXED

[Andy Estes]

Fallback content should be rendered when an <object> doesn't specify a data, type or classid attribute.

[Andy Estes]

<rdar://problem/8375816>

[Andy Estes]

See comment #2 on https://bugs.webkit.org/show_bug.cgi?id=44933 for the rationale behind this fix.

[Andy Estes]

Created attachment 66836 [details]
Patch

[WebKit Review Bot]

Attachment 66836 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
LayoutTests/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andy Estes]

Created attachment 66838 [details]
Patch

[Eric Carlson]

Comment on attachment 66836 [details]
Patch

> +        Fallback content should be rendered when an <object> doesn't specify a
> +        data, type or classid attribute.
> +        https://bugs.webkit.org/show_bug.cgi?id=45364
> +		<rdar://problem/8375816>
> +
> +        HTML5 says that if no data or type attribute is specified on the <object>,
> +        fallback content should be rendered. However, WebKit has traditionally
> +        supported specifying a URL and MIME type in a <param> element. 
> +
> +        To more closely match the spec while maintaining compatibility, continue
> +        to load resources specified in <param> elements under a more limited set
> +        of circumstances:
> +
> +        - MIME type is specified by a <param> named "type" or can be inferred
> +          from the extension of a URL specified in a param named "src", "movie",
> +          "code" or "url", and
> +        - resources of that MIME type will load in a plug-in.
> +
> +        If these two conditions aren't met, and the <object> element has no
> +        "data" or "type" attribute, fallback content will be rendered.
> +        Otherwise, there is no change in behavior.
> +
> +        * html/HTMLObjectElement.cpp:
> +        (WebCore::HTMLObjectElement::parametersForPlugin):
> +        (WebCore::HTMLObjectElement::updateWidget):
> +        * loader/SubframeLoader.cpp:
> +        (WebCore::SubframeLoader::resourceWillUsePlugin):
> +        * loader/SubframeLoader.h:
> +
The summary of what changed and why is great, but comments about what specifically changed in each function will be appreciated by someone that comes along later and tries to figure this out.


>          // FIXME: url adjustment does not belong in this function.
> -        if (url.isEmpty() && (equalIgnoringCase(name, "src") || equalIgnoringCase(name, "movie") || equalIgnoringCase(name, "code") || equalIgnoringCase(name, "url")))
> -            url = deprecatedParseURL(p->value());
> +        if (equalIgnoringCase(name, "src") || equalIgnoringCase(name, "movie") || equalIgnoringCase(name, "code") || equalIgnoringCase(name, "url"))
> +            urlParam = deprecatedParseURL(p->value());
>          // FIXME: serviceType calculation does not belong in this function.
>          if (serviceType.isEmpty() && equalIgnoringCase(name, "type")) {
>              serviceType = p->value();
1) "urlParam" won't be used later in this function unless "url" is empty so you can skip all of the string comparisons and url parsing if url is non-empty. 
2) The old code used the first non-empty url-equivalent param but this code uses the *last* non-empty param. Is this correct?


The ChangeLog says:

>    - MIME type is specified by a <param> named "type" or can be inferred
>      from the extension of a URL specified in a param named "src", "movie",
>      "code" or "url", and
Where is the type inferred from the url extension?

[Andy Estes]

(In reply to comment #6)
> (From update of attachment 66836 [details])
> > +        Fallback content should be rendered when an <object> doesn't specify a
> > +        data, type or classid attribute.
> > +        https://bugs.webkit.org/show_bug.cgi?id=45364
> > +		<rdar://problem/8375816>
> > +
> > +        HTML5 says that if no data or type attribute is specified on the <object>,
> > +        fallback content should be rendered. However, WebKit has traditionally
> > +        supported specifying a URL and MIME type in a <param> element. 
> > +
> > +        To more closely match the spec while maintaining compatibility, continue
> > +        to load resources specified in <param> elements under a more limited set
> > +        of circumstances:
> > +
> > +        - MIME type is specified by a <param> named "type" or can be inferred
> > +          from the extension of a URL specified in a param named "src", "movie",
> > +          "code" or "url", and
> > +        - resources of that MIME type will load in a plug-in.
> > +
> > +        If these two conditions aren't met, and the <object> element has no
> > +        "data" or "type" attribute, fallback content will be rendered.
> > +        Otherwise, there is no change in behavior.
> > +
> > +        * html/HTMLObjectElement.cpp:
> > +        (WebCore::HTMLObjectElement::parametersForPlugin):
> > +        (WebCore::HTMLObjectElement::updateWidget):
> > +        * loader/SubframeLoader.cpp:
> > +        (WebCore::SubframeLoader::resourceWillUsePlugin):
> > +        * loader/SubframeLoader.h:
> > +
> The summary of what changed and why is great, but comments about what specifically changed in each function will be appreciated by someone that comes along later and tries to figure this out.

I'll add these comments.

> 
> 
> >          // FIXME: url adjustment does not belong in this function.
> > -        if (url.isEmpty() && (equalIgnoringCase(name, "src") || equalIgnoringCase(name, "movie") || equalIgnoringCase(name, "code") || equalIgnoringCase(name, "url")))
> > -            url = deprecatedParseURL(p->value());
> > +        if (equalIgnoringCase(name, "src") || equalIgnoringCase(name, "movie") || equalIgnoringCase(name, "code") || equalIgnoringCase(name, "url"))
> > +            urlParam = deprecatedParseURL(p->value());
> >          // FIXME: serviceType calculation does not belong in this function.
> >          if (serviceType.isEmpty() && equalIgnoringCase(name, "type")) {
> >              serviceType = p->value();
> 1) "urlParam" won't be used later in this function unless "url" is empty so you can skip all of the string comparisons and url parsing if url is non-empty. 

Good point. I'll add back the url.isEmpty() clause.

> 2) The old code used the first non-empty url-equivalent param but this code uses the *last* non-empty param. Is this correct?

That was an unintended change. I don't think there is anything that says what we should do here, but it's probably best not to change our old behavior without reason. I'll fix this.

> 
> 
> The ChangeLog says:
> 
> >    - MIME type is specified by a <param> named "type" or can be inferred
> >      from the extension of a URL specified in a param named "src", "movie",
> >      "code" or "url", and
> Where is the type inferred from the url extension?

The Mac port does this in its implementation of FrameLoaderClient::objectContentType(), which gets eventually called by SubframeLoader::resourceWillUsePlugin(). I suppose I shouldn't assume that all ports do the same thing, though. Perhaps I should just rephrase the comment to say something more general like "if the MIME type can be determined from the information in the <param> elements."

Thanks for the feedback! I'll post a new patch shortly.

[Andy Estes]

Created attachment 66850 [details]
Patch

[Eric Carlson]

Comment on attachment 66850 [details]
Patch

> +    // HTML5 says that an object resource's URL is specified by the object's data
> +    // attribute, not by a param element. However, for compatibility, allow the
> +    // resource's URL to be given by a param named "src", "movie", "code" or "url"
> +    // if we know that resource points to a plug-in.
> +    SubframeLoader* loader = document()->frame()->loader()->subframeLoader();
> +    if (url.isEmpty() && loader->resourceWillUsePlugin(urlParam, serviceType))
> +        url = urlParam;
Minor nit, but you don't need the many dereferences to fetch "loader" unless url is empty.

r=me

[Andy Estes]

Committed r66992: <http://trac.webkit.org/changeset/66992>