Bug 54888 - Add regression test for clean shutdown during video playback.
Summary: Add regression test for clean shutdown during video playback.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-21 09:34 PST by Ami Fischman
Modified: 2011-02-21 15:34 PST (History)
3 users (show)

See Also:


Attachments
Patch (1.91 KB, patch)
2011-02-21 09:35 PST, Ami Fischman
no flags Details | Formatted Diff | Diff
Patch (1.91 KB, patch)
2011-02-21 09:41 PST, Ami Fischman
no flags Details | Formatted Diff | Diff
Patch (2.16 KB, patch)
2011-02-21 12:42 PST, Ami Fischman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ami Fischman 2011-02-21 09:34:27 PST
Add regression test for clean shutdown during video playback.
Comment 1 Ami Fischman 2011-02-21 09:35:44 PST
Created attachment 83173 [details]
Patch
Comment 2 WebKit Review Bot 2011-02-21 09:37:46 PST
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.
Comment 3 Ami Fischman 2011-02-21 09:41:06 PST
Created attachment 83175 [details]
Patch

removed errant TAB from ChangeLog
Comment 4 Eric Carlson 2011-02-21 11:32:59 PST
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.
Comment 5 Ami Fischman 2011-02-21 12:42:21 PST
Created attachment 83203 [details]
Patch

CR comments addressed
Comment 6 Ami Fischman 2011-02-21 12:44:40 PST
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)
Comment 7 Eric Carlson 2011-02-21 13:37:02 PST
Comment on attachment 83203 [details]
Patch

Thanks!
Comment 8 WebKit Commit Bot 2011-02-21 15:34:17 PST
Comment on attachment 83203 [details]
Patch

Clearing flags on attachment: 83203

Committed r79257: <http://trac.webkit.org/changeset/79257>
Comment 9 WebKit Commit Bot 2011-02-21 15:34:23 PST
All reviewed patches have been landed.  Closing bug.