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.
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 :)
See also: bug 35088.
*** Bug 75440 has been marked as a duplicate of this bug. ***
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.
(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
Created attachment 125797 [details] proposed patch
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 on attachment 125797 [details] proposed patch looks like, we need to rebase video-empty-source.html for chromium ports.
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.
Created attachment 126013 [details] updated patch
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 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.
Created attachment 126282 [details] updated patch Updated patch... also skipped failing test on chromium as it needs a rebaseline.
Created attachment 126284 [details] updated patch
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.
> 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 on attachment 126284 [details] updated patch Clearing flags on attachment: 126284 Committed r107244: <http://trac.webkit.org/changeset/107244>
All reviewed patches have been landed. Closing bug.