Last Comment Bug 462960 - Implement nsIDOMHTMLMediaElement::GetSeekable()
: Implement nsIDOMHTMLMediaElement::GetSeekable()
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: mozilla8
Assigned To: Paul Adenot (:padenot) (in PTO until early September)
:
Mentors:
Depends on: 619752 667929 751539
Blocks: video
  Show dependency treegraph
 
Reported: 2008-11-03 17:07 PST by Chris Pearce (:cpearce)
Modified: 2012-05-03 05:42 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v0 -- Add seekable member. (3.21 KB, patch)
2011-06-27 10:21 PDT, Paul Adenot (:padenot) (in PTO until early September)
no flags Details | Diff | Splinter Review
Patch v0 -- Testing seekable member (7.60 KB, patch)
2011-06-27 10:23 PDT, Paul Adenot (:padenot) (in PTO until early September)
no flags Details | Diff | Splinter Review
Patch v1 -- Implements point 7 of seeking in spec + moved core code to the decoder (8.28 KB, patch)
2011-06-28 11:08 PDT, Paul Adenot (:padenot) (in PTO until early September)
no flags Details | Diff | Splinter Review
Patch v1 - Updated test to reflect the changes in the implementation of the feature (7.62 KB, patch)
2011-06-28 11:10 PDT, Paul Adenot (:padenot) (in PTO until early September)
no flags Details | Diff | Splinter Review
Patch v2 - addressed chris' remarks. (11.60 KB, patch)
2011-06-29 08:05 PDT, Paul Adenot (:padenot) (in PTO until early September)
no flags Details | Diff | Splinter Review
Patch v1 -- removed todos (8.18 KB, patch)
2011-06-29 08:12 PDT, Paul Adenot (:padenot) (in PTO until early September)
no flags Details | Diff | Splinter Review
Patch v3 -- Adressed chris' remarks (11.40 KB, patch)
2011-06-30 10:10 PDT, Paul Adenot (:padenot) (in PTO until early September)
no flags Details | Diff | Splinter Review
Patch v2 -- Moved range_equals to the manifest.js file (8.71 KB, patch)
2011-06-30 10:24 PDT, Paul Adenot (:padenot) (in PTO until early September)
cpearce: review+
Details | Diff | Splinter Review
Patch v3 -- Adressed chris' remarks (good file). (11.40 KB, patch)
2011-07-01 02:53 PDT, Paul Adenot (:padenot) (in PTO until early September)
no flags Details | Diff | Splinter Review
Patch v3 - typos + comments + bugfix (12.65 KB, patch)
2011-07-04 04:53 PDT, Paul Adenot (:padenot) (in PTO until early September)
cpearce: review+
Details | Diff | Splinter Review
Patch v4 -- Last nit. (12.66 KB, patch)
2011-07-05 03:07 PDT, Paul Adenot (:padenot) (in PTO until early September)
cpearce: review+
Details | Diff | Splinter Review
Unified and cleaned patch. Implements .seekable member + test + step 7 of seeking algo (20.70 KB, patch)
2011-08-08 09:16 PDT, Paul Adenot (:padenot) (in PTO until early September)
no flags Details | Diff | Splinter Review

Description Chris Pearce (:cpearce) 2008-11-03 17:07:03 PST

    
Comment 1 Chris Pearce (:cpearce) 2008-11-03 18:11:10 PST
You'll need a nsIDOMHTMLTimeRanges interface to implement this. Stubs for that are removed in patch for bug 462953.
Comment 2 Paul Adenot (:padenot) (in PTO until early September) 2011-05-25 06:04:39 PDT
I need the initialTime member to implement this, but I guess I can write a "good enough" implementation using initialTime == 0.0. I'll may create a separate bug for a proper initialTime implementation.
Comment 3 cajbir (:cajbir) 2011-05-25 06:23:22 PDT
Bug 648595 adds initialTime as part of the support for media fragments.
Comment 4 Paul Adenot (:padenot) (in PTO until early September) 2011-06-27 10:21:28 PDT
Created attachment 542191 [details] [diff] [review]
Patch v0 -- Add seekable member.

Note : One need to apply bug 619752 patch for this implementation to work, since this patch relies on the duration member value.
Comment 5 Paul Adenot (:padenot) (in PTO until early September) 2011-06-27 10:23:00 PDT
Created attachment 542193 [details] [diff] [review]
Patch v0 -- Testing seekable member

Here is a test proposal, using .sjs files to test different server-side configuration (e.g. no range requests or no content-length).
Comment 6 Chris Pearce (:cpearce) 2011-06-27 18:04:15 PDT
Comment on attachment 542191 [details] [diff] [review]
Patch v0 -- Add seekable member.

>+#include "nsIResumableChannel.h"

Do you need this?

>+/* readonly attribute nsIDOMHTMLTimeRanges seekable; */
>+NS_IMETHODIMP nsHTMLMediaElement::GetSeekable(nsIDOMTimeRanges * *aSeekable)
>+{

Return an empty range if mReadyState == HAVE_NOTHING.

>+  nsTimeRanges* ranges = new nsTimeRanges();
>+  NS_ADDREF(*aSeekable = ranges);
>+  double duration = 0.0;
>+  GetDuration(&duration);
>+  //TODO : change 0.0 to GetInitialTime() when available
>+  double initialTime = 0.0;
>+
>+  if (mDecoder && mDecoder->GetSeekable()) {
>+    if (mDecoder->IsInfinite()) {
>+      ranges->Add(initialTime, NS_IEEEPositiveInfinity());
>+      return NS_OK;
>+    }
>+    ranges->Add(initialTime, initialTime + duration);
>+    return NS_OK;
>+  }
>+  if (mDecoder && mDecoder->IsInfinite()) {
>+    return NS_OK;

We can still safely seek inside the buffered ranges in this case, so no need to return here.

In the case of an unseekable resource, the media cache tries it's hardest to not throw away data after the current playback position, so in most cases we should be able to seek around and then seek back into the "live" range without the "live" range's download stopping.

>+  }
>+
>+  GetBuffered(aSeekable);

GetBuffered allocates another nsTimeRanges and assigns it to *aSeekable, so you'll leak |ranges| here. You can call mDecoder->GetBuffered(ranges) directly here to avoid the leak.

If you run firefox (or mochitests) with the environment variable XPCOM_MEM_LEAK_LOG=2 set, you should get a log of leaked objects on exit.

Can you also implement step 7 in the W3 seeking specification: http://www.w3.org/TR/html5/video.html#seeking ?

This should be as easy as calling your GetSeekable() implementation in nsBuiltinDecoderStateMachine::Seek(double), and using that to sanitize the value of aTime so that it's the nearest value in the seekable ranges. If seekable is empty, just return.

I'd prefer to put the guts of GetSeekable() into nsBuiltinDecoder. It seems wrong for nsBuiltinDecoderStateMachine::Seek to have to ask the element for decoder-centric data like this.
Comment 7 Paul Adenot (:padenot) (in PTO until early September) 2011-06-28 11:08:49 PDT
Created attachment 542522 [details] [diff] [review]
Patch v1 -- Implements point 7 of seeking in spec + moved core code to the decoder

Here is the updated version of the patch, implementing point 7 of the seeking spec, and moving the code to nsBuiltinDecoder.

Anyway, this feature relies on the buffered member, thus it doesn't work quite well on ogg for the moment (bug 667929).
Comment 8 Paul Adenot (:padenot) (in PTO until early September) 2011-06-28 11:10:13 PDT
Created attachment 542524 [details] [diff] [review]
Patch v1 - Updated test to reflect the changes in the implementation of the feature

I've added todos when the tests are failing due to the broken buffered member.
Comment 9 Chris Pearce (:cpearce) 2011-06-28 21:23:07 PDT
Comment on attachment 542522 [details] [diff] [review]
Patch v1 -- Implements point 7 of seeking in spec + moved core code to the decoder

Can you rename |PRBool nsMediaDecoder GetSeekable()| and friends to IsSeekable() so that we can distinguish it from GetSeekable(nsTimeRanges*) easier? Thanks.

>+/* readonly attribute nsIDOMHTMLTimeRanges seekable; */
>+NS_IMETHODIMP nsHTMLMediaElement::GetSeekable(nsIDOMTimeRanges * *aSeekable)

NS_IMETHODIMP nsHTMLMediaElement::GetSeekable(nsIDOMTimeRanges** aSeekable)


>+{
>+  nsTimeRanges* ranges = new nsTimeRanges();
>+  NS_ADDREF(*aSeekable = ranges);
>+
>+  if(mReadyState > nsIDOMHTMLMediaElement::HAVE_NOTHING) {

if (...) 

Always put a space between "if" and "(".

Also return here if !mDecoder, else you'll crash if mDecoder is null.

>+PRBool
>+nsTimeRanges::IsInRanges(double aValue, PRUint32* aIntervalIndex) {
>+  for (PRUint32 i = 0; i < mRanges.Length(); i++) {
>+    if (mRanges[i].mStart <= aValue && aValue <= mRanges[i].mEnd) {
>+      if (aIntervalIndex != nsnull) {
>+        *aIntervalIndex = i;
>+      }
>+      return PR_TRUE;
>+    }
>+  }
>+  return PR_FALSE;
>+}

Because this is only used in one place, and it only uses data which is publically exposed anyway, there's no need to add this method to nsTimeRanges. Just add it as a static function in nsBuiltinDecoder.cpp.


>diff --git a/content/media/nsBuiltinDecoder.cpp b/content/media/nsBuiltinDecoder.cpp
>--- a/content/media/nsBuiltinDecoder.cpp
>+++ b/content/media/nsBuiltinDecoder.cpp
>@@ -292,16 +292,54 @@ nsresult nsBuiltinDecoder::Play()
> nsresult nsBuiltinDecoder::Seek(double aTime)
> {
>   NS_ASSERTION(NS_IsMainThread(), "Should be on main thread.");
>   ReentrantMonitorAutoEnter mon(mReentrantMonitor);
> 
>   if (aTime < 0.0)
>     return NS_ERROR_FAILURE;

While you're here, can you change this to NS_ABORT_IF_FALSE(aTime >= 0.0, "Some useful error message") instead? nsHTMLMediaElement::SetCurrentTime() ensures that aTime is >= 0.

> 
>+  nsTimeRanges seekable;
>+  PRUint32 length = 0;
>+  GetSeekable(&seekable);

Check return value of GetSeekable(nsTimeRanges), and return NS_OK if GetSeekable() failed (our return value will return all the way up to JS, and we don't want JS throwing an exception in this case).

>+  seekable.GetLength(&length);
>+  if (!length) {
>+    return NS_OK;
>+  }
>+
>+  PRUint32 range = 0;
>+  if (!seekable.IsInRanges(aTime, &range)) {

|range| is not set when IsInRanges() returns PR_FALSE, so this code would end up assuming the range before the seek target is range 0.

I'd change IsInRanges() to loop over the contents of seekable until either you find the range containing aTime, or returning once you reach the first range which starts after aTime. You should return the last range which starts before aTime (or store it in *aIntervalIndex).

It sort of looks like you were intending to do that anyway?

>+    nsresult res;
>+    double leftBound, rightBound;
>+    res = seekable.Start(range, &leftBound);

Assuming |range| is the index of the range prior to aTime, shouldn't this be calling End(), not Start() ?

>+    NS_ENSURE_SUCCESS(res, 0);

Return NS_OK, not 0.

>+    double distanceLeft = NS_ABS(leftBound - aTime);
>+
>+    double distanceRight = -1;
>+    if (range < length) {

How can this be false? Did you mean (range + 1 < length) ?

>+      res = seekable.End(range+1, &rightBound);

Assuming |range| is the index of the range prior to aTime, shouldn't this be calling Start(), not End() ?



>+    if (distanceLeft != distanceRight || distanceRight == -1) {
>+      if (distanceLeft > distanceRight) {
>+        aTime = leftBound;
>+      } else {
>+        aTime = rightBound;
>+      }
>+    } else {
>+      if (NS_ABS(distanceLeft - mCurrentTime) < NS_ABS(distanceRight - mCurrentTime)) {
>+        aTime = leftBound;
>+      } else {
>+        aTime = rightBound;
>+      }
>+    }

To save lines and branches you could write this block as:

      if (distanceLeft == distanceRight) {
        distanceLeft = NS_ABS(leftBound - mCurrentTime);
        distanceRight = NS_ABS(rightBound - mCurrentTime);
      } 
      aTime = (distanceLeft < distanceRight) ? leftBound : rightBound;

      


>+nsresult nsBuiltinDecoder::GetSeekable(nsTimeRanges* aSeekable)
>+{
>+  //TODO : change 0.0 to GetInitialTime() when available
>+  double initialTime = 0.0;
>+
>+  if (GetSeekable()) {
>+    if (IsInfinite()) {
>+      aSeekable->Add(initialTime, std::numeric_limits<double>::infinity());
>+      return NS_OK;
>+    }
>+
>+    aSeekable->Add(initialTime, initialTime + GetDuration());
>+    return NS_OK;

To make the code less "branchy", I'd structure that as:

    double end = IsInfinite() ? std::numeric_limits<double>::infinity()
                              : initialTime + GetDuration();
    aSeekable->Add(initialTime, end);
    return NS_OK;                          


>+  }
>+
>+  GetBuffered(aSeekable);
>+  return NS_OK;

GetBuffered() can fail, so instead of ignoring the return value, |return GetBuffered(seekable)|.

>diff --git a/content/media/nsMediaDecoder.h b/content/media/nsMediaDecoder.h
>[...]
>+  // Return the time range that can be seeked to.
>+  virtual nsresult GetSeekable(nsTimeRanges* aSeekable) = 0;

Return the time ranges that can be seeked into.
Comment 10 Chris Pearce (:cpearce) 2011-06-28 21:40:56 PDT
(In reply to comment #8)
> Created attachment 542524 [details] [diff] [review] [review]
> Patch v1 - Updated test to reflect the changes in the implementation of the
> feature
> 
> I've added todos when the tests are failing due to the broken buffered
> member.

They could also fail if the entire media loads before loadedmetadata fire. You're probably safer comparing |seekable| with |buffered| in these cases?
Comment 11 Paul Adenot (:padenot) (in PTO until early September) 2011-06-29 08:05:41 PDT
Created attachment 542822 [details] [diff] [review]
Patch v2 - addressed chris' remarks.

Here is the corrected code. I wonder why it seemed to work, considering all the things that were wrong...
Comment 12 Paul Adenot (:padenot) (in PTO until early September) 2011-06-29 08:12:05 PDT
Created attachment 542825 [details] [diff] [review]
Patch v1 -- removed todos

I wonder if it would be acceptable to move range_equals() into some kind of header file (manifest.js ?), to avoid duplication ? I'm going to try to merge the tests, anyway, that should take care of the problem.
Comment 13 Chris Pearce (:cpearce) 2011-06-29 15:19:53 PDT
Comment on attachment 542822 [details] [diff] [review]
Patch v2 - addressed chris' remarks.

>diff --git a/content/media/nsBuiltinDecoder.cpp b/content/media/nsBuiltinDecoder.cpp
>+static PRBool IsInRanges(nsTimeRanges& ranges, double aValue, PRUint32* aIntervalIndex) {

Nit: function/method arguments should be prefixed with "a", so |ranges| should be named |aRanges| unfortunately.

>+  nsresult rv;
>+  PRUint32 length;
>+  rv = ranges.GetLength(&length);

This actually can't fail, so there's no point storing (or checking) the return value.

>+  for (PRUint32 i = 0; i < length; i++) {
>+    double start, end;
>+    rv = ranges.Start(i, &start);
>+    NS_ENSURE_SUCCESS(rv, 0);

You should be returning NS_* values here rather than ints, because it means readers of the code don't need to remember what 0 means in this context. You've done this a few times.

Note if this fails, the caller would still assume that *aIntervalIndex contains a valid value, which would be bad. However Start() (and End()) only fail if i >= length, but that can't happen because of the loop condition. So provided you don't change the loop condition, you can just remove the return value check for Start() and End(). (We're needlessly doing this check elsewhere, but oh well).


>+    if (start > aValue) {
>+      if (aIntervalIndex != nsnull) {
>+        *aIntervalIndex = i - 1;

What if i == 0? This will overflow then.

You'll need to figure out how to handle this case.

>+      }
>+      return PR_FALSE;
>+    }
>+    rv = ranges.End(i, &end);
>+    NS_ENSURE_SUCCESS(rv, 0);
>+    if (start <= aValue && aValue <= end) {

If start > aValue, we will have returned earlier in the loop, so no need to check if start > aValue.


>+      if (aIntervalIndex != nsnull) {
>+        *aIntervalIndex = i;
>+      }
>+      return PR_TRUE;
>+    }
>+  }
>+  return PR_FALSE;

If we get here, *aIntervalIndex won't have a valid value either. This can happen if |ranges| contains only ranges which end before aValue.

You'll need to figure out how to handle this case.

>+}
>+
>+

Nit: Only 1 newline between functions please.

> nsresult nsBuiltinDecoder::Seek(double aTime)
> {
>   NS_ASSERTION(NS_IsMainThread(), "Should be on main thread.");
>   ReentrantMonitorAutoEnter mon(mReentrantMonitor);
> 
>-  if (aTime < 0.0)
>-    return NS_ERROR_FAILURE;
>+  NS_ABORT_IF_FALSE(aTime >= 0.0, "Cannot seek to a negative value.");
>+
>+  nsTimeRanges seekable;
>+  nsresult res;
>+  PRUint32 length = 0;
>+  res = GetSeekable(&seekable);
>+  if (res) {
>+    return NS_OK;
>+  }

Failure is unexpected here, so use NS_ENSURE_SUCCESS(res, NS_OK).

Use NS_ENSURE_* in cases where failure is unexpected (it prints a warning). Use |if (NS_FAILED(res)) {...}| in cases where failure is expected.

Don't use |if (res) {...}|, it's not as obvious as NS_FAILED(res) and friends.

>+  seekable.GetLength(&length);
>+  if (!length) {
>+    return NS_OK;
>+  }

This is ok, because length==0 is possible (though rare!) and not catastrophic in this case.
Comment 14 Chris Pearce (:cpearce) 2011-06-29 15:44:10 PDT
Comment on attachment 542825 [details] [diff] [review]
Patch v1 -- removed todos

(In reply to comment #12)
> Created attachment 542825 [details] [diff] [review] [review]
> Patch v1 -- removed todos
> 
> I wonder if it would be acceptable to move range_equals() into some kind of
> header file (manifest.js ?), to avoid duplication ?  I'm going to try to
> merge the tests, anyway, that should take care of the problem.

Avoiding duplication is an excellent idea. Go ahead and move range_equals() into manifest.js if you decide not to merge the tests.

If you don't merge the tests, in test_seekable1.html there's no point in passing |tests| to |createTestArray()|, since it contains only 1 element.

Uses spaces instead of tabs, tab width 2 spaces please.

>diff --git a/content/media/test/test_seekable2.html b/content/media/test/test_seekable2.html
[...]
>+function on_metadataloaded() {
>+  var v = document.getElementById('v');
>+	ok(v.seekable.length ==  0 || range_equals(v.seekable, v.buffered), "seekable.length should be initialy empty, or equal to buffered");

Can seekable.length be 0 when buffered.length is non 0? Shouldn't seekable == buffered in the non-seekable case? Same question for test_seekable3.html as well.
Comment 15 Paul Adenot (:padenot) (in PTO until early September) 2011-06-30 10:10:12 PDT
Created attachment 543171 [details] [diff] [review]
Patch v3 -- Adressed chris' remarks
Comment 16 Paul Adenot (:padenot) (in PTO until early September) 2011-06-30 10:24:40 PDT
Created attachment 543174 [details] [diff] [review]
Patch v2 -- Moved range_equals to the manifest.js file

Chris, indeed such a situation cannot happen in those tests, since this is the behavior we want to test (i.e. if data is buffered, we can play it, so we should be able to seek to it). I blindly kept this test while adding the range equality test.
Comment 17 Chris Pearce (:cpearce) 2011-06-30 15:34:36 PDT
(In reply to comment #15)
> Created attachment 543171 [details] [diff] [review] [review]
> Patch v3 -- Adressed chris' remarks

Wrong patch? ;)
Comment 18 Paul Adenot (:padenot) (in PTO until early September) 2011-07-01 02:53:05 PDT
Created attachment 543383 [details] [diff] [review]
Patch v3 -- Adressed chris' remarks (good file).

Indeed :)
Comment 19 Chris Pearce (:cpearce) 2011-07-03 15:03:57 PDT
Comment on attachment 543383 [details] [diff] [review]
Patch v3 -- Adressed chris' remarks (good file).

>+/**
>+ * Returns PR_TRUE if aValue is inside a range of aRanges, and put the range
>+ * index in aIntervalIndex if it is not null.
>+ * If aValue is not inside a range, PR_FALSE is returned, and aIntervalIndex, if
>+ * not null, is set to the index of the range which end immediatly before aValue

s/end immediatly/ends immediately/ 

>+ * (and can be -1 if aValue is before aRanges.Start(0)).
>+ */
>+static PRBool IsInRanges(nsTimeRanges& aRanges, double aValue, PRInt32* aIntervalIndex) {

Pass aIntervalIndex by reference, then the compiler enforces that it can't be null for you.


> nsresult nsBuiltinDecoder::Seek(double aTime)
> {
>   NS_ASSERTION(NS_IsMainThread(), "Should be on main thread.");
>   ReentrantMonitorAutoEnter mon(mReentrantMonitor);
> 
>-  if (aTime < 0.0)
>-    return NS_ERROR_FAILURE;
>+  NS_ABORT_IF_FALSE(aTime >= 0.0, "Cannot seek to a negative value.");
>+
>+  nsTimeRanges seekable;
>+  nsresult res;
>+  PRUint32 length = 0;
>+  res = GetSeekable(&seekable);
>+  NS_ENSURE_SUCCESS(res, NS_OK);
>+
>+  seekable.GetLength(&length);
>+  if (!length) {
>+    return NS_OK;
>+  }
>+

Add a comment here explaining what we're doing here.

>+  PRInt32 range = 0;
>+  if (!IsInRanges(seekable, aTime, &range)) {
>+    if (range != -1) {
>+      double leftBound, rightBound;
>+      res = seekable.End(range, &leftBound);
>+      NS_ENSURE_SUCCESS(res, 0);

NS_ENSURE_SUCCESS(res, NS_OK);

>+      double distanceLeft = NS_ABS(leftBound - aTime);
>+
>+      double distanceRight = -1;
>+      if (range + 1 < length) {
>+        res = seekable.Start(range+1, &rightBound);
>+        NS_ENSURE_SUCCESS(res, NS_OK);
>+        distanceRight = NS_ABS(rightBound - aTime);
>+      }
>+
>+      if (distanceLeft == distanceRight) {
>+        distanceLeft = NS_ABS(leftBound - mCurrentTime);
>+        distanceRight = NS_ABS(rightBound - mCurrentTime);
>+      } 
>+      aTime = (distanceLeft < distanceRight) ? leftBound : rightBound;
>+    } else {
>+      seekable.Start(0, &aTime);

Shouldn't this be calling End() not Start()?

Since it's not obvious what case we're handling here, please add a comment explaining why we're doing this.


>+  // Return the time range that can be seeked into.

s/range/ranges/
Comment 20 Paul Adenot (:padenot) (in PTO until early September) 2011-07-04 04:53:09 PDT
Created attachment 543745 [details] [diff] [review]
Patch v3 - typos + comments + bugfix

> Shouldn't this be calling End() not Start()?

If range == -1, aTime is before the first seekable range, so we want the Start() of the first range. I've added a comment to clarify this part.
Comment 21 Chris Pearce (:cpearce) 2011-07-04 11:13:03 PDT
Comment on attachment 543745 [details] [diff] [review]
Patch v3 - typos + comments + bugfix

Nit; please change:

+  // If the position we want to seek to is not in a seekable range, we seek
+  // to the closest position in the seekable ranges instead . If two positions
+  // matches, we seek to the closest position from the currentTime.

to:

+  // If the position we want to seek to is not in a seekable range, we seek
+  // to the closest position in the seekable ranges instead. If two positions
+  // are equally close, we seek to the position closest to the currentTime.

r+ with that change. Looks good.
Comment 22 Paul Adenot (:padenot) (in PTO until early September) 2011-07-05 03:07:40 PDT
Created attachment 543902 [details] [diff] [review]
Patch v4 -- Last nit.
Comment 23 Chris Pearce (:cpearce) 2011-07-05 15:07:28 PDT
Comment on attachment 543902 [details] [diff] [review]
Patch v4 -- Last nit.

Review of attachment 543902 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 24 Chris Pearce (:cpearce) 2011-07-20 15:04:20 PDT
Comment on attachment 543174 [details] [diff] [review]
Patch v2 -- Moved range_equals to the manifest.js file

Looks good. Sorry for the delay, I must have lost this review request somehow. If in future I (or someone else for that matter) doesn't respond to a review request in a week or so, you should email them or comment to ensure they've not forgotten.

Can this land now, or must bug 619752 land first?
Comment 25 Paul Adenot (:padenot) (in PTO until early September) 2011-07-21 09:06:57 PDT
bug 619752 must indeed land first.

I've pushed several patches to try (http://tbpl.mozilla.org/?tree=Try&rev=4215ba20177f), and refactored the patches for them to be appliable without the patches for bug 462959.
Comment 26 Dão Gottwald [:dao] 2011-08-06 08:13:26 PDT
I see two patches, v2 and v4. I assume both need to land? v2 apparently adds tests for nsIDOMHTMLMediaElement.seekable. v4, according to its commit message (which contains random asterisks but lacks the reviewer), implements nsIDOMHTMLMediaElement.played, but as far as I can tell, that's a lie. If it implements nsIDOMHTMLMediaElement.seekable as this bug's summary indicates, there's no reason for it to be separate from the tests -- a unified patch with a cleaned up commit message would be appreciated.
Comment 27 Paul Adenot (:padenot) (in PTO until early September) 2011-08-08 09:16:52 PDT
Created attachment 551476 [details] [diff] [review]
Unified and cleaned patch. Implements .seekable member + test + step 7 of seeking algo

Here is a cleaned and unified patch, which applies and tests fine against last tip. I've fixed the commit message.
Comment 29 Eric Shepherd [:sheppy] 2011-08-24 07:56:10 PDT
Documentation updated:

https://developer.mozilla.org/en/DOM/HTMLMediaElement

Added info here:

https://developer.mozilla.org/en/Using_HTML5_audio_and_video#Seeking_through_media

An example needs to be added to the previous article, but otherwise this is done.

Also listed on Firefox 8 for developers.
Comment 30 :Ehsan Akhgari 2012-05-02 21:21:55 PDT
Tests: https://hg.mozilla.org/mozilla-central/rev/938757040622

Note You need to log in before you can comment on or make changes to this bug.