Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
419673007:
Adding more MSE tests. (Closed)
DescriptionAdding more MSE tests.
BUG=
R=wolenetz, acolwell
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179273
Patch Set 1 #Patch Set 2 : Fixing debug messages #
Total comments: 50
Patch Set 3 : Addressing CR comments #
Total comments: 24
Patch Set 4 : Fixing CR comments #Patch Set 5 : Fixing CR comments #Messages
Total messages: 9 (0 generated)
PTAL
Thanks for adding these! Mostly minor nits/style nits/test improvement suggestions: https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... File LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html (right): https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:353: test.expectEvent(sourceBuffer, "updateend", "partialInitSegment append ended."); eek! :) there's lots of " vs ' inconsistency in this CL and also in the previous code. Within js, prefer ' over ". https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:358: test.expectEvent(sourceBuffer, "updateend", "remainingInitSegment append ended."); Also, verify the init segment received algorithm has not yet executed by verifying the readyState (HAVE_NOTHING) and duration (NaN). https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:359: sourceBuffer.appendBuffer(remainingInitSegment); Also, expect loadedmetadata event on media element. https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:364: test.expectEvent(sourceBuffer, "updateend", "mediaSegment append ended."); Also, verify the init segment received algorithm HAS executed by verifying the readyState (HAVE_METADATA) and duration (+Infinity or the init segment's duration, depending on the test init segment appended..) https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:371: assert_equals(sourceBuffer.updating, false); Also verify HAVE_CURRENT_DATA readyState, though this could be flaky w.r.t. timing of the event deliveries and asynch progression of media playback. I don't think it will be flaky, though, since we haven't told the element to commence playback. https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:384: test.expectEvent(sourceBuffer, "updateend", "InitSegment append ended."); nit: also expect loadedmetadata? https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:389: test.expectEvent(sourceBuffer, "updateend", "partial media segment append ended."); verify HAVE_METADATA readyState and non-NaN duration, to convince ourselves that the init segment received algo has run. https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:395: test.expectEvent(sourceBuffer, "updateend", "mediaSegment append ended."); // Aside: we cannot assume HAVE_CURRENT_DATA at this point, because coded frame processing could have commenced on some platforms even prior to full media segment append. https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:402: assert_equals(sourceBuffer.updating, false); verify >= HAVE_CURRENT_DATA element readyState (but MS not in ENDED readyState). Also verify buffered ranges' length is 1. We have other tests that further verify the actual buffered range logic/values. https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:483: test.expectEvent(mediaSource.activeSourceBuffers, "removesourcebuffer", "SourceBuffer removed."); I think this line is actually testing non-compliant behavior, since activeSourceBuffers should still be empty in a compliant implementation (we've not actually appended an init segment yet to |sourceBuffer|, so it shouldn't really be in |mediaSource.activeSourceBuffers|.) I recommend s/active//, and add a separate test that appends an init segment prior to removeSourceBuffer, and that expects activeSourceBuffers to be given event 'removesourcebuffer' when the (actually active) sourceBuffer is removed. https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:493: nit: remove empty line. https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:517: test.failOnEvent(mediaElement, "error"); This is already done in mediasource_testafterdataloaded(), so is redundant here (and in all of these other mediasource_testafterdataloaded(...){ /*here*/ } contexts.) https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:527: assert_equals(sourceBuffer.appendWindowStart, 0, "appendWindowStart is reset to 0"); Improve this test by setting non-default appendwindow start/end values prior to the appendBuffer(), above. https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:529: "appendWindowEnd is reset to +INFINITY"); nit: indentation https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:532: }, "Test abort after appendBuffer."); nit: s/ffer/ffer update ends/ https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... File LayoutTests/http/tests/media/media-source/mediasource-appendwindow.html (right): https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-appendwindow.html:53: "set appendWindowStart throws an exception when equal to appendWindowEnd."); I see in our tracking spreadsheet that "set appendWindowEnd value equal to appendWindowStart" is "NewAdded" as of Jul 21, but I see no corresponding test. Note that there is distinct code executed by setting appendWindowStart vs setting appendWindowEnd attribute, so this specific assert_throws() doesn't exactly cover verification of both attribute's setter logic for this case. Please add another assert_throws() to really cover the "NewAdded" described above. https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... File LayoutTests/http/tests/media/media-source/mediasource-buffered.html (right): https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-buffered.html:197: }, "Get buffered range when only initsegment is appended."); nit: is there extra trailing whitespace here? if so, please remove. https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-buffered.html:201: test.expectEvent(mediaSource.activeSourceBuffers, "removesourcebuffer", "SourceBuffer removed."); ditto: sourceBuffer really shouldn't be in activeSourceBuffers until init segment received algo runs on a valid init segment that makes the sourceBuffer active. s/active// here and add another test that first appends an init segment and verifies removal from activeSourceBuffers. https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-buffered.html:209: nit: remove blank line. https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-buffered.html:212: nit: remove blank line. https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-buffered.html:213: }, "Get buffered range after removing sourcebuffer."); ditto: eek! " vs ' inconsistencies. prefer ' :) https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... File LayoutTests/http/tests/media/media-source/mediasource-closed.html (right): https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-closed.html:123: assert_equals(mediaSource.readyState, "open", "readyState is 'open'"); nit: eek! ditto (" vs ') https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-closed.html:131: "sourceBuffer.abort() throws INVALID_STATE_ERROR"); nit: remove any trailing whitespace here
PTAL... Fixing CR comments. Changed all occurrences of " to ' in the touched files to comply with the JS style guide (& matt's comment). https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... File LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html (right): https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:353: test.expectEvent(sourceBuffer, "updateend", "partialInitSegment append ended."); On 2014/07/26 00:07:14, wolenetz wrote: > eek! :) there's lots of " vs ' inconsistency in this CL and also in the previous > code. Within js, prefer ' over ". Done. https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:358: test.expectEvent(sourceBuffer, "updateend", "remainingInitSegment append ended."); On 2014/07/26 00:07:14, wolenetz wrote: > Also, verify the init segment received algorithm has not yet executed by > verifying the readyState (HAVE_NOTHING) and duration (NaN). Done. https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:359: sourceBuffer.appendBuffer(remainingInitSegment); On 2014/07/26 00:07:15, wolenetz wrote: > Also, expect loadedmetadata event on media element. Done. https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:364: test.expectEvent(sourceBuffer, "updateend", "mediaSegment append ended."); On 2014/07/26 00:07:14, wolenetz wrote: > Also, verify the init segment received algorithm HAS executed by verifying the > readyState (HAVE_METADATA) and duration (+Infinity or the init segment's > duration, depending on the test init segment appended..) Done. https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:371: assert_equals(sourceBuffer.updating, false); On 2014/07/26 00:07:15, wolenetz wrote: > Also verify HAVE_CURRENT_DATA readyState, though this could be flaky w.r.t. > timing of the event deliveries and asynch progression of media playback. I don't > think it will be flaky, though, since we haven't told the element to commence > playback. It appears to be responding with HAVE_ENOUGH_DATA instead. https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:371: assert_equals(sourceBuffer.updating, false); On 2014/07/26 00:07:15, wolenetz wrote: > Also verify HAVE_CURRENT_DATA readyState, though this could be flaky w.r.t. > timing of the event deliveries and asynch progression of media playback. I don't > think it will be flaky, though, since we haven't told the element to commence > playback. Done. https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:384: test.expectEvent(sourceBuffer, "updateend", "InitSegment append ended."); On 2014/07/26 00:07:14, wolenetz wrote: > nit: also expect loadedmetadata? Done. https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:389: test.expectEvent(sourceBuffer, "updateend", "partial media segment append ended."); On 2014/07/26 00:07:14, wolenetz wrote: > verify HAVE_METADATA readyState and non-NaN duration, to convince ourselves that > the init segment received algo has run. Done. https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:395: test.expectEvent(sourceBuffer, "updateend", "mediaSegment append ended."); On 2014/07/26 00:07:14, wolenetz wrote: > // Aside: we cannot assume HAVE_CURRENT_DATA at this point, because coded frame > processing could have commenced on some platforms even prior to full media > segment append. Done. https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:402: assert_equals(sourceBuffer.updating, false); On 2014/07/26 00:07:14, wolenetz wrote: > verify >= HAVE_CURRENT_DATA element readyState (but MS not in ENDED readyState). > Also verify buffered ranges' length is 1. We have other tests that further > verify the actual buffered range logic/values. Done. https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:483: test.expectEvent(mediaSource.activeSourceBuffers, "removesourcebuffer", "SourceBuffer removed."); On 2014/07/26 00:07:14, wolenetz wrote: > I think this line is actually testing non-compliant behavior, since > activeSourceBuffers should still be empty in a compliant implementation (we've > not actually appended an init segment yet to |sourceBuffer|, so it shouldn't > really be in |mediaSource.activeSourceBuffers|.) > I recommend s/active//, and add a separate test that appends an init segment > prior to removeSourceBuffer, and that expects activeSourceBuffers to be given > event 'removesourcebuffer' when the (actually active) sourceBuffer is removed. Done. https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:483: test.expectEvent(mediaSource.activeSourceBuffers, "removesourcebuffer", "SourceBuffer removed."); On 2014/07/26 00:07:14, wolenetz wrote: > I think this line is actually testing non-compliant behavior, since > activeSourceBuffers should still be empty in a compliant implementation (we've > not actually appended an init segment yet to |sourceBuffer|, so it shouldn't > really be in |mediaSource.activeSourceBuffers|.) > I recommend s/active//, and add a separate test that appends an init segment > prior to removeSourceBuffer, and that expects activeSourceBuffers to be given > event 'removesourcebuffer' when the (actually active) sourceBuffer is removed. New test added to mediasource-removesourcebuffer.html. https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:493: On 2014/07/26 00:07:14, wolenetz wrote: > nit: remove empty line. Done. https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:517: test.failOnEvent(mediaElement, "error"); On 2014/07/26 00:07:13, wolenetz wrote: > This is already done in mediasource_testafterdataloaded(), so is redundant here > (and in all of these other mediasource_testafterdataloaded(...){ /*here*/ } > contexts.) Done. https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:527: assert_equals(sourceBuffer.appendWindowStart, 0, "appendWindowStart is reset to 0"); On 2014/07/26 00:07:14, wolenetz wrote: > Improve this test by setting non-default appendwindow start/end values prior to > the appendBuffer(), above. Done. https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:529: "appendWindowEnd is reset to +INFINITY"); On 2014/07/26 00:07:15, wolenetz wrote: > nit: indentation Done. https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:532: }, "Test abort after appendBuffer."); On 2014/07/26 00:07:15, wolenetz wrote: > nit: s/ffer/ffer update ends/ Done. https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... File LayoutTests/http/tests/media/media-source/mediasource-appendwindow.html (right): https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-appendwindow.html:53: "set appendWindowStart throws an exception when equal to appendWindowEnd."); ah..got it! On 2014/07/26 00:07:15, wolenetz wrote: > I see in our tracking spreadsheet that "set appendWindowEnd value equal to > appendWindowStart" is "NewAdded" as of Jul 21, but I see no corresponding test. > Note that there is distinct code executed by setting appendWindowStart vs > setting appendWindowEnd attribute, so this specific assert_throws() doesn't > exactly cover verification of both attribute's setter logic for this case. > Please add another assert_throws() to really cover the "NewAdded" described > above. https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-appendwindow.html:53: "set appendWindowStart throws an exception when equal to appendWindowEnd."); On 2014/07/26 00:07:15, wolenetz wrote: > I see in our tracking spreadsheet that "set appendWindowEnd value equal to > appendWindowStart" is "NewAdded" as of Jul 21, but I see no corresponding test. > Note that there is distinct code executed by setting appendWindowStart vs > setting appendWindowEnd attribute, so this specific assert_throws() doesn't > exactly cover verification of both attribute's setter logic for this case. > Please add another assert_throws() to really cover the "NewAdded" described > above. Done. https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... File LayoutTests/http/tests/media/media-source/mediasource-buffered.html (right): https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-buffered.html:197: }, "Get buffered range when only initsegment is appended."); On 2014/07/26 00:07:15, wolenetz wrote: > nit: is there extra trailing whitespace here? if so, please remove. Done. https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-buffered.html:201: test.expectEvent(mediaSource.activeSourceBuffers, "removesourcebuffer", "SourceBuffer removed."); On 2014/07/26 00:07:15, wolenetz wrote: > ditto: sourceBuffer really shouldn't be in activeSourceBuffers until init > segment received algo runs on a valid init segment that makes the sourceBuffer > active. s/active// here and add another test that first appends an init segment > and verifies removal from activeSourceBuffers. Done. https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-buffered.html:209: On 2014/07/26 00:07:15, wolenetz wrote: > nit: remove blank line. Done. https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-buffered.html:212: On 2014/07/26 00:07:15, wolenetz wrote: > nit: remove blank line. Done. https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-buffered.html:213: }, "Get buffered range after removing sourcebuffer."); On 2014/07/26 00:07:15, wolenetz wrote: > ditto: eek! " vs ' inconsistencies. prefer ' :) Done. https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... File LayoutTests/http/tests/media/media-source/mediasource-closed.html (right): https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-closed.html:123: assert_equals(mediaSource.readyState, "open", "readyState is 'open'"); On 2014/07/26 00:07:15, wolenetz wrote: > nit: eek! ditto (" vs ') Done. https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-closed.html:131: "sourceBuffer.abort() throws INVALID_STATE_ERROR"); On 2014/07/26 00:07:16, wolenetz wrote: > nit: remove any trailing whitespace here Done.
Looking pretty good. Thanks for fixing many of those "->' in js. Mostly nits: https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... File LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html (right): https://codereview.chromium.org/419673007/diff/20001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:371: assert_equals(sourceBuffer.updating, false); On 2014/07/28 21:36:48, prabhur1 wrote: > On 2014/07/26 00:07:15, wolenetz wrote: > > Also verify HAVE_CURRENT_DATA readyState, though this could be flaky w.r.t. > > timing of the event deliveries and asynch progression of media playback. I > don't > > think it will be flaky, though, since we haven't told the element to commence > > playback. > > It appears to be responding with HAVE_ENOUGH_DATA instead. Acknowledged. https://codereview.chromium.org/419673007/diff/40001/LayoutTests/http/tests/m... File LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html (right): https://codereview.chromium.org/419673007/diff/40001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:270: nit: remove extra line https://codereview.chromium.org/419673007/diff/40001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:353: nit: remove extra horizontal whitespace in this line. https://codereview.chromium.org/419673007/diff/40001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:360: assert_equals(mediaSource.duration, Number.NaN);; nit: remove trailing extra ; https://codereview.chromium.org/419673007/diff/40001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:369: assert_equals(mediaSource.duration, segmentInfo.durationInInitSegment);; nit: remove trailing extra ; https://codereview.chromium.org/419673007/diff/40001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:379: assert_not_equals(mediaSource.readyState, 'ended'); nit: I'm not convinced this check needs to be this loose to add value. Assert that it equals 'open' instead? https://codereview.chromium.org/419673007/diff/40001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:407: test.expectEvent(mediaElement, 'loadeddata', 'loadeddata fired.'); loadeddata could occur earlier, so this looks like potentially flaky expectation. Also, we have related parsing media segment detection test (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). ISTM this new append-buffer tests adds value only w.r.t. checking mediaElement.readyState along the way. Is there some other value I'm missing or that would be good to add here? Merge this partial media segment test with that other set of tests? I don't have a strong feeling either way. *Not* merging can help regression root causing (that other test deals with timestampOffsets and appendModes, too.) https://codereview.chromium.org/419673007/diff/40001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:414: assert_not_equals(mediaSource.readyState, 'ended'); nit: ditto: assert 'open' instead? https://codereview.chromium.org/419673007/diff/40001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:525: }, 'Test abort after readyState is ended.'); Some of these tests, like all the "test abort *", are not specific to appendBuffer. They are equally valid if using appendStream. So I'm not convinced this file is where they should exist. Relocate in a later CL? (If you agree, please add this as P2 somewhere to the spreadsheet or file a P3 crbug.) Maybe we should consider adding stream variants of these tests to mediasource-append-stream.html. wdyt? https://codereview.chromium.org/419673007/diff/40001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:568: }, 'Test appending after removeSourceBuffer().'); nit: remove trailing whitespace https://codereview.chromium.org/419673007/diff/40001/LayoutTests/http/tests/m... File LayoutTests/http/tests/media/media-source/mediasource-buffered.html (right): https://codereview.chromium.org/419673007/diff/40001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-buffered.html:198: nit: remove extra horizontal whitespace on this line
PTAL https://codereview.chromium.org/419673007/diff/40001/LayoutTests/http/tests/m... File LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html (right): https://codereview.chromium.org/419673007/diff/40001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:270: On 2014/07/29 22:33:21, wolenetz wrote: > nit: remove extra line ah...I need to get better are spotting those extra lines/spaces! https://codereview.chromium.org/419673007/diff/40001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:270: On 2014/07/29 22:33:21, wolenetz wrote: > nit: remove extra line Done. https://codereview.chromium.org/419673007/diff/40001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:353: On 2014/07/29 22:33:22, wolenetz wrote: > nit: remove extra horizontal whitespace in this line. Done. https://codereview.chromium.org/419673007/diff/40001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:360: assert_equals(mediaSource.duration, Number.NaN);; On 2014/07/29 22:33:22, wolenetz wrote: > nit: remove trailing extra ; Done. https://codereview.chromium.org/419673007/diff/40001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:369: assert_equals(mediaSource.duration, segmentInfo.durationInInitSegment);; On 2014/07/29 22:33:22, wolenetz wrote: > nit: remove trailing extra ; Done. https://codereview.chromium.org/419673007/diff/40001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:379: assert_not_equals(mediaSource.readyState, 'ended'); On 2014/07/29 22:33:22, wolenetz wrote: > nit: I'm not convinced this check needs to be this loose to add value. Assert > that it equals 'open' instead? Done. https://codereview.chromium.org/419673007/diff/40001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:407: test.expectEvent(mediaElement, 'loadeddata', 'loadeddata fired.'); On 2014/07/29 22:33:22, wolenetz wrote: > loadeddata could occur earlier, so this looks like potentially flaky > expectation. > Also, we have related parsing media segment detection test > (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). > ISTM this new append-buffer tests adds value only w.r.t. checking > mediaElement.readyState along the way. Is there some other value I'm missing or > that would be good to add here? Merge this partial media segment test with that > other set of tests? I don't have a strong feeling either way. *Not* merging can > help regression root causing (that other test deals with timestampOffsets and > appendModes, too.) Done. https://codereview.chromium.org/419673007/diff/40001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:407: test.expectEvent(mediaElement, 'loadeddata', 'loadeddata fired.'); On 2014/07/29 22:33:22, wolenetz wrote: > loadeddata could occur earlier, so this looks like potentially flaky > expectation. > Also, we have related parsing media segment detection test > (https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...). > ISTM this new append-buffer tests adds value only w.r.t. checking > mediaElement.readyState along the way. Is there some other value I'm missing or > that would be good to add here? Merge this partial media segment test with that > other set of tests? I don't have a strong feeling either way. *Not* merging can > help regression root causing (that other test deals with timestampOffsets and > appendModes, too.) As per our discussion keeping this test as it is. opened bug w.r.t timing of the loadeddata event. https://codereview.chromium.org/419673007/diff/40001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:414: assert_not_equals(mediaSource.readyState, 'ended'); On 2014/07/29 22:33:22, wolenetz wrote: > nit: ditto: assert 'open' instead? Done. https://codereview.chromium.org/419673007/diff/40001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:525: }, 'Test abort after readyState is ended.'); On 2014/07/29 22:33:22, wolenetz wrote: > Some of these tests, like all the "test abort *", are not specific to > appendBuffer. They are equally valid if using appendStream. So I'm not convinced > this file is where they should exist. Relocate in a later CL? (If you agree, > please add this as P2 somewhere to the spreadsheet or file a P3 crbug.) Maybe we > should consider adding stream variants of these tests to > mediasource-append-stream.html. wdyt? Added a P2 to the spreadsheet. Yes, we need to add appendstream tests to the spreadsheet and track them. https://codereview.chromium.org/419673007/diff/40001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:525: }, 'Test abort after readyState is ended.'); On 2014/07/29 22:33:22, wolenetz wrote: > Some of these tests, like all the "test abort *", are not specific to > appendBuffer. They are equally valid if using appendStream. So I'm not convinced > this file is where they should exist. Relocate in a later CL? (If you agree, > please add this as P2 somewhere to the spreadsheet or file a P3 crbug.) Maybe we > should consider adding stream variants of these tests to > mediasource-append-stream.html. wdyt? Done. https://codereview.chromium.org/419673007/diff/40001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:568: }, 'Test appending after removeSourceBuffer().'); On 2014/07/29 22:33:22, wolenetz wrote: > nit: remove trailing whitespace Done. https://codereview.chromium.org/419673007/diff/40001/LayoutTests/http/tests/m... File LayoutTests/http/tests/media/media-source/mediasource-buffered.html (right): https://codereview.chromium.org/419673007/diff/40001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-buffered.html:198: On 2014/07/29 22:33:22, wolenetz wrote: > nit: remove extra horizontal whitespace on this line Done.
lgtm. Thanks for putting these together! https://codereview.chromium.org/419673007/diff/40001/LayoutTests/http/tests/m... File LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html (right): https://codereview.chromium.org/419673007/diff/40001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-append-buffer.html:270: On 2014/07/30 19:56:22, prabhur1 wrote: > On 2014/07/29 22:33:21, wolenetz wrote: > > nit: remove extra line > > ah...I need to get better are spotting those extra lines/spaces! I can share off-CR some tips for horizontal trailing whitespace highlighting, if you wish.
The CQ bit was checked by prabhur@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/prabhur@chromium.org/419673007/80001
Message was sent while issue was closed.
Change committed as 179273 |
