Summary: | Add regression test for clean shutdown during video playback. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ami Fischman <fischman> | ||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, eric.carlson, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Other | ||||||||||
OS: | OS X 10.5 | ||||||||||
Attachments: |
|
Description
Ami Fischman
2011-02-21 09:34:27 PST
Created attachment 83173 [details]
Patch
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.
Created attachment 83175 [details]
Patch
removed errant TAB from ChangeLog
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. Created attachment 83203 [details]
Patch
CR comments addressed
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 on attachment 83203 [details]
Patch
Thanks!
Comment on attachment 83203 [details] Patch Clearing flags on attachment: 83203 Committed r79257: <http://trac.webkit.org/changeset/79257> All reviewed patches have been landed. Closing bug. |