Bug 42001 – Update media element's handling of empty 'src' attribute

• Component: Media
• Status: RESOLVED FIXED

[Eric Carlson]

The HTML5 spec previously said the resolve all non-null media urls, now empty urls are not resolved.

[Eric Carlson]

Created attachment 61122 [details]
proposed patch

[WebKit Review Bot]

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

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/html/HTMLMediaElement.cpp:1488:  check_again is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 14 files


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

[Darin Adler]

Comment on attachment 61122 [details]
proposed patch

Don’t we want this for all sorts of URL attributes? I seem to recall discussion of that on one of the HTML5 mailing lists. I don’t want to go through all the classes removing the ReflectURL calls one by one if we can just change the rules about reflected URLs. It's not even out of the question to have more than one flavor of ReflectURL. I hate to hand-write the code just because of this issue. I can help you make a version of ReflectURL that treats an empty attribute the same way it treats a missing one.

Long term we might even want to change the behavior of completeURL itself. I don’t think there are very may contexts where we want to treat the empty string as meaning "expand this to be the base URL".

I also noticed that HTMLMediaElement::src and HTMLSourceElement::src don't strip whitespace before completing the URL the way other similar attribute getters do. That may be a bug.

Functions like HTMLVideoElement::setPoster should take const AtomicString& not const String&.

[Eric Carlson]

Comment on attachment 61122 [details]
proposed patch

Clearing the review flag for now. I will resubmit an updated patch after take Darin's suggestion and make some changes to ReflectURL.

[Darin Adler]

I’m going to improve the URL attribute reflection in a patch I’ll attach to another bug, and then you can adopt the improvements in this bug.

[Darin Adler]

(In reply to comment #3)
> (From update of attachment 61122 [details])
> I also noticed that HTMLMediaElement::src and HTMLSourceElement::src don't strip whitespace before completing the URL the way other similar attribute getters do. That may be a bug.

The specification definitely calls for this stripping. Sadly, our code uses deprecatedParseURL which does unwanted additional processing, not just stripping. I think we should make our code use the getURLAttribute functions and then we can fix the deprecatedParseURL thing in one place. It’s not good to have code doing the completeURL without the stripping.

[Eric Carlson]

Created attachment 61247 [details]
proposed patch

[Darin Adler]

Comment on attachment 61247 [details]
proposed patch

> +    if (!hasAttribute(srcAttr)) {

I'm surprised the above logic is right. Why should a missing src attribute be treated differently from one that is empty or contains only spaces?

> +        // If candidate does not have a src attribute, or if its src attribute's value is the empty string ... jump down to the failed step below
> +        if (!source->hasAttribute(srcAttr))
> +            goto check_again;
> +        
> +        mediaURL = source->getNonEmptyURLAttribute(srcAttr);
> +        if (mediaURL.isEmpty())
> +            goto check_again;

I think you could just leave out the hasAttribute check here. It's just an optimization for the case where there's no src attribute at all. And I don't think that case needs to be optimized.

r=me

[Eric Carlson]

(In reply to comment #8)
> (From update of attachment 61247 [details])
> > +    if (!hasAttribute(srcAttr)) {
> 
> I'm surprised the above logic is right. Why should a missing src attribute be treated differently from one that is empty or contains only spaces?
> 
The difference is that an empty (or bogus) src attribute generates an 'error' event, but an element with no src attribute and no source element children does not.


> > +        // If candidate does not have a src attribute, or if its src attribute's value is the empty string ... jump down to the failed step below
> > +        if (!source->hasAttribute(srcAttr))
> > +            goto check_again;
> > +        
> > +        mediaURL = source->getNonEmptyURLAttribute(srcAttr);
> > +        if (mediaURL.isEmpty())
> > +            goto check_again;
> 
> I think you could just leave out the hasAttribute check here. It's just an optimization for the case where there's no src attribute at all. And I don't think that case needs to be optimized.
> 
Good point, done.

> r=me
Thanks!

[Eric Carlson]

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