Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
20114005:
Layout Test for basic MSE seek scenario. (Closed)
|
Created:
7 years, 5 months ago by anandc Modified:
7 years, 4 months ago CC:
blink-reviews, dglazkov, eae, yihongg1 Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionLayout Test for basic MSE seek scenario.
BUG=None
NOTRY=true
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=155643
Patch Set 1 #
Total comments: 16
Patch Set 2 : Explicitly wait 2 seconds, for play to commence. #
Total comments: 7
Patch Set 3 : Fix call to function specified for setTimeout. #
Total comments: 5
Patch Set 4 : Reduce test flakiness. #
Total comments: 6
Patch Set 5 : Define timeout handler variables, and call clearTimeout for first timeout handler. #
Total comments: 10
Patch Set 6 : Add timeout callbacks to expectations-manager. Clear timeouts in test.done() # #
Total comments: 6
Patch Set 7 : Check for completion of all expected delayed callbacks. #
Total comments: 2
Patch Set 8 : Properly track and remove timeoutIDs for each delayed callback. #
Total comments: 4
Patch Set 9 : Fix bug with using array indices to manage timeout IDs. #
Total comments: 4
Patch Set 10 : Fix silly bug in looping through map. #
Total comments: 12
Patch Set 11 : Confirm that seeked and timeupdate actually fired, before checking play. #
Total comments: 15
Patch Set 12 : Style fixes. #
Total comments: 2
Patch Set 13 : Fix indentation. #
Total comments: 16
Patch Set 14 : Code cleanup and reorganisation. #
Total comments: 8
Patch Set 15 : Wait for timeupdate and check for time-change, rather than waiting for a hard-coded change. #
Total comments: 2
Patch Set 16 : Remove unused function, cleanup setTimeout and removeEventListener calls. #
Total comments: 7
Patch Set 17 : Add only 1 listener to timeupdate. Use step_func. #
Total comments: 18
Patch Set 18 : Add notes describing functionality of waitForTimeUpdate. #
Total comments: 3
Patch Set 19 : Code cleanup and reorganisation, round deux. #
Total comments: 2
Patch Set 20 : Remove unused function. #
Messages
Total messages: 53 (0 generated)
Matt/Aaron, PTAL. This is a pretty basic seek scenario: load all media segments, kick off play, make sure media is playing, wait for currentTime to be non-zero, then seek back to 0. Once this gets to LGTM, there are more seek scenarios that we plan to implement. Depending on how those get laid out, I might add more tests to this file. To cut a long story short (too late :-)), there might be some reorganization of this and related code to come later. Thanks.
I'm curious: did PS1 pass on desktop? https://codereview.chromium.org/20114005/diff/1/LayoutTests/http/tests/media/... File LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html (right): https://codereview.chromium.org/20114005/diff/1/LayoutTests/http/tests/media/... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:35: test.expectEvent(mediaElement, "playing", "Playing triggered"); style nit: prefer ' over " in js (multiple places in this file) https://codereview.chromium.org/20114005/diff/1/LayoutTests/http/tests/media/... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:41: if (mediaElement.currentTime > 0.5 ) { style nit: 4-space indent (multiple places in this file) https://codereview.chromium.org/20114005/diff/1/LayoutTests/http/tests/media/... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:42: assert_greater_than(mediaElement.currentTime, 0.1); Why are we asserting within a conditional that already guarantees this (unless maybe currentTime is decreasing between the two queries??) https://codereview.chromium.org/20114005/diff/1/LayoutTests/http/tests/media/... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:46: test.expectEvent(mediaElement, "seeked", "mediaElement finished seek"); You'll need a 'timeupdate' expectation between 'seeking' and 'seeked', and it would be good to confirm on receipt of this specific 'timeupdate' that seeking is true and currentTime is the expected seek target (0.0). Note also that some implementations may already have expired already played data in buffers, and may also issue 'waiting' between 'seeking' and 'timeupdate' and also 'playing', though I'm not exactly clear in this case if 'playing' would precede 'timeupdate' or 'seeked'. https://codereview.chromium.org/20114005/diff/1/LayoutTests/http/tests/media/... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:47: test.expectEvent(mediaElement, "playing", "Playing triggered"); I don't think 'playing' is expected here, unless there was some intervening 'waiting' for data to be (re-)appended for the media time 0. Even then, 'playing' might be required to dispatch prior to the timeupdate or seeked events (see previous comment). https://codereview.chromium.org/20114005/diff/1/LayoutTests/http/tests/media/... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:48: test.expectEvent(mediaElement, "timeupdate", "timeupdate fired."); Note for future: you'll actually get lots and lots of these (every 250ms during normal playback progress). Expecting just one here should be OK though. https://codereview.chromium.org/20114005/diff/1/LayoutTests/http/tests/media/... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:53: test.waitForExpectedEvents(function() It's highly likely that the previous currentTime > 0.5 check will fail. Perhaps you want to setTimeout() to give playback some time after "playing" event received? Otherwise, it's likely new expectations for events won't be added, and the seek back to 0 will not have been started at this point. https://codereview.chromium.org/20114005/diff/1/LayoutTests/http/tests/media/... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:56: mediaElement.HAVE_FUTURE_DATA, "Greater than HAVE_FUTURE_DATA"); I'm not sure all implementations can guarantee they estimate they can play through to completion at this point. This assertion may introduce flakiness.
(Dropping larger aliases). Matt/Aaron, thanks for the comments so far. I'm having trouble getting the test media to play. Following Matt's feedback, I'm using setTimeout to wait 2 seconds in the expectation that media would start playing in that time. However, that seems to not be happening, and the test fails with: "assert_greater_than: Playback has started. expected a number greater than 0 but got 0" Maybe addBuffer here isn't doing all that I think it is: sourceBuffer.appendBuffer(mediaData); ? Any suggestions? Thank you. https://codereview.chromium.org/20114005/diff/1/LayoutTests/http/tests/media/... File LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html (right): https://codereview.chromium.org/20114005/diff/1/LayoutTests/http/tests/media/... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:35: test.expectEvent(mediaElement, "playing", "Playing triggered"); On 2013/07/24 23:16:43, wolenetz wrote: > style nit: prefer ' over " in js (multiple places in this file) Done. https://codereview.chromium.org/20114005/diff/1/LayoutTests/http/tests/media/... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:41: if (mediaElement.currentTime > 0.5 ) { On 2013/07/24 23:16:43, wolenetz wrote: > style nit: 4-space indent (multiple places in this file) Done. https://codereview.chromium.org/20114005/diff/1/LayoutTests/http/tests/media/... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:42: assert_greater_than(mediaElement.currentTime, 0.1); On 2013/07/24 23:16:43, wolenetz wrote: > Why are we asserting within a conditional that already guarantees this (unless > maybe currentTime is decreasing between the two queries??) Yes, that was pretty stupid. :-) Gone. https://codereview.chromium.org/20114005/diff/1/LayoutTests/http/tests/media/... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:46: test.expectEvent(mediaElement, "seeked", "mediaElement finished seek"); On 2013/07/24 23:16:43, wolenetz wrote: > You'll need a 'timeupdate' expectation between 'seeking' and 'seeked', and it > would be good to confirm on receipt of this specific 'timeupdate' that seeking > is true and currentTime is the expected seek target (0.0). > Note also that some implementations may already have expired already played data > in buffers, and may also issue 'waiting' between 'seeking' and 'timeupdate' and > also 'playing', though I'm not exactly clear in this case if 'playing' would > precede 'timeupdate' or 'seeked'. Based on all your comments, adding an explicit wait with setTimeout now, to let media play before seeking back.
Noting the actual line of failure, in case it helps. Thanks. https://codereview.chromium.org/20114005/diff/5001/LayoutTests/http/tests/med... File LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html (right): https://codereview.chromium.org/20114005/diff/5001/LayoutTests/http/tests/med... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:53: 'Playback has started.'); This is the assert that is failing.
https://codereview.chromium.org/20114005/diff/5001/LayoutTests/http/tests/med... File LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html (right): https://codereview.chromium.org/20114005/diff/5001/LayoutTests/http/tests/med... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:40: test.failOnEvent(mediaElement, 'error'); nit: You shouldn't need this here since you have this in the code above. https://codereview.chromium.org/20114005/diff/5001/LayoutTests/http/tests/med... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:52: assert_greater_than(mediaElement.currentTime, 0.0, nit: put this all one one line. https://codereview.chromium.org/20114005/diff/5001/LayoutTests/http/tests/med... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:74: setTimeout(playAndSeek(test, mediaElement), 2000); this calls playAndSeek() immediately. It isn't waiting 2 seconds before calling. I think you are intending to do something along these lines. setTimeout(function() { playAndSeek(test, mediaElement); }, 2000);
https://codereview.chromium.org/20114005/diff/1/LayoutTests/http/tests/media/... File LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html (right): https://codereview.chromium.org/20114005/diff/1/LayoutTests/http/tests/media/... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:47: test.expectEvent(mediaElement, "playing", "Playing triggered"); On 2013/07/24 23:16:43, wolenetz wrote: > I don't think 'playing' is expected here, unless there was some intervening > 'waiting' for data to be (re-)appended for the media time 0. Even then, > 'playing' might be required to dispatch prior to the timeupdate or seeked events > (see previous comment). Correct, rearranged the flow now. https://codereview.chromium.org/20114005/diff/1/LayoutTests/http/tests/media/... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:48: test.expectEvent(mediaElement, "timeupdate", "timeupdate fired."); On 2013/07/24 23:16:43, wolenetz wrote: > Note for future: you'll actually get lots and lots of these (every 250ms during > normal playback progress). Expecting just one here should be OK though. ok https://codereview.chromium.org/20114005/diff/1/LayoutTests/http/tests/media/... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:53: test.waitForExpectedEvents(function() On 2013/07/24 23:16:43, wolenetz wrote: > It's highly likely that the previous currentTime > 0.5 check will fail. Perhaps > you want to setTimeout() to give playback some time after "playing" event > received? Otherwise, it's likely new expectations for events won't be added, and > the seek back to 0 will not have been started at this point. Done. https://codereview.chromium.org/20114005/diff/1/LayoutTests/http/tests/media/... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:56: mediaElement.HAVE_FUTURE_DATA, "Greater than HAVE_FUTURE_DATA"); On 2013/07/24 23:16:43, wolenetz wrote: > I'm not sure all implementations can guarantee they estimate they can play > through to completion at this point. This assertion may introduce flakiness. Removed. https://codereview.chromium.org/20114005/diff/5001/LayoutTests/http/tests/med... File LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html (right): https://codereview.chromium.org/20114005/diff/5001/LayoutTests/http/tests/med... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:40: test.failOnEvent(mediaElement, 'error'); On 2013/07/25 23:11:31, acolwell wrote: > nit: You shouldn't need this here since you have this in the code above. Done. https://codereview.chromium.org/20114005/diff/5001/LayoutTests/http/tests/med... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:52: assert_greater_than(mediaElement.currentTime, 0.0, On 2013/07/25 23:11:31, acolwell wrote: > nit: put this all one one line. Done. https://codereview.chromium.org/20114005/diff/5001/LayoutTests/http/tests/med... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:74: setTimeout(playAndSeek(test, mediaElement), 2000); On 2013/07/25 23:11:31, acolwell wrote: > this calls playAndSeek() immediately. It isn't waiting 2 seconds before calling. > I think you are intending to do something along these lines. > > setTimeout(function() { playAndSeek(test, mediaElement); }, 2000); Thanks. Done.
(Got sent too soon) Thanks for all the comments/tips, Aaron + Matt. PTAL. How has our experience been with having setTimeout calls in Layout Tests? Do they tend to add flakiness/random test execution timeouts? I've tried to keep the wait times reasonably low, but not sure what the right values should be to ensure that the test would pass reliably on all platforms. On Fri, Jul 26, 2013 at 11:49 AM, <anandc@chromium.org> wrote: > > https://codereview.chromium.**org/20114005/diff/1/** > LayoutTests/http/tests/media/**media-source/mediasource-play-** > then-seek-back.html<https://codereview.chromium.org/20114005/diff/1/LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html> > File > LayoutTests/http/tests/media/**media-source/mediasource-play-** > then-seek-back.html > (right): > > https://codereview.chromium.**org/20114005/diff/1/** > LayoutTests/http/tests/media/**media-source/mediasource-play-** > then-seek-back.html#newcode47<https://codereview.chromium.org/20114005/diff/1/LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html#newcode47> > LayoutTests/http/tests/media/**media-source/mediasource-play-** > then-seek-back.html:47: > test.expectEvent(mediaElement, "playing", "Playing triggered"); > On 2013/07/24 23:16:43, wolenetz wrote: > >> I don't think 'playing' is expected here, unless there was some >> > intervening > >> 'waiting' for data to be (re-)appended for the media time 0. Even >> > then, > >> 'playing' might be required to dispatch prior to the timeupdate or >> > seeked events > >> (see previous comment). >> > > Correct, rearranged the flow now. > > > https://codereview.chromium.**org/20114005/diff/1/** > LayoutTests/http/tests/media/**media-source/mediasource-play-** > then-seek-back.html#newcode48<https://codereview.chromium.org/20114005/diff/1/LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html#newcode48> > LayoutTests/http/tests/media/**media-source/mediasource-play-** > then-seek-back.html:48: > test.expectEvent(mediaElement, "timeupdate", "timeupdate fired."); > On 2013/07/24 23:16:43, wolenetz wrote: > >> Note for future: you'll actually get lots and lots of these (every >> > 250ms during > >> normal playback progress). Expecting just one here should be OK >> > though. > > ok > > > https://codereview.chromium.**org/20114005/diff/1/** > LayoutTests/http/tests/media/**media-source/mediasource-play-** > then-seek-back.html#newcode53<https://codereview.chromium.org/20114005/diff/1/LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html#newcode53> > LayoutTests/http/tests/media/**media-source/mediasource-play-** > then-seek-back.html:53: > test.waitForExpectedEvents(**function() > On 2013/07/24 23:16:43, wolenetz wrote: > >> It's highly likely that the previous currentTime > 0.5 check will >> > fail. Perhaps > >> you want to setTimeout() to give playback some time after "playing" >> > event > >> received? Otherwise, it's likely new expectations for events won't be >> > added, and > >> the seek back to 0 will not have been started at this point. >> > > Done. > > > https://codereview.chromium.**org/20114005/diff/1/** > LayoutTests/http/tests/media/**media-source/mediasource-play-** > then-seek-back.html#newcode56<https://codereview.chromium.org/20114005/diff/1/LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html#newcode56> > LayoutTests/http/tests/media/**media-source/mediasource-play-** > then-seek-back.html:56: > mediaElement.HAVE_FUTURE_DATA, "Greater than HAVE_FUTURE_DATA"); > On 2013/07/24 23:16:43, wolenetz wrote: > >> I'm not sure all implementations can guarantee they estimate they can >> > play > >> through to completion at this point. This assertion may introduce >> > flakiness. > > Removed. > > > https://codereview.chromium.**org/20114005/diff/5001/** > LayoutTests/http/tests/media/**media-source/mediasource-play-** > then-seek-back.html<https://codereview.chromium.org/20114005/diff/5001/LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html> > File > LayoutTests/http/tests/media/**media-source/mediasource-play-** > then-seek-back.html > (right): > > https://codereview.chromium.**org/20114005/diff/5001/** > LayoutTests/http/tests/media/**media-source/mediasource-play-** > then-seek-back.html#newcode40<https://codereview.chromium.org/20114005/diff/5001/LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html#newcode40> > LayoutTests/http/tests/media/**media-source/mediasource-play-** > then-seek-back.html:40: > test.failOnEvent(mediaElement, 'error'); > On 2013/07/25 23:11:31, acolwell wrote: > >> nit: You shouldn't need this here since you have this in the code >> > above. > > Done. > > > https://codereview.chromium.**org/20114005/diff/5001/** > LayoutTests/http/tests/media/**media-source/mediasource-play-** > then-seek-back.html#newcode52<https://codereview.chromium.org/20114005/diff/5001/LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html#newcode52> > LayoutTests/http/tests/media/**media-source/mediasource-play-** > then-seek-back.html:52: > assert_greater_than(**mediaElement.currentTime, 0.0, > On 2013/07/25 23:11:31, acolwell wrote: > >> nit: put this all one one line. >> > > Done. > > > https://codereview.chromium.**org/20114005/diff/5001/** > LayoutTests/http/tests/media/**media-source/mediasource-play-** > then-seek-back.html#newcode74<https://codereview.chromium.org/20114005/diff/5001/LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html#newcode74> > LayoutTests/http/tests/media/**media-source/mediasource-play-** > then-seek-back.html:74: > setTimeout(playAndSeek(test, mediaElement), 2000); > On 2013/07/25 23:11:31, acolwell wrote: > >> this calls playAndSeek() immediately. It isn't waiting 2 seconds >> > before calling. > >> I think you are intending to do something along these lines. >> > > setTimeout(function() { playAndSeek(test, mediaElement); }, 2000); >> > > Thanks. Done. > > https://codereview.chromium.**org/20114005/<https://codereview.chromium.org/2... >
https://codereview.chromium.org/20114005/diff/10001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html (right): https://codereview.chromium.org/20114005/diff/10001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:4: <script src='/w3c/resources/testharness.js'></script> consistency/interoperability nit: Our other tests use double quotes here (in html), but single quotes within the js itself. https://codereview.chromium.org/20114005/diff/10001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:51: assert_greater_than(mediaElement.currentTime, 0.0, 'Playback has started.'); This assert could be flaky because playback may have just now begun, even with the earlier setTimeout. Why not just let this waitForExpectedEvents be done outside the timeout, and trigger the timeout wait from within the handler, and then assert in the timeout callback that currentTime is now > 0.0?
(Also got sent too soon :)) I don't think 300ms delay one time should introduce any flakiness. If it does, then we need to consider increasing the test's overall timeout option.
Thanks, Matt. PTAL. https://codereview.chromium.org/20114005/diff/10001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html (right): https://codereview.chromium.org/20114005/diff/10001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:4: <script src='/w3c/resources/testharness.js'></script> On 2013/07/29 21:23:40, wolenetz wrote: > consistency/interoperability nit: Our other tests use double quotes here (in > html), but single quotes within the js itself. Thanks. Done. https://codereview.chromium.org/20114005/diff/10001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:51: assert_greater_than(mediaElement.currentTime, 0.0, 'Playback has started.'); On 2013/07/29 21:23:40, wolenetz wrote: > This assert could be flaky because playback may have just now begun, even with > the earlier setTimeout. Why not just let this waitForExpectedEvents be done > outside the timeout, and trigger the timeout wait from within the handler, and > then assert in the timeout callback that currentTime is now > 0.0? Not sure if PS#4 captures what you are recommending here. PTAL. Also, should I do something similar for the "playing" check after seeking back to 0?
LGTM % nits. I highly recommend you also obtain acolwell's lgtm. Until we get better logging implemented in our version of testharnessreport.js, new test development may require some temporary, private failing asserts (like printf debugging, :() to ensure the expected code paths being reached. (Just don't get commit the versions with the private failing asserts.) https://codereview.chromium.org/20114005/diff/10001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html (right): https://codereview.chromium.org/20114005/diff/10001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:51: assert_greater_than(mediaElement.currentTime, 0.0, 'Playback has started.'); On 2013/07/29 22:55:51, anandc wrote: > On 2013/07/29 21:23:40, wolenetz wrote: > > This assert could be flaky because playback may have just now begun, even with > > the earlier setTimeout. Why not just let this waitForExpectedEvents be done > > outside the timeout, and trigger the timeout wait from within the handler, and > > then assert in the timeout callback that currentTime is now > 0.0? > > Not sure if PS#4 captures what you are recommending here. PTAL. > Also, should I do something similar for the "playing" check after seeking back > to 0? Yes, you've captured my recommendation well :). Regarding after seeking back to 0, you've also captured what I think can best be tested and should be guaranteed. https://codereview.chromium.org/20114005/diff/17001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html (right): https://codereview.chromium.org/20114005/diff/17001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:47: function playAndSeek(test, mediaElement) nit: prefer 'var someTimeoutHandlerVariable = test.step_func(...)'. I'm also not sure, but it may be a good idea to store the timeoutID and override the test.done() to cancel any in-flight timeouts that haven't yet been dispatched. https://codereview.chromium.org/20114005/diff/17001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:58: function confirmPlaying(test, mediaElement) nit: ditto of my playAndSeek() comment, above. https://codereview.chromium.org/20114005/diff/17001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:71: test.expectEvent(mediaElement, 'seeking', 'mediaElement'); nit: This makes more sense just prior to setting the currentTime to 0.0 in playAndSeek().
Thanks a lot, Matt. PTAL at PS5. :-) This was tested with a failing assert placed, separately, in the 2 timeout handlers, and they failed as expected when the layout test was run. They've been removed from the patch; were only added for debugging. Thanks for that tip. acolwell@: PTAL as well when you have the time. Thank you. https://codereview.chromium.org/20114005/diff/17001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html (right): https://codereview.chromium.org/20114005/diff/17001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:47: function playAndSeek(test, mediaElement) On 2013/07/30 18:44:21, wolenetz wrote: > nit: prefer 'var someTimeoutHandlerVariable = test.step_func(...)'. I'm also not > sure, but it may be a good idea to store the timeoutID and override the > test.done() to cancel any in-flight timeouts that haven't yet been dispatched. The 1st part is done. The 2nd part: PS5 now calls clearTimeout before test.done(). Do we still have to override test.done(), if the timeoutID gets cleared in the manner currently implemented? https://codereview.chromium.org/20114005/diff/17001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:58: function confirmPlaying(test, mediaElement) On 2013/07/30 18:44:21, wolenetz wrote: > nit: ditto of my playAndSeek() comment, above. Done. https://codereview.chromium.org/20114005/diff/17001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:71: test.expectEvent(mediaElement, 'seeking', 'mediaElement'); On 2013/07/30 18:44:21, wolenetz wrote: > nit: This makes more sense just prior to setting the currentTime to 0.0 in > playAndSeek(). Agreed. Done.
Thanks Anand. I spotted some more nits in PS5. Also, the clearTimeout() route in PS5 needs further work if pursued. Hence, I'm marking NOT LGTM. https://codereview.chromium.org/20114005/diff/24001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html (right): https://codereview.chromium.org/20114005/diff/24001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:49: var playTimeoutHandler = test.step_func(function playAndSeek(test, mediaElement) nits: remove ' playAndSeek' because the function can be anonymous. Also, test and mediaElement are in scope, and don't need to be passed. https://codereview.chromium.org/20114005/diff/24001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:61: var playAfterSeekHandler = test.step_func(function confirmPlaying(test, mediaElement) nits: anonymize the function and remove redundant function parameters. https://codereview.chromium.org/20114005/diff/24001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:64: clearTimeout(playTimeoutID); See my comments @ line 67 and 74, below. This clearTimeout(...) would best be done within a new test.done(). https://codereview.chromium.org/20114005/diff/24001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:67: setTimeout(function() {playAfterSeekHandler(test, mediaElement)}, 300); Ditto with my playTimeoutID comment @ line 74, below: This timeout may similarly require cancellation during test.done(). https://codereview.chromium.org/20114005/diff/24001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:74: playTimeoutID = setTimeout(function() {playTimeoutHandler(test, mediaElement)}, 300); If going this route with intent to cancel your not-yet-dispatched timeouts prior to ending test (I'm still not sure if required), then overriding test.done() to clearTimeout(playTimeoutID), then call the overridden test.done() is necessary. Otherwise, the harness' internal test timeout that calls test.done() won't cancel your custom timeouts on test TIMEOUT. See mediasource-utils.js for example for how to override/chain the previous test.done(). Since this route involves two timeout expectations and overriding test.done() for each, I recommend lifting such expectation management and overriding utility to mediasource-utils.js.
Thanks a lot for all the feedback, Matt. PTAL. How do we figure out for sure if we must have clear out not-yet-dispatched timeouts prior to ending test? Is it just good coding practice? https://codereview.chromium.org/20114005/diff/24001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html (right): https://codereview.chromium.org/20114005/diff/24001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:49: var playTimeoutHandler = test.step_func(function playAndSeek(test, mediaElement) On 2013/07/31 01:20:45, wolenetz wrote: > nits: remove ' playAndSeek' because the function can be anonymous. Also, test > and mediaElement are in scope, and don't need to be passed. Done. https://codereview.chromium.org/20114005/diff/24001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:61: var playAfterSeekHandler = test.step_func(function confirmPlaying(test, mediaElement) On 2013/07/31 01:20:45, wolenetz wrote: > nits: anonymize the function and remove redundant function parameters. Done. https://codereview.chromium.org/20114005/diff/24001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:64: clearTimeout(playTimeoutID); On 2013/07/31 01:20:45, wolenetz wrote: > See my comments @ line 67 and 74, below. This clearTimeout(...) would best be > done within a new test.done(). Done. https://codereview.chromium.org/20114005/diff/24001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:67: setTimeout(function() {playAfterSeekHandler(test, mediaElement)}, 300); On 2013/07/31 01:20:45, wolenetz wrote: > Ditto with my playTimeoutID comment @ line 74, below: This timeout may similarly > require cancellation during test.done(). Done. https://codereview.chromium.org/20114005/diff/24001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:74: playTimeoutID = setTimeout(function() {playTimeoutHandler(test, mediaElement)}, 300); On 2013/07/31 01:20:45, wolenetz wrote: > If going this route with intent to cancel your not-yet-dispatched timeouts prior > to ending test (I'm still not sure if required), then overriding test.done() to > clearTimeout(playTimeoutID), then call the overridden test.done() is necessary. > Otherwise, the harness' internal test timeout that calls test.done() won't > cancel your custom timeouts on test TIMEOUT. See mediasource-utils.js for > example for how to override/chain the previous test.done(). > > Since this route involves two timeout expectations and overriding test.done() > for each, I recommend lifting such expectation management and overriding utility > to mediasource-utils.js. PTAL at updated mediasource-utils.
I like the util additions. Some further work is necessary. Clearing pending timeouts is good coding practice IMO. If we don't clear them, and the objects are still around after test.done(), edge cases of spurious timeout processing may exist if the pending timeout is finally delivered after test.done() or harness TIMEOUT. https://codereview.chromium.org/20114005/diff/33001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-util.js (right): https://codereview.chromium.org/20114005/diff/33001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:35: var timeoutID = setTimeout(callback, timeout); For purposes of automatically failing the test if not all expected timeouts have occurred by the time test.done() is running, need to intercept the callback with a wrapping function that removes timeoutID from this.timeoutIDs_. Also, test.done() needs to check if all timeout expectations have been met. https://codereview.chromium.org/20114005/diff/33001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:186: test.callOnTimeout = function(callback, timeout) nit: Would 'expectTimeout' be better? https://codereview.chromium.org/20114005/diff/33001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:206: assert_false(test.eventExpectations_.expectingEvents(), "No pending event expectations."); Need to also assert that there are no pending timeouts still expected (other than any in the test harness or others unmanaged by EventExpectationsManager.
Thanks again, Matt. PTAL. https://codereview.chromium.org/20114005/diff/33001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-util.js (right): https://codereview.chromium.org/20114005/diff/33001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:35: var timeoutID = setTimeout(callback, timeout); On 2013/07/31 19:42:37, wolenetz wrote: > For purposes of automatically failing the test if not all expected timeouts have > occurred by the time test.done() is running, need to intercept the callback with > a wrapping function that removes timeoutID from this.timeoutIDs_. Also, > test.done() needs to check if all timeout expectations have been met. Done. https://codereview.chromium.org/20114005/diff/33001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:186: test.callOnTimeout = function(callback, timeout) On 2013/07/31 19:42:37, wolenetz wrote: > nit: Would 'expectTimeout' be better? Now named expectDelayedCallback. https://codereview.chromium.org/20114005/diff/33001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:206: assert_false(test.eventExpectations_.expectingEvents(), "No pending event expectations."); On 2013/07/31 19:42:37, wolenetz wrote: > Need to also assert that there are no pending timeouts still expected (other > than any in the test harness or others unmanaged by EventExpectationsManager. Done.
Almost there :) https://codereview.chromium.org/20114005/diff/37001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-util.js (right): https://codereview.chromium.org/20114005/diff/37001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:37: // 2. Clean-up the list of timeoutIDs, removing the first in the list I don't think we should require that the timeoutIDs be in chronological order. Regardless, the wrapper should remove exactly the timeoutID that was created for it. Otherwise, clearTimeout() later during test.done() could clear the wrong ID(s). Maybe you could use a new utility function within the manager to remove a specific ID from the EventExpectationsManager's timeoutIDs_, to make the wrapper simpler.
Agreed, we should cleanly track and remove timeoutIDs, without assuming chronology. PTAL. Thanks. https://codereview.chromium.org/20114005/diff/37001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-util.js (right): https://codereview.chromium.org/20114005/diff/37001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:37: // 2. Clean-up the list of timeoutIDs, removing the first in the list On 2013/08/01 00:16:15, wolenetz wrote: > I don't think we should require that the timeoutIDs be in chronological order. > Regardless, the wrapper should remove exactly the timeoutID that was created for > it. Otherwise, clearTimeout() later during test.done() could clear the wrong > ID(s). Maybe you could use a new utility function within the manager to remove a > specific ID from the EventExpectationsManager's timeoutIDs_, to make the wrapper > simpler. Makes sense. Done. There also was a bug where, if the callback happened to also call test.done(), the timeoutID for that callback wouldn't get removed. (This was found in testing prior to PS7 being uploaded, but fix didn't get committed with it.) The bug has been fixed by removing the ID first then invoking the callback.
Thanks for catching the bug to remove the ID prior to executing the callback. The clearTimeout/timeoutIDs[] maintenance code looks incorrect still. Please see my new comments. https://codereview.chromium.org/20114005/diff/42001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-util.js (right): https://codereview.chromium.org/20114005/diff/42001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:40: timeoutIDs.splice(index, 1); I don't think this will always work as intended. Example: expectDelayedCallback(callback1, 50); --> timeoutIDs[0] = id of callback1 wrapper --> callback1 wrapper will be told to use index 0 expectDelayedCallback(callback2, 1000); --> timeoutIDs[1] = id of callback2 wrapper --> callback2 wrapper will be told to use index 1 ... ~50 ms elapses and callback1 wrapper is called --> timeoutIDs resulting from splice(0,1) is [callback2 ID (at index 0, *not at index 1*)] ... ~950 more ms elapse and callback2 wrapper is called --> timeoutIDs resulting from splice(1,1) is still [callback2 ID]. Takeaway is that the wrapper shouldn't assume constant index. Rather, it should remove the specific timeoutID to which it is associated. One way to do this is to have a wrapper object for the timeoutID be defined and registered to be passed in the setTimeout call, and then immediately update that wrapper object with the timeoutID that was returned from the setTimeout call (and use that wrapper object's timeoutID when the callback wrapper is called to remove the correct timeoutID from the manager's timeoutIDs). https://codereview.chromium.org/20114005/diff/42001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:45: var timeoutIDs = this.timeoutIDs_; nit: why not just use this.timeoutIDs_ everywhere, even in the callback wrapper? It's at least a redundant argument currently.
Thanks for catching that bug. PTAL: using a hashmap to store timeoutIDs now. https://codereview.chromium.org/20114005/diff/42001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-util.js (right): https://codereview.chromium.org/20114005/diff/42001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:40: timeoutIDs.splice(index, 1); On 2013/08/01 18:32:24, wolenetz wrote: > I don't think this will always work as intended. Example: > expectDelayedCallback(callback1, 50); > --> timeoutIDs[0] = id of callback1 wrapper > --> callback1 wrapper will be told to use index 0 > expectDelayedCallback(callback2, 1000); > --> timeoutIDs[1] = id of callback2 wrapper > --> callback2 wrapper will be told to use index 1 > ... ~50 ms elapses and callback1 wrapper is called > --> timeoutIDs resulting from splice(0,1) is [callback2 ID (at index 0, *not at > index 1*)] > ... ~950 more ms elapse and callback2 wrapper is called > --> timeoutIDs resulting from splice(1,1) is still [callback2 ID]. > > Takeaway is that the wrapper shouldn't assume constant index. Rather, it should > remove the specific timeoutID to which it is associated. One way to do this is > to have a wrapper object for the timeoutID be defined and registered to be > passed in the setTimeout call, and then immediately update that wrapper object > with the timeoutID that was returned from the setTimeout call (and use that > wrapper object's timeoutID when the callback wrapper is called to remove the > correct timeoutID from the manager's timeoutIDs). This was really useful. Thanks. Done. https://codereview.chromium.org/20114005/diff/42001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:45: var timeoutIDs = this.timeoutIDs_; On 2013/08/01 18:32:24, wolenetz wrote: > nit: why not just use this.timeoutIDs_ everywhere, even in the callback wrapper? > It's at least a redundant argument currently. Done in most places. There is a problem with the reference used when this.timeoutIDs_ is passed as a parameter when calling callbackWrapper in the line below. It results in the following error: Uncaught TypeError: Cannot convert undefined or null to object So keeping a local variable here just for that case.
The cleanup in PS9's overridden test.done() is broken. See my new comments. https://codereview.chromium.org/20114005/diff/52001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-util.js (right): https://codereview.chromium.org/20114005/diff/52001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:7: this.timeoutIDs_ = new Array(); new Array() is the same as []. At least according to JSON.stringify(). Do you mean to use {} or []? Any of these three appear to work when I tested in private (with the caveat that removing '[i]' in the enumeration below is necessary to make any of these options work - see my later comment), so I'm not sure why 'new Array()' is used here. Also, I'm not sure Array is most efficient here vs just Object properties by way of {}. https://codereview.chromium.org/20114005/diff/52001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:214: for (var i in test.eventExpectations_.timeoutIDs_[i]) { I believe this [i] breaks the enumeration code. Example (compare the resulting s and v, below): a=new Array(); --> [] a --> [] a[100]=100; --> 100 a --> Array[101] s = new String(); --> String {} for (var i in a) { s+=(a[i]).toString()+',' } --> "100," s --> "100," v = new String(); --> String {} for (var i in a[i]) { v+=(a[i]).toString()+',' } --> undefined v --> String {}
Yes, thanks for catching that, don't know how that snuck in. Fixed now. PTAL. https://codereview.chromium.org/20114005/diff/52001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-util.js (right): https://codereview.chromium.org/20114005/diff/52001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:7: this.timeoutIDs_ = new Array(); On 2013/08/02 20:04:25, wolenetz wrote: > new Array() is the same as []. At least according to JSON.stringify(). Do you > mean to use {} or []? Any of these three appear to work when I tested in private > (with the caveat that removing '[i]' in the enumeration below is necessary to > make any of these options work - see my later comment), so I'm not sure why 'new > Array()' is used here. Also, I'm not sure Array is most efficient here vs just > Object properties by way of {}. Thanks for trying out the different options. Going with {} now. https://codereview.chromium.org/20114005/diff/52001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:214: for (var i in test.eventExpectations_.timeoutIDs_[i]) { On 2013/08/02 20:04:25, wolenetz wrote: > I believe this [i] breaks the enumeration code. > Example (compare the resulting s and v, below): > a=new Array(); > --> [] > a > --> [] > a[100]=100; > --> 100 > a > --> Array[101] > s = new String(); > --> String {} > for (var i in a) { s+=(a[i]).toString()+',' } > --> "100," > s > --> "100," > v = new String(); > --> String {} > for (var i in a[i]) { v+=(a[i]).toString()+',' } > --> undefined > v > --> String {} Stupid bug. Thanks for catching. Fixed.
lgtm % nits. This will also need acolwell's CR. https://codereview.chromium.org/20114005/diff/56001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-util.js (right): https://codereview.chromium.org/20114005/diff/56001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:40: var callbackWrapper = function(callback, timeoutIDs) { nit: Remove timeoutIDs parameter. It should still be in scope, just like timeoutIDHolder. Move declaration/definition of timeoutIDs to before callbackWrapper, too. https://codereview.chromium.org/20114005/diff/56001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:47: timeoutIDHolder = setTimeout(function() {callbackWrapper(callback, timeoutIDs);}, delay); nit: Remove timeoutIDs parameter. It should still be in scope, just like timeoutIDHolder. https://codereview.chromium.org/20114005/diff/56001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:50: this.timeoutIDs_[timeoutIDHolder] = timeoutIDHolder; low-pri nit: We have a local reference to this.timeoutIDs_ already in timeoutIDs. Use it instead of this.timeoutIDs_ here?
Oops, I sent earlier too soon. I found some more nits and an issue that I believe requires fixing before commit. Sorry, not lgtm. https://codereview.chromium.org/20114005/diff/56001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html (right): https://codereview.chromium.org/20114005/diff/56001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:17: mediasource_test(function(test, mediaElement, mediaSource) nit: Indentation (4 spaces). https://codereview.chromium.org/20114005/diff/56001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:31: function getSegment(mediaData, info) nit: Remove unused function. https://codereview.chromium.org/20114005/diff/56001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:64: test.expectDelayedCallback(playAfterSeekHandler, 300); More than a nit: This line needs to be done only after the timeuptate+seeked events have been received. So, put it within another test.waitForExpectedEvents(function() { <here> }); block. If too hard to read, abstract into functions. Otherwise, it is possible the seek never completed but the implementation is (broken and) misreporting currentTime, or the implementation is broken and didn't report a completed seek. Note, if seek never completed, lack of event delivery (timeupdate + seeked) would be caught in test.done(), so this could be deemed a nit. IMO, it's best to fix because this test could be used as a template and other asynch code inserted before the test.done() that presumes incorrectly that all the seek completed events had already been confirmed delivered.
Thanks a lot again, Matt. PTAL. https://codereview.chromium.org/20114005/diff/56001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html (right): https://codereview.chromium.org/20114005/diff/56001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:17: mediasource_test(function(test, mediaElement, mediaSource) On 2013/08/02 21:41:37, wolenetz wrote: > nit: Indentation (4 spaces). Done. https://codereview.chromium.org/20114005/diff/56001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:31: function getSegment(mediaData, info) On 2013/08/02 21:41:37, wolenetz wrote: > nit: Remove unused function. Huh. Done. https://codereview.chromium.org/20114005/diff/56001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:64: test.expectDelayedCallback(playAfterSeekHandler, 300); On 2013/08/02 21:41:37, wolenetz wrote: > More than a nit: > This line needs to be done only after the timeuptate+seeked events have been > received. So, put it within another test.waitForExpectedEvents(function() { > <here> }); block. If too hard to read, abstract into functions. > > Otherwise, it is possible the seek never completed but the implementation is > (broken and) misreporting currentTime, or the implementation is broken and > didn't report a completed seek. Note, if seek never completed, lack of event > delivery (timeupdate + seeked) would be caught in test.done(), so this could be > deemed a nit. IMO, it's best to fix because this test could be used as a > template and other asynch code inserted before the test.done() that presumes > incorrectly that all the seek completed events had already been confirmed > delivered. Thanks for the thorough review and explanations. It helps me better understand MSE flow and the way our library for expectations management works. Done. https://codereview.chromium.org/20114005/diff/56001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-util.js (right): https://codereview.chromium.org/20114005/diff/56001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:40: var callbackWrapper = function(callback, timeoutIDs) { On 2013/08/02 21:24:07, wolenetz wrote: > nit: Remove timeoutIDs parameter. It should still be in scope, just like > timeoutIDHolder. Move declaration/definition of timeoutIDs to before > callbackWrapper, too. Done. https://codereview.chromium.org/20114005/diff/56001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:50: this.timeoutIDs_[timeoutIDHolder] = timeoutIDHolder; On 2013/08/02 21:24:07, wolenetz wrote: > low-pri nit: We have a local reference to this.timeoutIDs_ already in > timeoutIDs. Use it instead of this.timeoutIDs_ here? Done. https://codereview.chromium.org/20114005/diff/56001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:50: this.timeoutIDs_[timeoutIDHolder] = timeoutIDHolder; On 2013/08/02 21:24:07, wolenetz wrote: > low-pri nit: We have a local reference to this.timeoutIDs_ already in > timeoutIDs. Use it instead of this.timeoutIDs_ here? Done.
lgtm % nits. Thanks for following through my sometimes-verbose explanations :) https://codereview.chromium.org/20114005/diff/63001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html (right): https://codereview.chromium.org/20114005/diff/63001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:40: var confirmPlayAfterSeek = test.step_func(function(){ nit: { on next line. https://codereview.chromium.org/20114005/diff/63001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:45: assert_greater_than(mediaElement.currentTime, 0.0, 'Playback has started after seek.'); nit: indent 4. hmm. I missed this one earlier, too :) https://codereview.chromium.org/20114005/diff/63001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:60: confirmPlayAfterSeek(); low-pri nit: (like below; read the next comment first for better context...) Wrap this call inside test.waitForExpectedEvents (move the wait here from confirmPlayAfterSeek). Also, change the name from confirmPlayAfterSeek to confirmPlayThenEnd. Maybe also change delayedPlayAfterSeekHandler to be just delayedPlayHandlerThatEnds (or somesuch...) https://codereview.chromium.org/20114005/diff/63001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:70: seekAndPlayAgain(); low-pri nit: Wrap this call inside test.waitForExpectedEvents (move the wait here from seekAndPlayAgain()). And/or, maybe rename seekAndPlayAgain() to be finishSeekThenPlay? Otherwise, it's unclear when the seek is actually started (it's started when delayedPlayHandler sets currentTime, not in seekAndPlayAgain()). Maybe even do something like: test.waitForExpectedEvents(function(){finishSeekThen(confirmPlayAfterSeek);}); And change the call in confirmPlayAfterSeek to chain to the passed-in function instead.
one other nit found. https://codereview.chromium.org/20114005/diff/63001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-util.js (right): https://codereview.chromium.org/20114005/diff/63001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:47: timeoutIDHolder = setTimeout(function() {callbackWrapper(callback);}, delay); nit: I'm not sure this line is style-correct. Break into multiple? Add whitespace?
Thanks, Matt. Updated. acolwell@: PTAL as well. Thank you. https://codereview.chromium.org/20114005/diff/63001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html (right): https://codereview.chromium.org/20114005/diff/63001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:40: var confirmPlayAfterSeek = test.step_func(function(){ On 2013/08/02 23:06:45, wolenetz wrote: > nit: { on next line. Done. https://codereview.chromium.org/20114005/diff/63001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:45: assert_greater_than(mediaElement.currentTime, 0.0, 'Playback has started after seek.'); On 2013/08/02 23:06:45, wolenetz wrote: > nit: indent 4. hmm. I missed this one earlier, too :) Done. https://codereview.chromium.org/20114005/diff/63001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:60: confirmPlayAfterSeek(); On 2013/08/02 23:06:45, wolenetz wrote: > low-pri nit: (like below; read the next comment first for better context...) > Wrap this call inside test.waitForExpectedEvents (move the wait here from > confirmPlayAfterSeek). Also, change the name from confirmPlayAfterSeek to > confirmPlayThenEnd. Maybe also change delayedPlayAfterSeekHandler to be just > delayedPlayHandlerThatEnds (or somesuch...) Done. https://codereview.chromium.org/20114005/diff/63001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:70: seekAndPlayAgain(); On 2013/08/02 23:06:45, wolenetz wrote: > low-pri nit: Wrap this call inside test.waitForExpectedEvents (move the wait > here from seekAndPlayAgain()). And/or, maybe rename seekAndPlayAgain() to be > finishSeekThenPlay? Otherwise, it's unclear when the seek is actually started > (it's started when delayedPlayHandler sets currentTime, not in > seekAndPlayAgain()). > > Maybe even do something like: > test.waitForExpectedEvents(function(){finishSeekThen(confirmPlayAfterSeek);}); > And change the call in confirmPlayAfterSeek to chain to the passed-in function > instead. Done. https://codereview.chromium.org/20114005/diff/63001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-util.js (right): https://codereview.chromium.org/20114005/diff/63001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:47: timeoutIDHolder = setTimeout(function() {callbackWrapper(callback);}, delay); On 2013/08/02 23:13:02, wolenetz wrote: > nit: I'm not sure this line is style-correct. Break into multiple? Add > whitespace? Done.
https://codereview.chromium.org/20114005/diff/63001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html (right): https://codereview.chromium.org/20114005/diff/63001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:70: seekAndPlayAgain(); On 2013/08/02 23:06:45, wolenetz wrote: > low-pri nit: Wrap this call inside test.waitForExpectedEvents (move the wait > here from seekAndPlayAgain()). And/or, maybe rename seekAndPlayAgain() to be > finishSeekThenPlay? Otherwise, it's unclear when the seek is actually started > (it's started when delayedPlayHandler sets currentTime, not in > seekAndPlayAgain()). > > Maybe even do something like: > test.waitForExpectedEvents(function(){finishSeekThen(confirmPlayAfterSeek);}); > And change the call in confirmPlayAfterSeek to chain to the passed-in function > instead. Done the first part. Didn't chain calls. Maybe for future rounds?
more nits :) thanks https://codereview.chromium.org/20114005/diff/63001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html (right): https://codereview.chromium.org/20114005/diff/63001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:70: seekAndPlayAgain(); On 2013/08/02 23:49:11, anandc wrote: > On 2013/08/02 23:06:45, wolenetz wrote: > > low-pri nit: Wrap this call inside test.waitForExpectedEvents (move the wait > > here from seekAndPlayAgain()). And/or, maybe rename seekAndPlayAgain() to be > > finishSeekThenPlay? Otherwise, it's unclear when the seek is actually started > > (it's started when delayedPlayHandler sets currentTime, not in > > seekAndPlayAgain()). > > > > Maybe even do something like: > > test.waitForExpectedEvents(function(){finishSeekThen(confirmPlayAfterSeek);}); > > And change the call in confirmPlayAfterSeek to chain to the passed-in function > > instead. > Done the first part. Didn't chain calls. Maybe for future rounds? looks good. chaining was just an idea for readability. thanks! https://codereview.chromium.org/20114005/diff/63001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-util.js (right): https://codereview.chromium.org/20114005/diff/63001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:47: timeoutIDHolder = setTimeout(function() {callbackWrapper(callback);}, delay); On 2013/08/02 23:46:43, anandc wrote: > On 2013/08/02 23:13:02, wolenetz wrote: > > nit: I'm not sure this line is style-correct. Break into multiple? Add > > whitespace? > > Done. I'm still not sure about blink js style: Is this correct, or just whitespace additions but not multi-line, or full-blown multi-line like how var eventHandler is initialized in expectEvent, above? acolwell@, PTAL. https://codereview.chromium.org/20114005/diff/63003/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-util.js (right): https://codereview.chromium.org/20114005/diff/63003/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:42: delete timeoutIDs[timeoutIDHolder]; Oh noes. Another nit: indent 4.
On 2013/08/03 00:06:28, wolenetz wrote: > more nits :) thanks > > https://codereview.chromium.org/20114005/diff/63001/LayoutTests/http/tests/me... > File > LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html > (right): > > https://codereview.chromium.org/20114005/diff/63001/LayoutTests/http/tests/me... > LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:70: > seekAndPlayAgain(); > On 2013/08/02 23:49:11, anandc wrote: > > On 2013/08/02 23:06:45, wolenetz wrote: > > > low-pri nit: Wrap this call inside test.waitForExpectedEvents (move the wait > > > here from seekAndPlayAgain()). And/or, maybe rename seekAndPlayAgain() to be > > > finishSeekThenPlay? Otherwise, it's unclear when the seek is actually > started > > > (it's started when delayedPlayHandler sets currentTime, not in > > > seekAndPlayAgain()). > > > > > > Maybe even do something like: > > > > test.waitForExpectedEvents(function(){finishSeekThen(confirmPlayAfterSeek);}); > > > And change the call in confirmPlayAfterSeek to chain to the passed-in > function > > > instead. > > Done the first part. Didn't chain calls. Maybe for future rounds? > > looks good. chaining was just an idea for readability. thanks! > > https://codereview.chromium.org/20114005/diff/63001/LayoutTests/http/tests/me... > File LayoutTests/http/tests/media/media-source/mediasource-util.js (right): > > https://codereview.chromium.org/20114005/diff/63001/LayoutTests/http/tests/me... > LayoutTests/http/tests/media/media-source/mediasource-util.js:47: > timeoutIDHolder = setTimeout(function() {callbackWrapper(callback);}, delay); > On 2013/08/02 23:46:43, anandc wrote: > > On 2013/08/02 23:13:02, wolenetz wrote: > > > nit: I'm not sure this line is style-correct. Break into multiple? Add > > > whitespace? > > > > Done. > > I'm still not sure about blink js style: Is this correct, or just whitespace > additions but not multi-line, or full-blown multi-line like how var eventHandler > is initialized in expectEvent, above? acolwell@, PTAL. > > https://codereview.chromium.org/20114005/diff/63003/LayoutTests/http/tests/me... > File LayoutTests/http/tests/media/media-source/mediasource-util.js (right): > > https://codereview.chromium.org/20114005/diff/63003/LayoutTests/http/tests/me... > LayoutTests/http/tests/media/media-source/mediasource-util.js:42: delete > timeoutIDs[timeoutIDHolder]; > Oh noes. Another nit: indent 4. Thanks. Fixed the indentation: is there any automated way to catch this? A recommended JS-linter?
https://codereview.chromium.org/20114005/diff/63001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-util.js (right): https://codereview.chromium.org/20114005/diff/63001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:47: timeoutIDHolder = setTimeout(function() {callbackWrapper(callback);}, delay); On 2013/08/03 00:06:28, wolenetz wrote: > On 2013/08/02 23:46:43, anandc wrote: > > On 2013/08/02 23:13:02, wolenetz wrote: > > > nit: I'm not sure this line is style-correct. Break into multiple? Add > > > whitespace? > > > > Done. > > I'm still not sure about blink js style: Is this correct, or just whitespace > additions but not multi-line, or full-blown multi-line like how var eventHandler > is initialized in expectEvent, above? acolwell@, PTAL. The white-space and multi-line additions were made based off a few examples in other layout-tests (albeit for assert calls). For example, these lines from mediasource-addsourcebuffer.html: assert_throws("InvalidAccessError", function() { mediaSource.addSourceBuffer(""); }, "addSourceBuffer() threw an exception when passed an empty string."); And this one from mediasource-closed.html: assert_throws("NotFoundError", function() { mediaSource.removeSourceBuffer(sourceBuffer); }, "removeSourceBuffer() throws an exception when closed."); Those examples do have the 1st parameter in the same line, so maybe I should do the same? https://codereview.chromium.org/20114005/diff/63003/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-util.js (right): https://codereview.chromium.org/20114005/diff/63003/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:42: delete timeoutIDs[timeoutIDHolder]; On 2013/08/03 00:06:28, wolenetz wrote: > Oh noes. Another nit: indent 4. Done.
Looks to me like it's ready for acolwell to CR. Thanks! https://codereview.chromium.org/20114005/diff/63001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-util.js (right): https://codereview.chromium.org/20114005/diff/63001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:47: timeoutIDHolder = setTimeout(function() {callbackWrapper(callback);}, delay); On 2013/08/03 00:27:25, anandc wrote: > On 2013/08/03 00:06:28, wolenetz wrote: > > On 2013/08/02 23:46:43, anandc wrote: > > > On 2013/08/02 23:13:02, wolenetz wrote: > > > > nit: I'm not sure this line is style-correct. Break into multiple? Add > > > > whitespace? > > > > > > Done. > > > > I'm still not sure about blink js style: Is this correct, or just whitespace > > additions but not multi-line, or full-blown multi-line like how var > eventHandler > > is initialized in expectEvent, above? acolwell@, PTAL. > > > The white-space and multi-line additions were made based off a few examples in > other layout-tests (albeit for assert calls). > > For example, these lines from mediasource-addsourcebuffer.html: > > assert_throws("InvalidAccessError", > function() { mediaSource.addSourceBuffer(""); }, > "addSourceBuffer() threw an exception when passed an empty > string."); > > And this one from mediasource-closed.html: > > assert_throws("NotFoundError", > function() { mediaSource.removeSourceBuffer(sourceBuffer); }, > "removeSourceBuffer() throws an exception when closed."); > > Those examples do have the 1st parameter in the same line, so maybe I should do > the same? Yeah, I'd error on the side of consistency. I prefer the mediasource-closed.html version (1st param on same line, and indents 4) vs the style example from mediasource-addsourcebuffer.html (relative -2 indent, or indent 11 ?! I'm not sure how we would keep consistent with this example.)
https://codereview.chromium.org/20114005/diff/73001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html (right): https://codereview.chromium.org/20114005/diff/73001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:20: assert_equals(segmentInfo.duration, 6.042, 'Expected test media duration'); nit: Why do you need to verify this specific value? It doesn't look like your test relies on this value anywhere. If you need the test file to be longer than a specific duration then you should use an assert_greater_than() to verify that segmentInfo.duration meets this minimal criteria. https://codereview.chromium.org/20114005/diff/73001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:40: var confirmPlayThenEnd = test.step_func(function() nit: I don't think you should need test.step_func() here. https://codereview.chromium.org/20114005/diff/73001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:42: var delayedPlayHandlerThatEnds = test.step_func(function() nit: I don't think you should use test.step_func() here either. In this case I think that test.expectedDelayCallback() should do this sort of wrapping internally so that code like this doesn't have to. https://codereview.chromium.org/20114005/diff/73001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:47: test.expectDelayedCallback(delayedPlayHandlerThatEnds, 300); nit: 300ms seems arbitrary. Should this just be baked into expectDelayedCallback's implementation so we don't have to copy it everywhere? https://codereview.chromium.org/20114005/diff/73001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:50: var finishSeekThenPlay = test.step_func(function() nit: I don't think step_func() is needed here. https://codereview.chromium.org/20114005/diff/73001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:63: var delayedPlayHandler = test.step_func(function() ditto and the "function delayedPlayHandler()" form is preferred here and above in these sorts of situations. This is inside a function here so you don't have to worry about this function becoming part of the global scope. https://codereview.chromium.org/20114005/diff/73001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-util.js (right): https://codereview.chromium.org/20114005/diff/73001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:41: var callbackWrapper = function(callback) { nit: callback parameter isn't necessary here. Just referencing it in this function will make it part of the closure. This is also where you should put the this.test_.step_func() so that the callback is properly wrapped. https://codereview.chromium.org/20114005/diff/73001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:48: function() { callbackWrapper(callback); }, nit: put the whole setTimeout call all on a single line. "setTimeout(callbackWrapper, delay);" should work fine here given the suggestions above.
Thanks a lot, Aaron. PTAL at PS#14. https://codereview.chromium.org/20114005/diff/73001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html (right): https://codereview.chromium.org/20114005/diff/73001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:20: assert_equals(segmentInfo.duration, 6.042, 'Expected test media duration'); On 2013/08/05 21:06:06, acolwell wrote: > nit: Why do you need to verify this specific value? It doesn't look like your > test relies on this value anywhere. If you need the test file to be longer than > a specific duration then you should use an assert_greater_than() to verify that > segmentInfo.duration meets this minimal criteria. Done. https://codereview.chromium.org/20114005/diff/73001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:40: var confirmPlayThenEnd = test.step_func(function() On 2013/08/05 21:06:06, acolwell wrote: > nit: I don't think you should need test.step_func() here. Done. https://codereview.chromium.org/20114005/diff/73001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:42: var delayedPlayHandlerThatEnds = test.step_func(function() On 2013/08/05 21:06:06, acolwell wrote: > nit: I don't think you should use test.step_func() here either. In this case I > think that test.expectedDelayCallback() should do this sort of wrapping > internally so that code like this doesn't have to. Done. https://codereview.chromium.org/20114005/diff/73001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:47: test.expectDelayedCallback(delayedPlayHandlerThatEnds, 300); On 2013/08/05 21:06:06, acolwell wrote: > nit: 300ms seems arbitrary. Should this just be baked into > expectDelayedCallback's implementation so we don't have to copy it everywhere? Moved delay to expectDelayCallback, defaulting to 300ms. https://codereview.chromium.org/20114005/diff/73001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:50: var finishSeekThenPlay = test.step_func(function() On 2013/08/05 21:06:06, acolwell wrote: > nit: I don't think step_func() is needed here. Done. https://codereview.chromium.org/20114005/diff/73001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:63: var delayedPlayHandler = test.step_func(function() On 2013/08/05 21:06:06, acolwell wrote: > ditto and the "function delayedPlayHandler()" form is preferred here and above > in these sorts of situations. This is inside a function here so you don't have > to worry about this function becoming part of the global scope. Done. https://codereview.chromium.org/20114005/diff/73001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-util.js (right): https://codereview.chromium.org/20114005/diff/73001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:41: var callbackWrapper = function(callback) { On 2013/08/05 21:06:06, acolwell wrote: > nit: callback parameter isn't necessary here. Just referencing it in this > function will make it part of the closure. > > This is also where you should put the this.test_.step_func() so that the > callback is properly wrapped. Done. https://codereview.chromium.org/20114005/diff/73001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:48: function() { callbackWrapper(callback); }, On 2013/08/05 21:06:06, acolwell wrote: > nit: put the whole setTimeout call all on a single line. > "setTimeout(callbackWrapper, delay);" should work fine here given the > suggestions above. Done. https://codereview.chromium.org/20114005/diff/81001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-util.js (right): https://codereview.chromium.org/20114005/diff/81001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:46: // Default to a wait time of 300ms. OK to manage the delay in this manner?
looks pretty good. One more observation that I think is worth pursuing. https://codereview.chromium.org/20114005/diff/81001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html (right): https://codereview.chromium.org/20114005/diff/81001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:76: test.expectDelayedCallback(delayedPlayHandler); After looking at this some more, it seems line what you really want here and above is something along the lines of test.waitForCurrentTimeToAdvance(mediaElement, function() { ... }); This function could capture the current value of currentTime and then listen for timeupdate events until currentTime changes. I think that would be more robust to bot load then a hard coded timeout. I also think it would make your intentions here more explicit. https://codereview.chromium.org/20114005/diff/81001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-util.js (right): https://codereview.chromium.org/20114005/diff/81001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:49: timeoutIDHolder = setTimeout(function() { callbackWrapper(); }, delay); nit: You shouldn't need to wrap callbackWrapper here. setTimeout(callbackWrapper, delay); should work.
https://codereview.chromium.org/20114005/diff/81001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html (right): https://codereview.chromium.org/20114005/diff/81001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:76: test.expectDelayedCallback(delayedPlayHandler); On 2013/08/06 18:42:37, acolwell wrote: > After looking at this some more, it seems line what you really want here and > above is something along the lines of > > test.waitForCurrentTimeToAdvance(mediaElement, function() > { > ... > }); > > This function could capture the current value of currentTime and then listen for > timeupdate events until currentTime changes. I think that would be more robust > to bot load then a hard coded timeout. I also think it would make your > intentions here more explicit. If going this route, please use a separate mechanism to listen / expect the timeupdate events than the existing expectEvent / waitForExpectedEvents. Otherwise, users of waitForCurrentTimeToAdvance may get flaky expectation matching for their expected timeupdate and other events on the mediaElement (sequence is enforced in waitForExpectedEvents; waitForCurrentTimeToAdvance shouldn't impact this other than mentioning in the util API that timeupdate events are used by this method to trigger checking for time advancement).
Thanks, Aaron. PTAL at PS#15. Matt recommended a different approach to what I have in PS15: At regular, small intervals, check for a time change and then execute callback. What I have done instead seems simpler. Let me know what you think. https://codereview.chromium.org/20114005/diff/81001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html (right): https://codereview.chromium.org/20114005/diff/81001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:76: test.expectDelayedCallback(delayedPlayHandler); On 2013/08/06 18:42:37, acolwell wrote: > After looking at this some more, it seems line what you really want here and > above is something along the lines of > > test.waitForCurrentTimeToAdvance(mediaElement, function() > { > ... > }); > > This function could capture the current value of currentTime and then listen for > timeupdate events until currentTime changes. I think that would be more robust > to bot load then a hard coded timeout. I also think it would make your > intentions here more explicit. This is a good idea, thanks. PTAL at the new waitForTimeUpdate in *utils.js. I thought of adding parameters that specify a target-time and a comparison operator, to check if current time is < or > target-time, and then calling the callback. Testing that properly took a while, so uploading this for now. https://codereview.chromium.org/20114005/diff/81001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-util.js (right): https://codereview.chromium.org/20114005/diff/81001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:49: timeoutIDHolder = setTimeout(function() { callbackWrapper(); }, delay); On 2013/08/06 18:42:37, acolwell wrote: > nit: You shouldn't need to wrap callbackWrapper here. > setTimeout(callbackWrapper, delay); should work. Got it. This function has been removed now. PTAL at waitForTimeUpdate.
It looks like your preliminary PS15 handles the util's timeupdate expectations independently of the expectEvent/waitForExpectedEvents, which is a good thing IMO :) One nit within PS15 (acolwell's others still need addressing): https://codereview.chromium.org/20114005/diff/87001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-util.js (right): https://codereview.chromium.org/20114005/diff/87001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:67: mediaElement.removeEventListener('timeupdate', onTimeUpdate); Simplify by only removing if the currentTime mismatches initialTime. No need to remove and re-add, then.
Thanks, Matt and Aaron for the offline input. PTAL. https://codereview.chromium.org/20114005/diff/81001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-util.js (right): https://codereview.chromium.org/20114005/diff/81001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:49: timeoutIDHolder = setTimeout(function() { callbackWrapper(); }, delay); On 2013/08/06 21:30:51, anandc wrote: > On 2013/08/06 18:42:37, acolwell wrote: > > nit: You shouldn't need to wrap callbackWrapper here. > > setTimeout(callbackWrapper, delay); should work. > > Got it. This function has been removed now. PTAL at waitForTimeUpdate. Hmm. Not removed, so fixing. https://codereview.chromium.org/20114005/diff/87001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-util.js (right): https://codereview.chromium.org/20114005/diff/87001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:67: mediaElement.removeEventListener('timeupdate', onTimeUpdate); On 2013/08/06 21:33:35, wolenetz wrote: > Simplify by only removing if the currentTime mismatches initialTime. No need to > remove and re-add, then. Done.
https://codereview.chromium.org/20114005/diff/81001/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-util.js (right): https://codereview.chromium.org/20114005/diff/81001/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:49: timeoutIDHolder = setTimeout(function() { callbackWrapper(); }, delay); On 2013/08/06 21:55:30, anandc wrote: > On 2013/08/06 21:30:51, anandc wrote: > > On 2013/08/06 18:42:37, acolwell wrote: > > > nit: You shouldn't need to wrap callbackWrapper here. > > > setTimeout(callbackWrapper, delay); should work. > > > > Got it. This function has been removed now. PTAL at waitForTimeUpdate. > > Hmm. Not removed, so fixing. > Actually removed. :-)
https://codereview.chromium.org/20114005/diff/94002/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-util.js (right): https://codereview.chromium.org/20114005/diff/94002/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:43: function onTimeUpdate() { nit: step_func here, so caller's callback doesn't need it. https://codereview.chromium.org/20114005/diff/94002/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:47: mediaElement.addEventListener('timeupdate', onTimeUpdate); nit: No need to re-add. While duplicates are suppressed and this should work, this line is unnecessary. https://codereview.chromium.org/20114005/diff/94002/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:199: test.expectDelayedCallback = function(callback, delay) This needs removal, too.
Thanks, Matt. PTAL. https://codereview.chromium.org/20114005/diff/94002/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-util.js (right): https://codereview.chromium.org/20114005/diff/94002/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:43: function onTimeUpdate() { On 2013/08/06 22:01:10, wolenetz wrote: > nit: step_func here, so caller's callback doesn't need it. Ah yes, thank you. Done. https://codereview.chromium.org/20114005/diff/94002/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:47: mediaElement.addEventListener('timeupdate', onTimeUpdate); On 2013/08/06 22:01:10, wolenetz wrote: > nit: No need to re-add. While duplicates are suppressed and this should work, > this line is unnecessary. Hmm. Did not know that duplicates are suppressed. Is that the default behaviour? Removing, based on your feedback. https://codereview.chromium.org/20114005/diff/94002/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:199: test.expectDelayedCallback = function(callback, delay) On 2013/08/06 22:01:10, wolenetz wrote: > This needs removal, too. Thanks. Done.
A couple more nits. Looking pretty good otherwise. https://codereview.chromium.org/20114005/diff/94002/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-util.js (right): https://codereview.chromium.org/20114005/diff/94002/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-util.js:47: mediaElement.addEventListener('timeupdate', onTimeUpdate); On 2013/08/06 22:17:42, anandc wrote: > On 2013/08/06 22:01:10, wolenetz wrote: > > nit: No need to re-add. While duplicates are suppressed and this should work, > > this line is unnecessary. > > Hmm. Did not know that duplicates are suppressed. Is that the default behaviour? > Removing, based on your feedback. > Yes, per http://www.w3.org/TR/2000/REC-DOM-Level-2-Events-20001113/events.html, adding same exact listener with same exact params is a no-op and only one remove would be necessary to remove the single listener. Also, listeners don't stop listening when dispatched. See our previous video-test.js waitForEvent() implementation, for example. https://codereview.chromium.org/20114005/diff/100001/LayoutTests/http/tests/m... File LayoutTests/http/tests/media/media-source/mediasource-util.js (right): https://codereview.chromium.org/20114005/diff/100001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-util.js:7: this.timeoutIDs_ = {}; Oh noes! This also needs removal :) https://codereview.chromium.org/20114005/diff/100001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-util.js:195: test.waitForTimeUpdate = function(mediaElement, callback) nit: Please add brief documentation here that not just timeupdate is sufficient for callback, but it will keep listening for timeupdates and only stop and invoke callback once mediaElement.currentTime has changed.
Thanks, Matt. PTAL. https://codereview.chromium.org/20114005/diff/100001/LayoutTests/http/tests/m... File LayoutTests/http/tests/media/media-source/mediasource-util.js (right): https://codereview.chromium.org/20114005/diff/100001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-util.js:195: test.waitForTimeUpdate = function(mediaElement, callback) On 2013/08/06 22:35:55, wolenetz wrote: > nit: Please add brief documentation here that not just timeupdate is sufficient > for callback, but it will keep listening for timeupdates and only stop and > invoke callback once mediaElement.currentTime has changed. Good point. Done.
lgtm % nits https://codereview.chromium.org/20114005/diff/100001/LayoutTests/http/tests/m... File LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html (right): https://codereview.chromium.org/20114005/diff/100001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:46: test.waitForTimeUpdate(mediaElement, delayedPlayHandlerThatEnds); nit: Just inline the function here. I think it would be clearer. https://codereview.chromium.org/20114005/diff/100001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:55: test.waitForExpectedEvents(function() nit: You can just do test.waitForExpectedEvents(confirmPlayThenEnd) here. https://codereview.chromium.org/20114005/diff/100001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:67: test.waitForExpectedEvents(function() nit: You can just do test.waitForExpectedEvents(finishSeekThenPlay) here. https://codereview.chromium.org/20114005/diff/100001/LayoutTests/http/tests/m... File LayoutTests/http/tests/media/media-source/mediasource-util.js (right): https://codereview.chromium.org/20114005/diff/100001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-util.js:195: test.waitForTimeUpdate = function(mediaElement, callback) On 2013/08/06 22:35:55, wolenetz wrote: > nit: Please add brief documentation here that not just timeupdate is sufficient > for callback, but it will keep listening for timeupdates and only stop and > invoke callback once mediaElement.currentTime has changed. nit: An alternative is to just change the method name to waitForCurrentTimeChange. https://codereview.chromium.org/20114005/diff/100001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-util.js:197: test.eventExpectations_.waitForTimeUpdate(mediaElement, callback); nit: Why is added as part of the EventExpectationsManager? It doesn't use any functionality in that class. I think you should put all the code in here instead. https://codereview.chromium.org/20114005/diff/100001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-util.js:209: for (var i in test.eventExpectations_.timeoutIDs_) { nit: I think this can be removed since timeoutIDs doesn't appear to be used anywhere. https://codereview.chromium.org/20114005/diff/100001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-util.js:214: assert_equals(Object.keys(test.eventExpectations_.timeoutIDs_).length, 0); nit: ditto
There are still some remnants of the expectDelayedCallback implementation route that need removal. As discussed, we'll land expectDelayedCallback in a separate CL when it is actually used. https://codereview.chromium.org/20114005/diff/106001/LayoutTests/http/tests/m... File LayoutTests/http/tests/media/media-source/mediasource-util.js (right): https://codereview.chromium.org/20114005/diff/106001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-util.js:7: this.timeoutIDs_ = {}; This still needs removal :) https://codereview.chromium.org/20114005/diff/106001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-util.js:211: // loop through and clear timeoutIDs And clearing timeoutIDs needs removal too. https://codereview.chromium.org/20114005/diff/106001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-util.js:217: assert_equals(Object.keys(test.eventExpectations_.timeoutIDs_).length, 0); And this line also needs removal.
Huge thanks again, Aaron & Matt. Going to commit PS19 unless there any objections. https://codereview.chromium.org/20114005/diff/100001/LayoutTests/http/tests/m... File LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html (right): https://codereview.chromium.org/20114005/diff/100001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:46: test.waitForTimeUpdate(mediaElement, delayedPlayHandlerThatEnds); On 2013/08/06 23:05:09, acolwell wrote: > nit: Just inline the function here. I think it would be clearer. Done. https://codereview.chromium.org/20114005/diff/100001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:55: test.waitForExpectedEvents(function() On 2013/08/06 23:05:09, acolwell wrote: > nit: You can just do test.waitForExpectedEvents(confirmPlayThenEnd) here. Done. https://codereview.chromium.org/20114005/diff/100001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:67: test.waitForExpectedEvents(function() On 2013/08/06 23:05:09, acolwell wrote: > nit: You can just do test.waitForExpectedEvents(finishSeekThenPlay) here. Done. https://codereview.chromium.org/20114005/diff/100001/LayoutTests/http/tests/m... File LayoutTests/http/tests/media/media-source/mediasource-util.js (right): https://codereview.chromium.org/20114005/diff/100001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-util.js:7: this.timeoutIDs_ = {}; On 2013/08/06 22:35:55, wolenetz wrote: > Oh noes! This also needs removal :) Many thanks for catching these. Done. https://codereview.chromium.org/20114005/diff/100001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-util.js:195: test.waitForTimeUpdate = function(mediaElement, callback) On 2013/08/06 23:05:09, acolwell wrote: > On 2013/08/06 22:35:55, wolenetz wrote: > > nit: Please add brief documentation here that not just timeupdate is > sufficient > > for callback, but it will keep listening for timeupdates and only stop and > > invoke callback once mediaElement.currentTime has changed. > nit: An alternative is to just change the method name to > waitForCurrentTimeChange. Renaming is better. Done. https://codereview.chromium.org/20114005/diff/100001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-util.js:197: test.eventExpectations_.waitForTimeUpdate(mediaElement, callback); On 2013/08/06 23:05:09, acolwell wrote: > nit: Why is added as part of the EventExpectationsManager? It doesn't use any > functionality in that class. I think you should put all the code in here > instead. Of course. :-) Thank you. Done. https://codereview.chromium.org/20114005/diff/100001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-util.js:209: for (var i in test.eventExpectations_.timeoutIDs_) { On 2013/08/06 23:05:09, acolwell wrote: > nit: I think this can be removed since timeoutIDs doesn't appear to be used > anywhere. Yes, removed. https://codereview.chromium.org/20114005/diff/100001/LayoutTests/http/tests/m... LayoutTests/http/tests/media/media-source/mediasource-util.js:214: assert_equals(Object.keys(test.eventExpectations_.timeoutIDs_).length, 0); On 2013/08/06 23:05:09, acolwell wrote: > nit: ditto Done.
lgtm % one more nit. Thanks for your patience :) https://codereview.chromium.org/20114005/diff/99002/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html (right): https://codereview.chromium.org/20114005/diff/99002/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:41: function delayedPlayHandlerThatEnds() nit: This needs removal now that it is anonymously inlined, below.
Thanks, Matt. Fixed. PTAL. https://codereview.chromium.org/20114005/diff/99002/LayoutTests/http/tests/me... File LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html (right): https://codereview.chromium.org/20114005/diff/99002/LayoutTests/http/tests/me... LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:41: function delayedPlayHandlerThatEnds() On 2013/08/06 23:57:13, wolenetz wrote: > nit: This needs removal now that it is anonymously inlined, below. Yes, should have done that earlier. Thanks. Done now.
On 2013/08/07 00:05:24, anandc wrote: > Thanks, Matt. Fixed. PTAL. > > https://codereview.chromium.org/20114005/diff/99002/LayoutTests/http/tests/me... > File > LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html > (right): > > https://codereview.chromium.org/20114005/diff/99002/LayoutTests/http/tests/me... > LayoutTests/http/tests/media/media-source/mediasource-play-then-seek-back.html:41: > function delayedPlayHandlerThatEnds() > On 2013/08/06 23:57:13, wolenetz wrote: > > nit: This needs removal now that it is anonymously inlined, below. > > Yes, should have done that earlier. Thanks. Done now. Cool. lgtm!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/anandc@chromium.org/20114005/117001
Message was sent while issue was closed.
Change committed as 155643 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
