You'll need a nsIDOMHTMLTimeRanges interface to implement this. Stubs for that are removed in patch for bug 462953.

I'm going to add a test for this patch, but I need to learn how to use the test framework first.

Comment on attachment 533718 [details] [diff] [review] Patch v2 for Bug 462959 Fixed a tiny bug I didn't catch.

A few comments: Read the style guide: <https://developer.mozilla.org/En/Developer_Guide/Coding_Style>, in particular if (..) { } instead of if(..) { } Same for for-loops, and in |for(PRUint32 i = 0 ;...|, drop the space before the semicolon. Don't change iff, it's correct. when the stream has ever been played -> if the stream has ever been played. Lose the new newline in nsHTMLMediaElement::Play. Use NS_MAX instead of PR_MAX. Rev the uuid of nsIDOMHTMLMediaElement.

This needs tests added to content/media/test. mRangePlay might be better named mCurrentPlayRangeStart (or, if you can think of a shorter name that makes it clear it's the start of the current play range, use that instead). + // Update the mPlayed membed. Typo - member? I don't think this comment is necessary though. I'm not sure I understand what mHasPlayed is for. It seems like you use |mRangePlay == -1| instead. + if(mRangePlay == -1.0) // first time we play + GetCurrentTime(&mRangePlay); + Please use {} brackets here, and remove the comment. Thanks for doing this!

The mHasPlayed member was indeed unecessary, thanks for pointing that out.

Comment on attachment 534709 [details] [diff] [review] Patch v5, adressing :kinetik concern's Thanks. I just realized you have already attached a mochitest for this in a separate patch. I'll review that in the morning.

I think the way you're using waitForExplicitFinish() and finish() in the test will result in the test finishing the first time finish() is called. Unless I'm missing something, there's only a single SimpleTest for the entire page. Take a look at content/media/test/test_load.html, which uses a counter of running tests to determine when to call finish(). You might need to use a longer file to get good test coverage. I'm worried that a 1 second file will mean that, e.g. play_pause could easily play through to the end once before the timer runs due to high load (and thus delayed event execution) on the mochitest servers. In fact, it'd be better to rewrite these tests to avoid setTimeout altogether, because it usually results in unreliable tests under load. See https://developer.mozilla.org/en/QA/Avoiding_intermittent_oranges

I've rewritten the tests, in order to remove all the calls to setInterval and setTimout. The tests should be machine-speed independent and much more reliable, now. The duration of the file I use was 9.3 seconds, (not 1 sec), and that should be long enough to avoid the problems you mentioned. I've also refactored the tests using the MediaTestManager class, provided in the manifest.js file, in the content/media/test directory, which implements what you proposed.

I think you attached an old version of the patch.

Indeed. Here is the good file.

I haven't completely reviewed it yet, but one thing I noticed: +manager.runTests(gAudioTests, initial_value); This causes the initial_value test to be called once for each file in gAudioTests, but then you're using AUDIO_FILE inside the test: + a1.src=AUDIO_FILE; You can add a new g*Tests in manifest.js for this, or you can use one of the existing ones. The advantage of using an existing one is that the tests will run with multiple decoder backends--which doesn't matter much for this code, but it could in the future.

Comment on attachment 536047 [details] [diff] [review] Patch v5, using the mediatestmanager to provide files >+// Used by test_played. We need a file long enough to avoid random orange >+var gPlayedTests = [ >+ { name:"big.wav", type:"audio/x-wav", duration:9.0, size:102444 }, >+ { name:"bogus.duh", type:"bogus/duh" } >+]; You should include at least one Ogg and one WebM file as well, and the tests should run on video as well as audio to ensure that this feature works across the different decoder back ends. You can see an example in test_bug495300.html at line 36 of how to create the appropriate audio/video element based on the test's "type" field. >+// Whithout playing, check that player.played.length == 0 >+manager.runTests(gPlayedTests, initial_value); >+// Play the file, test the range we have >+manager.runTests(gPlayedTests, no_merge); >+// Play the first half of the file, pause, play >+// an check we have only one range >+manager.runTests(gPlayedTests, play_pause); >+// Play the first half of the file, seek back, while >+// continuing to play. We shall have only one range >+manager.runTests(gPlayedTests, play_seek); >+// Play and seek to have two ranges, and check that, as well a >+// boudaries >+manager.runTests(gPlayedTests, multiple_range_nomerge); >+// Play and seek to have to overlapping ranges. They should be merged, to a >+// range spanning all the test audio file. >+manager.runTests(gPlayedTests, multiple_range_merge); >+// Play to create two ranges, in the revers order. check that they are sorted. >+manager.runTests(gPlayedTests, multiple_range_sort); MediaTestManager.runTests() is not synchronous. It also resets its tokens array when called, so every time it's called it will wipe out history of previously started tests. I'm guessing that's why you need long media files to ensure you don't get random failures. If you want to run multiple tests in a single MediaTestManager, you need to take an approach similar to how test_preload_actions.html does it, i.e. create an array of closures, and call MediaTestManager.runTests() once only.

Ok, I guess finally understood the point of MediaTestManager. The tests are now running using a single |runTests()| call, and are ran 4 times (wav audio, ogg audio, ogg video, webm video) each, to test all backends.

Comment on attachment 537774 [details] [diff] [review] Patch v6 Getting there. ;) The purpose of MediaTestManager is ultimately to make parallelism tunable in our media tests. This is so we can change the value of PARALLEL_TESTS in manifest.js to control how many tests are run at once. Each <audio> or <video> element (currently) requires 4 threads to run, and on Linux x86 each thread has 10MB virtual address space reserved for each thread stack. When there are a lot of media elements around, we can acutally run out of address space, and this has caused intermittent test failures on Linux x86. This is why many tests reset the elements src to "" and remove the element from the document; we're trying to destory the decoders and force them to be GC'd to release their memory. (To this end, can you set src="" in your finish_test() function? Note this may fire some events, so it may cause test failures if you've got listeners for those events, in which case don't bother.) To allow the parallelism to be tuned, only *one* test should be started each time the MediaTestManager calls your startTest() function. At the moment you're running four tests per startTest() call. You can achieve this by creating a test array with multiple entries "per test", which vary in what resource they're loading. e.g. something like: function createTestArray(tests) { var A = []; for (var i=0; i<tests.length; i++) { for (var k=0; k<gPlayedTests.length; k++) { var t = new Object(); t.setup = tests[i].setup; t.name = gPlayedTests[k].name; t.type = gPlayedTests[k].type; A.push(t); } } return A; } function startTest(test, token) { var elemType = /^audio/.test(test.type) ? "audio" : "video"; var element = document.createElement(elemType); element.src = test.name; element.token = token; test.setup(element); manager.started(token); } manager.runTests(createTestArray(tests, startTest)); Note that every time startTest() is called, it's passed a unique token, so you should call manager.started(token) exactly once and manager.finished(token) exactly once for each token (and thus test) that's run. ATM you're reusing each token passed to startTest() 4 times. Do you need to have a "size" field on some of the members of gPlayedTests? +// Play and seek to have to overlapping ranges. They should be merged, to a +// range spanning all the test audio file. In this test, and others, it's probably best to start playback in the "loadedmetadata" handler, just in case we manage to play through before the "loadedmetadata" or "timeupdate" handler have a chance to run (it could for really small files, or if the main thread is under heavy load). +// Play to create two ranges, in the revers order. check that they are sorted. "reverse order". Other than that, the tests themselves look good.

Patch v7, I've checked that the tokens are not reused, and fixed the other problem you mentioned.

Comment on attachment 537976 [details] [diff] [review] Patch v7 >diff --git a/content/media/test/test_played.html b/content/media/test/test_played.html >new file mode 100644 >--- /dev/null >+++ b/content/media/test/test_played.html >@@ -0,0 +1,209 @@ >+ >+var test = getPlayableVideo(gPlayedTests); test is unused? Otherwise looks good!

I pushed this to try, and the test passed on Linux, but failed on most OSX and Windows builds. (Mainly rounding errors, apparently. See <http://tbpl.mozilla.org/?tree=Try&rev=f6a0c427accd>.

I have fixed the rounding error issues, but a test keep failing on the try servers, only on OSX. I've checked today with a macbookpro, and it works quite nice, I've ran the test like 20 times, and it is always green. My only guess is that the system running the tests is under high cpu pressure, and the "timeupdate" events are delayed, and the audio stream ends. Again, I can't explain yet why it is happening only on macs, and only on the tryservers. In the process, I've fixed a bug which created zero-width ranges when seeking without playing, and added a test case for this update.

(In reply to comment #24) > Created attachment 540076 [details] [diff] [review] [review] > Patch v6 - Fixed a bug. > > I have fixed the rounding error issues, but a test keep failing on the try > servers, only on OSX. Not much point in r+ing and landing this if it's got a test failure. :( You can try setting DEBUG_TEST_LOOP_FOREVER = true in content/media/test/manifest.js, and running the test locally. Then since your test uses the MediaTestManager, it will loop forever until it fails. That can help when trying to reproduce/debug random failures. Running the test looping in multiple tabs will increase the CPU load and should reduce the amount of time required to repro the failure. Increasing PARALLEL_TESTS in manifest.js may reduce time to repro too. Of course all that won't help if the failure actually is platform specific, then you've only got the tryserver and to help you out. :( Maybe try pushing a patch with debug logging to tryserver? stdout is captured in the tinderbox log.

(In reply to comment #24) > Created attachment 540076 [details] [diff] [review] [review] > Patch v6 - Fixed a bug. > > I have fixed the rounding error issues, but a test keep failing on the try > servers, only on OSX. I can reproduce some failures on my old mac mini at the office, I'll see if I can figure out what's causing them in the next day or so.

Comment on attachment 540079 [details] [diff] [review] Patch v7 - Added a test about the bugfix >+// Play to create two ranges, in the reverse order. check that they are sorted. >+// The ranges created sould be as follow >+{ >+ setup : function (element) { >+ function end() { >+ element.pause(); >+ let p = element.played; >+ is(p.length, 2, "There should be two ranges"); >+ is(p.start(0), element.duration/6, "Start of first range should be the sixth of the duration"); >+ ok(p.end(0) > 3*element.duration/6, "End of first range should be greater than three times the sixth of the duration"); >+ is(p.start(1), 4 * element.duration/6, "Start of second range should be four times the sixth of the duration"); >+ ok(p.end(1) > 5*element.duration/6, "End of second range should be greater that five times the sixth of the duration"); >+ finish_test(element); >+ } The problem is here. By the time the timeupdate fires and you end the test, it's possible for playback to have played enough so that the two ranges have merged. So you'll have only 1 range, so the |p.start(1)| and |p.end(1)| calls will fail and throw an index out of bounds exception. You can't rely on timeupdate events to be processed in a timely fashion. The best you can do is check that p.length>=1, and check that the start of the first range is duration/6 and the end of the last range (p.end(p.length-1)) is >= 5*duration/6. You can't do the other checks reliably. I pushed that to TryServer, and it passed: http://tbpl.mozilla.org/?tree=Try&rev=5b9b486ceb40 See http://hg.mozilla.org/try/rev/5b9b486ceb40 for the exact changes I added, you'll need to incorporate those changes or something equivalent into your patch.

Comment on attachment 540076 [details] [diff] [review] Patch v6 - Fixed a bug. Matthew reviewed the original patch, so he should look at this updated one.

Here is the test patch updated. I've also fixed the caption which was not valid anymore. Anyway, Chris, thanks for helping me fixing that.

Can you please fix the new |if(foo)| instances? The coding style dictates |if (foo)|, so please stick to that. Also, merge the code fix included in the "test" patch into the patch containing the code changes please. For the first test using timeupdate (that plays half the file), a more reliable way to test this may be to seek to duration/2 first, play the last half of the file, then (when ended is fired) seek to the beginning and play the first half, then check that played reports a single range. In general, I'd prefer to have fewer tests that always test the expected behaviour, rather than many unreliable/timing dependent tests that only test the expected behaviour some of the time. The test code: + element.addEventListener('loadedmetadata', function() { + element.currentTime = element.duration / 5; + element.currentTime = 2 * element.duration / 5; + element.currentTime = 3 * element.duration / 5; + element.currentTime = 4 * element.duration / 5; + element.currentTime = 5 * element.duration / 5; + }, false); doesn't really work, because it's not waiting for one seek to complete before starting another. You need to start the next seek when a seeked event is fired. Also, please fix the typos: Whithout, boudaries, sould, repeteadly,

Comment on attachment 542137 [details] [diff] [review] Patch v9 +// Seek repeteadly without playing. No range should appear. One last typo.

Comment on attachment 542525 [details] [diff] [review] Patch v10 -- Typo fixed >+ element.curretTime = element.duration / 2; I fixed this typo and sent to try. I'll push it when I get the results.

Unfortunately, that fixed assignment threw. I've now removed the line entirely and will push to try again.

http://hg.mozilla.org/mozilla-central/rev/ed18b0cca283 http://hg.mozilla.org/mozilla-central/rev/29ec386d969f

Sorry, had to back it out in http://hg.mozilla.org/mozilla-central/rev/dd9a2ec82f68 for causing the same infinite assertions as in bug 635726 (which is why I mistakenly starred the first four failures as being that, before I finally realized it had to be something new).

While fixing bug 635726 I found another intermittent test failure in test_played: failed | element.played.length == 2 - got 1, expected 2 failed | [SimpleTest/SimpleTest.js, window.onerror] - An error occurred: uncaught exception: [Exception... "Index or size is negative or greater than the allowed amount" code: "1" nsresult: "0x80530001 (NS_ERROR_DOM_INDEX_SIZE_ERR)" location: "http://mochi.test:8888/tests/content/media/test/test_played.html?autorun=1&timeout=100000&logFile=c%3A%5CUsers%5Ccpearce%5Csrc%5Cmauve%5Cobjdir%5C_tests%5Ctesting%5Cmochitest.log&fileLevel=DEBUG&consoleLevel=DEBUG Line: 111"] at :0 This is in the "ended" handler for the "Play and seek to have two ranges, and check that, as well a boundaries" test. The line numbers are slightly different because I added some instrumentation. I reproduced this by setting DEBUG_TEST_LOOP_FOREVER=true in manifest.js, and running the test looping in 20 tabs. You'll want the fix for bug 635726 applied before you try fixing this, I'll post that fix later on tonight or tomorrow.

I can reproduce this, and I guess that we cannot test this reliably, it seems to be the same problem as before, we cannot be sure that |timeUpdate| will be fired in a timely fashion. Maybe we could detect when the event has not been fired on time, and do something like this : > element.addEventListener("ended", (function() { > if(element.played.length > 1) { > is(element.played.length, 2, "element.played.length == 2"); > var guess = element.played.end(0) + element.duration/100.0; > ok(rangeCheck(element.played.start(1), guess), "we should have seeked forward by one tenth of the duration"); > is(element.played.end(1), element.duration, "end of second range shall be the total duration"); > } > is(element.played.start(0), 0, "start of first range shall be 0"); > finish_test(element); > }), false); This way, we can test the behavior of the feature, but we are protected from random oranges.

That's probably good enough.

I pushed comment 41 modifications to try (http://tbpl.mozilla.org/?tree=Try&rev=e8e3ba577ba7).

Paul, AFAICT, there's no difference between attachment 542525 [details] [diff] [review] and <http://hg.mozilla.org/try/rev/f475d7fdf832>. I also don't see the code from comment 41 anywhere in the latter.

http://hg.mozilla.org/integration/mozilla-inbound/rev/e0aab5011b70 http://hg.mozilla.org/integration/mozilla-inbound/rev/0040f406ea6d Crossing fingers...

backed out due to intermittent crashes in the test added here http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla-Inbound/1312309712.1312311113.1630.gz http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla-Inbound/1312313596.1312317274.31012.gz http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla-Inbound/1312311857.1312313999.15592.gz http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla-Inbound/1312317423.1312319532.9605.gz more to come, but these may be enough to start investigation

I've rebased this patch, it does not seem to fail anymore. As it was reviewed quite a long time ago, I've asked for review again. Green try (apart from usual random orange) : https://tbpl.mozilla.org/?tree=Try&rev=d33ae585467c, with quite a few re-run to ensure correctness.

Comment on attachment 617555 [details] [diff] [review] Rebased feature patch. Review of attachment 617555 [details] [diff] [review]: ----------------------------------------------------------------- No major problems here, just some minor style fixes. I'd like to see it again before r+ing it though. ::: content/html/content/public/nsHTMLMediaElement.h @@ +733,5 @@ > > + // Range of time played. > + nsTimeRanges mPlayed; > + > + // Temporary variable for storing a time, when the stream starts to play Full stop at end of sentences please. ::: content/html/content/src/nsHTMLMediaElement.cpp @@ +1132,5 @@ > NS_IMETHODIMP nsHTMLMediaElement::SetCurrentTime(double aCurrentTime) > { > StopSuspendingAfterFirstFrame(); > > + if (mCurrentPlayRangeStart != -1) { May as well use -1.0 and 0.0 to be consistent here? @@ +1133,5 @@ > { > StopSuspendingAfterFirstFrame(); > > + if (mCurrentPlayRangeStart != -1) { > + double oldCurrentTime = 0; This is still the current time throughout its usage, so calling it "old" isn't quite right. I'd call it "rangeEndTime" or somesuch, to denote its purpose. @@ +1135,5 @@ > > + if (mCurrentPlayRangeStart != -1) { > + double oldCurrentTime = 0; > + GetCurrentTime(&oldCurrentTime); > + LOG(PR_LOG_DEBUG, ("Adding a range : [%f, %f]", mCurrentPlayRangeStart, oldCurrentTime)); Follow the convention of the other LOG calls, i.e. LOG(PR_LOG_DEBUG, "%p ....", this, ...). This enables us to distinguish what log message belongs to what element when we have multiple media elements running at the same time. I'd also say "Adding a 'played' range: "..., so it's clear what kind of range you're talking about here. @@ +1136,5 @@ > + if (mCurrentPlayRangeStart != -1) { > + double oldCurrentTime = 0; > + GetCurrentTime(&oldCurrentTime); > + LOG(PR_LOG_DEBUG, ("Adding a range : [%f, %f]", mCurrentPlayRangeStart, oldCurrentTime)); > + // Multiple seek without playing This also happens when you play and then seek, not only after multiple seeks without playing... Can you also end sentences with a full stop please? Thanks. @@ +1137,5 @@ > + double oldCurrentTime = 0; > + GetCurrentTime(&oldCurrentTime); > + LOG(PR_LOG_DEBUG, ("Adding a range : [%f, %f]", mCurrentPlayRangeStart, oldCurrentTime)); > + // Multiple seek without playing > + if (mCurrentPlayRangeStart != oldCurrentTime) {} around if statement blocks please, unless it's a trivial error return. @@ +1170,5 @@ > // event if it changes the playback position as a result of the seek. > LOG(PR_LOG_DEBUG, ("%p SetCurrentTime(%f) starting seek", this, aCurrentTime)); > nsresult rv = mDecoder->Seek(clampedTime); > > + // Start a new range at position we seeked to Full stop at end of sentences please. @@ +1226,5 @@ > + if (mCurrentPlayRangeStart != -1.0) { > + double now = 0.0; > + GetCurrentTime(&now); > + if (mCurrentPlayRangeStart != now) > + ranges->Add(mCurrentPlayRangeStart, now); {} around if statement bodies please. @@ +2827,5 @@ > QueueSelectResourceTask(); > } > > // A load was paused in the resource selection algorithm, waiting for > + // a new source child to be added, resume the resource selection algorithm. Good spotting. ::: content/html/content/src/nsTimeRanges.cpp @@ +82,5 @@ > } > > void > nsTimeRanges::Add(double aStart, double aEnd) { > mRanges.AppendElement(TimeRange(aStart,aEnd)); Can you change this so that if aStart > aEnd, we don't add the range? We should warn with NS_WARNING() when someone tries to do this. Thanks. @@ +96,5 @@ > + // This merges the intervals > + TimeRange current(mRanges[0]); > + for (PRUint32 i = 1; i < mRanges.Length(); i++) { > + if (current.mEnd >= mRanges[i].mStart) { > + current.mEnd = NS_MAX(current.mEnd, mRanges[i].mEnd); current.mEnd = mRanges[i].mEnd; (since we know that a range's end must be after its start thanks to the change to nsTimeRanges::Add() above.) ::: content/html/content/src/nsTimeRanges.h @@ +56,5 @@ > void Add(double aStart, double aEnd); > > + // See > + // http://www.whatwg.org/specs/web-apps/current-work/multipage/ > + // the-iframe-element.html#normalized-timeranges-object http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#normalized-timeranges-object @@ +71,5 @@ > }; > > + struct CompareTimeRanges > + { > + PRBool Equals(const TimeRange& tr1, const TimeRange& tr2) const aTr1, aTr2. Sorry, aOurSillyNamingConvention applies here too. @@ +83,5 @@ > + { > + return tr1.mStart < tr2.mStart; > + } > + }; > + Too many extra line breaks. ::: dom/interfaces/html/nsIDOMHTMLMediaElement.idl @@ +56,5 @@ > #undef GetCurrentTime > #endif > %} > > +[scriptable, uuid(11d560c7-4fc6-41e1-bb0e-de0255f1e8b1)] You also need to rev the uuids for elements which inherit from this one, so audio and video elements. There's a script to do this for you BTW: http://people.mozilla.org/~sfink/uploads/update-uuids @@ +90,5 @@ > attribute double currentTime; > readonly attribute double initialTime; > readonly attribute double duration; > readonly attribute boolean paused; > + readonly attribute nsIDOMTimeRanges played; Leave the linebreak in.

(In reply to Chris Pearce (:cpearce) from comment #53) > ::: content/html/content/src/nsTimeRanges.h > @@ +56,5 @@ > > void Add(double aStart, double aEnd); > > > > + // See > > + // http://www.whatwg.org/specs/web-apps/current-work/multipage/ > > + // the-iframe-element.html#normalized-timeranges-object > > http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video- > element.html#normalized-timeranges-object A more reliable link (that doesn't rely on where the multipage spec is split) would be <http://www.whatwg.org/html/#normalized-timeranges-object>.

Comment on attachment 618758 [details] [diff] [review] Addressed cpearce comments. Review of attachment 618758 [details] [diff] [review]: ----------------------------------------------------------------- Better, but I picked up a few nits again. Sorry, I should have caught them last time. ::: content/html/content/public/nsHTMLMediaElement.h @@ +733,5 @@ > > + // Range of time played. > + nsTimeRanges mPlayed; > + > + // Temporary variable for storing a time, when the stream starts to play. This isn't a "Temporary variable", in the classic sense if it was "temporary", it'd be a local variable right. How about: "Stores the time at the start of the current 'played' range." ::: content/html/content/src/nsTimeRanges.cpp @@ +83,5 @@ > > void > nsTimeRanges::Add(double aStart, double aEnd) { > + if (aStart < aEnd) { > + NS_WARNING("Can't add a range if the end is older that the start."); Return here, so we *don't* add the range if its end is after its start. ::: content/html/content/src/nsTimeRanges.h @@ +67,5 @@ > double mStart; > double mEnd; > }; > > + struct CompareTimeRanges Local brace convention seems to be: struct structName { type functionName() { } }; Rather than braces on their own lines. The style varies between files/modules, follow the local convention in each file when adding code. Also we use bool rather than PRBool now. @@ +74,5 @@ > + { > + return aTr1.mStart == aTr2.mStart && aTr1.mEnd == aTr2.mEnd; > + } > + > + // Here, we aim at time range normalization. That why we order only by start "Here, we aim at time range normalization."; this doesn't quite follow. Better to just add a comment at the start of the class describing what the class does, and who uses it, e.g.: "Comparator which orders TimeRanges by start time. Used by Normalize()."

Comment on attachment 618759 [details] [diff] [review] Tests Review of attachment 618759 [details] [diff] [review]: ----------------------------------------------------------------- It seems one of the tests has a spelling mistake which is the only thing preventing it from failing. Yay dynamic languages. ::: content/html/content/src/nsTimeRanges.cpp @@ +82,5 @@ > } > > void > nsTimeRanges::Add(double aStart, double aEnd) { > + if (aStart > aEnd) { This should be corrected in the previous patch, along with returning here instead of adding the incorrect range. ::: content/media/test/manifest.js @@ +27,5 @@ > +var gPlayedTests = [ > + { name:"big.wav", type:"audio/x-wav", duration:9.0, size:102444 }, > + { name:"sound.ogg", type:"audio/ogg", duration:4.0, size:2603}, > + { name:"seek.ogv", type:"video/ogg", duration:3.966, size:285310}, > + { name:"seek.webm", type:"video/webm", duration:3.966 }, Why does this not have a size when the others do? Probably none of them need sizes? ::: content/media/test/test_played.html @@ +52,5 @@ > +// Play the first half of the file, pause, play > +// an check we have only one range. > +{ > + setup : function (element) { > + let onEnded = function() { Aren't you playing the second half of the file through, then trying to replay the file again? i.e. the comment describing this test doesn't match reality? Oh wait, you misspelt element.curretTime below, so you're not seeking to duration/2. You'd better fix this test. Also, assuming you fix the spelling mistake, won't check_full_file_played's ended handler will run as well as onEnded on the first play through? Therefore, won't check_full_file_played's ended handler will finish the test, after only playing half the media, and nothing onEnded does will have any affect on the test? But check_full_file_played's ended handler checks that the entire media has played, but only half of it will have playes... How can this test possibly pass? @@ +113,5 @@ > + element.play(); > + } > +}, > + > +// Play and seek to have to overlapping ranges. They should be merged, to a Isn't this test the same as your " Play the first half of the file, seek back, while // continuing to play. We shall have only one range." test, except that your dividing element.duration by 3 instead of 4?? @@ +226,5 @@ > + test.setup(element); > + manager.started(token); > +} > + > +manager.runTests(createTestArray(tests), startTest); You don't need to pass tests here right? It's already declared in this scope?

> ::: content/media/test/test_played.html > @@ +52,5 @@ > > +// Play the first half of the file, pause, play > > +// an check we have only one range. > > +{ > > + setup : function (element) { > > + let onEnded = function() { > > Aren't you playing the second half of the file through, then trying to > replay the file again? i.e. the comment describing this test doesn't match > reality? Oh wait, you misspelt element.curretTime below, so you're not > seeking to duration/2. > > You'd better fix this test. > > Also, assuming you fix the spelling mistake, won't check_full_file_played's > ended handler will run as well as onEnded on the first play through? > Therefore, won't check_full_file_played's ended handler will finish the > test, after only playing half the media, and nothing onEnded does will have > any affect on the test? But check_full_file_played's ended handler checks > that the entire media has played, but only half of it will have playes... > How can this test possibly pass? The comment was indeed incorrect. The behavior that is tested in this particular test is : - Go to the half of the file upon loading ; - Play until the end ; - Go to the beginning ; - Play until the end ; - Check that we have only one range. At the moment, it was playing the file in its entirety twice, hence the test pass. If I fix only the typo, I get a failure for each media, which is logical. > @@ +113,5 @@ > > + element.play(); > > + } > > +}, > > + > > +// Play and seek to have to overlapping ranges. They should be merged, to a > > Isn't this test the same as your " Play the first half of the file, seek > back, while // continuing to play. We shall have only one range." test, > except that your dividing element.duration by 3 instead of 4?? Absolutely, I've removed it this test.

Comment on attachment 619115 [details] [diff] [review] Addressed cpearce comments. Review of attachment 619115 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, looks good!

Comment on attachment 619117 [details] [diff] [review] Tests Review of attachment 619117 [details] [diff] [review]: ----------------------------------------------------------------- Nice. Thanks!

Comment on attachment 619115 [details] [diff] [review] Addressed cpearce comments. Review of attachment 619115 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/html/content/public/nsHTMLMediaElement.h @@ +731,5 @@ > // True if the sound is muted > bool mMuted; > > + // Range of time played. > + nsTimeRanges mPlayed; This sucks for packing. Please put the two new members above the booleans. ::: content/html/content/src/nsHTMLMediaElement.cpp @@ +1208,5 @@ > return NS_OK; > } > > +/* readonly attribute nsIDOMHTMLTimeRanges played; */ > +NS_IMETHODIMP nsHTMLMediaElement::GetPlayed(nsIDOMTimeRanges * *aPlayed) nsIDOMTimeRanges** aPlayed ::: content/html/content/src/nsTimeRanges.cpp @@ +90,5 @@ > mRanges.AppendElement(TimeRange(aStart,aEnd)); > } > + > +void > +nsTimeRanges::Normalize() { { on the next line (yes, I know this is wrong in surrounding code too).

I suppose there is no need for a review this time.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f9ab94a0e23 https://hg.mozilla.org/integration/mozilla-inbound/rev/938757040622

Docs updated: https://developer.mozilla.org/en/DOM/HTMLMediaElement https://developer.mozilla.org/en/HTML/Element/Video https://developer.mozilla.org/en/HTML/Element/audio Listed on Firefox 15 for developers.