[webkit-reviews] review denied: [Bug 175031] Old subtitle track is not deleted on 'src' attribute change event : [Attachment 317231] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has denied Kirill Ovchinnikov
<kirill.ovchinn at gmail.com>'s request for review:
Bug 175031: Old subtitle track is not deleted on 'src' attribute change event
https://bugs.webkit.org/show_bug.cgi?id=175031

Attachment 317231: Patch

https://bugs.webkit.org/attachment.cgi?id=317231&action=review




--- Comment #6 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 317231
  --> https://bugs.webkit.org/attachment.cgi?id=317231
Patch

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

Thank you for the patch! Marking r- for now because of the manual test.

> ManualTests/media-track-src-change.html:18
> +    <title>HTML5 Video Subtitle refresh bug test</title>
> +    <script>
> +	   var vtts =
["https://svn.webkit.org/repository/webkit/trunk/LayoutTests/media/track/captio
ns-webvtt/captions-long.vtt", 
> +		      
"https://svn.webkit.org/repository/webkit/trunk/LayoutTests/media/track/caption
s-webvtt/captions.vtt"];
> +	   var actualSub = 0;
> +	   document.addEventListener("DOMContentLoaded", function () {
> +	       document.getElementById('vid').play();
> +	       document.getElementById('nextSub').addEventListener('click',
function () {
> +		   actualSub++;
> +		   var subtitlesSrc = vtts[actualSub % vtts.length];
> +		  
document.getElementById('vid').children[0].setAttribute("src", subtitlesSrc);
> +	       }, false);
> +	   });

Manual tests are not run frequently, so they are generally used for changes
that can only be verified manually. This would be much better as a regular
layout test, so it will be run automatically every time someone submits a
patch.

Because this bug results in a track with both the old and new cues, a test can
check the number of cues before and after changing .src. Existing vtt tests are
in LayoutTests/media/track/. Something like track-text-track-cue-list.html
could be modified to test your change if you wish.