[webkit-reviews] review granted: [Bug 47907] Setting media element 'src' attribute to "" should trigger load : [Attachment 126013] updated patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted 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 126013: updated patch
https://bugs.webkit.org/attachment.cgi?id=126013&action=review

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