[webkit-reviews] review denied: [Bug 47907] Setting media element 'src' attribute to "" should trigger load : [Attachment 125797] proposed patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has denied Arun Patole
<arun.patole at motorola.com>'s request for review:
Bug 47907: Setting media element 'src' attribute to "" should trigger load
https://bugs.webkit.org/show_bug.cgi?id=47907

Attachment 125797: proposed patch
https://bugs.webkit.org/attachment.cgi?id=125797&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
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.