Bug 54888 – Add regression test for clean shutdown during video playback.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ami Fischman]

Add regression test for clean shutdown during video playback.

[Ami Fischman]

Created attachment 83173 [details]
Patch

[WebKit Review Bot]

Attachment 83173 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/medi..." exit_code: 1

LayoutTests/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ami Fischman]

Created attachment 83175 [details]
Patch

removed errant TAB from ChangeLog

[Eric Carlson]

Comment on attachment 83175 [details]
Patch

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

Marking r- for the minor changes noted.

> LayoutTests/ChangeLog:6
> +        Add regression test for clean shutdown during video playback.
> +        This didn't always use to be the case (http://crbug.com/72730).

It would be good to have more information about why this test is necessary, I didn't understand what "clean shutdown during video playback" meant until I read the chromium bug.

> LayoutTests/media/video-plays-past-end-of-test.html:3
> +<!-- This test intentionally lets the video keep playing past endTest() to
> +     ensure that shutdown is clean. -->
> +

I really like to have information about what a test does in the markup so someone opening the test manually can figure out what is happening without having to view source. As above, "clean shutdown during video playback" didn't explain it for me.

[Ami Fischman]

Created attachment 83203 [details]
Patch

CR comments addressed

[Ami Fischman]

Comment on attachment 83175 [details]
Patch

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

>> LayoutTests/ChangeLog:6
>> +        This didn't always use to be the case (http://crbug.com/72730).
> 
> It would be good to have more information about why this test is necessary, I didn't understand what "clean shutdown during video playback" meant until I read the chromium bug.

Expanded changelog & in-test text.

>> LayoutTests/media/video-plays-past-end-of-test.html:3
>> +
> 
> I really like to have information about what a test does in the markup so someone opening the test manually can figure out what is happening without having to view source. As above, "clean shutdown during video playback" didn't explain it for me.

Done.
(I used an HTML comment instead b/c in non-dumpAsText tests, this kind of explanatory text is often the sole reason for the need for per-platform expectations, as antialiasing/hinting cause minute differences; I realize now I should take it up with a larger group than just my local team if I want a change to SOP in this area; but anyway it's irrelevant to this test b/c it's a dumpAsText test, anyway)

[Eric Carlson]

Comment on attachment 83203 [details]
Patch

Thanks!

[WebKit Commit Bot]

Comment on attachment 83203 [details]
Patch

Clearing flags on attachment: 83203

Committed r79257: <http://trac.webkit.org/changeset/79257>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.