479859 - Implement new new load algorithm

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P2)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://www.whatwg.org/specs/web-apps/current-work/#loading-the-media-resource
has changed again. Since it's in response to our feedback, we'd better change to suit.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v1

Patch v1, includes mochitest.

* Updated load to new load() algorithm.
* Adds nsIDOMRange to point to <source> child we're iterating over during load.
* Added cycle collection to nsHTMLMediaElement, as source pointer adds cycle.
* Removed nsMediaLoad, we don't need to store a list of candidates anymore, so I just replaced it with an int counter.
* Removed old tests which tested edge cases of previous load algorithm.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  enum LoadWaitStatus {

Maybe call this "LoadAlgorithmState"? Since we're not necessarily actually waiting.

+  // PR_TRUE if we're delaying the "load" event. They are delayed until either
+  // an error occurs, or readyState >= HAVE_METADATA and networkState == LOADED.
+  PRPackedBool mDelayingLoadEvent;

This is currently write-only; I suggest just taking it out until we use it.

AbortExistingLoads can only return false now, right? May as well remove the return value and maybe pull the code up to Load().

+  if (aNotify &&
+      aNameSpaceID == kNameSpaceID_None &&
+      aName == nsGkAtoms::src &&
+      mLoadWaitStatus == WAITING_FOR_SRC_OR_SOURCE) 
+  {
+    // A previous load algorithm instance is waiting on a src
+    // addition, resume the load. It is waiting at "step 1 of the load
+    // algorithm".
+    mLoadWaitStatus = NOT_WAITING;
+    QueueLoadTask();

Shouldn't this be QueueSelectResourceTask? I'm not sure if it's possible to observe a difference though.

+    nsCOMPtr<nsINode> startNode = do_QueryInterface(startContainer);
+    if (!startNode) return nsnull;

Why do you need startNode? You already know that it's 'this', and 'this' inherits from nsINode.

+    NS_ASSERTION(startOffset<GetChildCount(), "Offset or child count wrong!");

Spaces around <

+    }
+
+  }

Spurious blank line.

+++ b/content/html/content/src/nsHTMLVideoElement.cpp
@@ -67,6 +67,9 @@
 #include "nsIDOMDocumentEvent.h"
 #include "nsIDOMProgressEvent.h"
 #include "nsHTMLMediaError.h"
+#include "nsCycleCollectionParticipant.h"
+#include "nsISupportsImpl.h"

Why?

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v2

As patch v1, but addresses review comments.
* AbortExistingLoads() returns void. I didn't pull it up into Load(), as its used in LoadWithChannel() as well.
* I changed the loop in nsHTMLMediaElement::GetNextSource() as well. I think it's more correct now.
* Video mochitests pass on Windows and Linux tip.
* To behave correctly, this really needs the patch from bug 480889.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    if (startContainer != thisDomNode) {
+      // Start container is not the media element, so it's a descendant, but
+      // not a direct child of the media element. We only consider direct
+      // children of the media element, so skip past it.
+      rv = mSourcePointer->SetStartAfter(startContainer);
+      NS_ENSURE_SUCCESS(rv, nsnull);
+      continue;
+    }

Could this just be an assertion that startContainer == thisDomNode? It seems to me it should not be able to fail.

In that case, all we need to get from the range is its offset, and we can call SetStart to avoid having to QI the child to an nsIDOMNode.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v3

With review comment addressed - no longer need to QI the child to an nsIDOMNode.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 365049 [details] [diff] [review]
Patch v3

+    nsCOMPtr<nsIDOMNode> startContainer;
+    rv = mSourcePointer->GetStartContainer(getter_AddRefs(startContainer));
+    if (NS_FAILED(rv)) return nsnull;

This can be #ifdef DEBUG now.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v4

* With roc's requested #ifdef DEBUG.
sr+ roc.

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v5 - fixed typo

Fixed a typo. r+ doublec sr+ roc.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Oops, I found another mistake, I should have changed the QueueLoadTask() call in nsHTMLMediaElement::BindToTree() to a QueueSelectResourceTask().

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v6

QueueSelectResourceTask() in nsHTMLMediaElement::BindToTree(). Since it's a pretty small change, I'm carrying forward r+/sr+, hope that's ok.

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: v7 [Checkin: See comment 12]

I noticed this while working on bug 479711, I'm amending the patch in this bug so that all the changes related to the new load algorithm land in the same patch...

This patch is the same as v6 except it:
* Calls QueueSelectResourceTask() instead of QueueLoadTask() in nsHTMLMediaElement::DoneAddingChildren(). There are no more calls to QueueLoadTask().
* Removes nsHTMLMediaElement::QueueLoadTask(). The spec never asynchronously invokes the load() method now, so we don't need QueueLoadTask anymore.

Sorry, I should have picked this up when making the changes for patch v6. This is a pretty trivial change, so carrying forward r+/sr+.

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

Pushed: http://hg.mozilla.org/mozilla-central/rev/0d2da3338359

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

Note: the push changeset is slightly different to the patch; nsIDOMHTMLMediaElement.idl had moved to a different location, so I had to update the changes.

Comment 14

[Serge Gautherie (:sgautherie)]

(In reply to comment #12)
> Pushed: http://hg.mozilla.org/mozilla-central/rev/0d2da3338359

Does not apply to 1.9.1:
{
patching file content/html/content/public/nsHTMLMediaElement.h
Hunk #3 FAILED at 200
1 out of 4 hunks FAILED
patching file content/media/video/test/Makefile.in
Hunk #1 FAILED at 56
Hunk #2 FAILED at 164
2 out of 2 hunks FAILED
unable to find 'dom/interfaces/html/nsIDOMHTMLMediaElement.idl' for patching
1 out of 1 hunks FAILED
}

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a8b031027e4e

Comment 16

[Robert Strong (they/them - no direct email)]

test_load.html has been intermittently hanging

Comment 17

[Robert Strong (they/them - no direct email)]

(In reply to comment #16)
> test_load.html has been intermittently hanging
bug 494271