Bug 47907 - Setting media element 'src' attribute to "" should trigger load
Summary: Setting media element 'src' attribute to "" should trigger load
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords:
: 75440 (view as bug list)
Depends on:
Blocks: 78233
  Show dependency treegraph
 
Reported: 2010-10-19 08:42 PDT by Eric Carlson
Modified: 2012-02-09 07:49 PST (History)
5 users (show)

See Also:


Attachments
proposed patch (8.72 KB, patch)
2012-02-07 02:46 PST, Arun Patole
eric.carlson: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
updated patch (9.15 KB, patch)
2012-02-07 23:10 PST, Arun Patole
eric.carlson: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
updated patch (9.74 KB, patch)
2012-02-09 04:57 PST, Arun Patole
no flags Details | Formatted Diff | Diff
updated patch (9.68 KB, patch)
2012-02-09 05:03 PST, Arun Patole
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2010-10-19 08:42:27 PDT
The spec currently says:

If a src attribute of a media element is set or changed, the user agent must invoke the media element's media element load algorithm. (Removing the src attribute does not do this, even if there are source elements present.)

but HTMLMediaElement::attributeChanged only triggers a load if the src attribute is non-empty:

    if (attrName == srcAttr) {
        // Trigger a reload, as long as the 'src' attribute is present.
        if (!getAttribute(srcAttr).isEmpty())
            scheduleLoad();
    }

The comment  is correct but the code is wrong.
Comment 1 Andrew Scherkus 2010-11-04 17:02:33 PDT
Other tips when discussing this with hixie:
"""
Regarding setting .src to "": that is indeed non-conforming (it technically means "load the current HTML page as a video" — the empty string is a relative URL to the current page), but you can just remove the "src" attribute altogether and call load(), which is legal.
"""

Regardless we still need to kick off a load, even if it attempts to load the current HTML page :)
Comment 2 Alexey Proskuryakov 2012-01-31 12:40:11 PST
See also: bug 35088.
Comment 3 Alexey Proskuryakov 2012-01-31 12:41:02 PST
*** Bug 75440 has been marked as a duplicate of this bug. ***
Comment 4 Arun Patole 2012-01-31 22:04:33 PST
Currently webkit allows media element's src attribute to be set to "" but doesn't invoke the media element's media element load algorithm.

But the spec says:
"The attribute, if present, must contain a valid non-empty URL potentially surrounded by spaces."
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#location-of-the-media-resource

so, should we really allow src to be set to null or invalid value??

One option is to just remove the src attribute if attempt is made to set it to null or invalid url. And as per the spec, removing it doesn't invoke load algo and current behaviour of playing older media resource looks fine. This option looks good as it is also closer to the spec ("The attribute, if present, must be nonempty/valid URL")

Other option is, like Firefox just let it be set to null or invalid and then load algo when invoked, aborts current play and throws error as src is invalid. But this is not what current spec says.
Comment 5 Eric Carlson 2012-02-01 12:05:34 PST
(In reply to comment #4)
> But the spec says:
> "The attribute, if present, must contain a valid non-empty URL potentially surrounded by spaces."
> http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#location-of-the-media-resource
> 
This is an authoring requirement.

> so, should we really allow src to be set to null or invalid value??
> 
Yes. The spec says [1]:

If a src attribute of a media element is set or changed, the user agent must invoke the media element's media element load algorithm. (Removing the src attribute does not do this, even if there are source elements present.)

and step 6.1 of the resource selection algorithm says:

Process candidate: If the src attribute's value is the empty string, then end the synchronous section, and jump down to the failed step below.

> Other option is, like Firefox just let it be set to null or invalid and then load algo when invoked, aborts current play and throws error as src is invalid. But this is not what current spec says.

Firefox is correct.

[1] http://dev.w3.org/html5/spec/media-elements.html#location-of-the-media-resource
Comment 6 Arun Patole 2012-02-07 02:46:45 PST
Created attachment 125797 [details]
proposed patch
Comment 7 WebKit Review Bot 2012-02-07 03:27:51 PST
Comment on attachment 125797 [details]
proposed patch

Attachment 125797 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11442257

New failing tests:
media/video-empty-source.html
Comment 8 Arun Patole 2012-02-07 04:13:21 PST
Comment on attachment 125797 [details]
proposed patch

looks like, we need to rebase video-empty-source.html for chromium ports.
Comment 9 Eric Carlson 2012-02-07 09:20:36 PST
Comment on attachment 125797 [details]
proposed patch

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

Marking r- because I think the new test should set src to "" after another video has loaded.

> LayoutTests/media/video-src-empty.html:13
> +            function start()
> +            {
> +                videos = document.querySelectorAll('video');
> +                run('videos[0].src = ""');
> +                testLoad();
> +            }

I think this would be much more useful test if it set src to "" after the <video> element has already loaded another file so we test to ensure that it unloads the other file and fires an error.

> LayoutTests/media/video-src-empty.html:17
> +                if(state == "load() with empty 'src'") {

In this test an 'error' event should only fire once the src has been set to "".

> LayoutTests/media/video-src-empty.html:30
> +                state = "load() with empty 'src'";

Setting "state" to a string sort of made sense in video-src-none.html where two <video> elements were being tested, but you might as well just use a bool here.

> LayoutTests/media/video-src-none.html:33
> +                state = "load() with missing 'src'";

As above, you may as well update this to use a bool instead of a string now that only one element is being tested.
Comment 10 Arun Patole 2012-02-07 23:10:28 PST
Created attachment 126013 [details]
updated patch
Comment 11 WebKit Review Bot 2012-02-07 23:56:03 PST
Comment on attachment 126013 [details]
updated patch

Attachment 126013 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11449299

New failing tests:
media/video-empty-source.html
Comment 12 Eric Carlson 2012-02-08 10:21:47 PST
Comment on attachment 126013 [details]
updated patch

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

Marking r+, but I would prefer to have the minor issues noted cleaned up before the patch is landed.

> LayoutTests/media/video-src-empty.html:21
> +                run('video.load()');

Calling load() is unnecessary, setting src trigger a load implicitly.

> LayoutTests/media/video-src-empty.html:27
> +                testExpected(event.target, video);

Nit: "testExpected('event.target', video)" will generate "EXPECTED (event.target == '[object HTMLVideoElement]')", which IMO is slightly more readable.

> LayoutTests/media/video-src-empty.html:37
> +                run('video.load()');

load() is unnecessary.

> LayoutTests/media/video-src-none.html:19
> +                failTest("<br><i>Calling load() with no 'src' should NOT fire 'error' event<" + "/i>");              

This will cause the test to fail, but the message it logs is confusing because it is exactly the same as that already logged from the "test()" function. I would rather see another message here.
Comment 13 Arun Patole 2012-02-09 04:57:13 PST
Created attachment 126282 [details]
updated patch

Updated patch... also skipped failing test on chromium as it needs a rebaseline.
Comment 14 Arun Patole 2012-02-09 05:03:52 PST
Created attachment 126284 [details]
updated patch
Comment 15 Eric Carlson 2012-02-09 06:36:22 PST
Comment on attachment 126284 [details]
updated patch

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

> LayoutTests/platform/chromium/test_expectations.txt:4122
> +BUGWK47907 : media/video-empty-source.html = IMAGE+TEXT

Please either get someone with a cr build to land new results or file a new bug asking for a rebaseline.
Comment 16 Arun Patole 2012-02-09 06:47:33 PST
> Please either get someone with a cr build to land new results or file a new bug asking for a rebaseline.
Done:https://bugs.webkit.org/show_bug.cgi?id=78233 , Thanks!
Comment 17 WebKit Review Bot 2012-02-09 07:49:30 PST
Comment on attachment 126284 [details]
updated patch

Clearing flags on attachment: 126284

Committed r107244: <http://trac.webkit.org/changeset/107244>
Comment 18 WebKit Review Bot 2012-02-09 07:49:37 PST
All reviewed patches have been landed.  Closing bug.