Closed Bug 479859 Opened 11 years ago Closed 11 years ago

Implement new new load algorithm

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: roc, Assigned: cpearce)

References

Details

(Keywords: fixed1.9.1)

Attachments

(1 file, 6 obsolete files)

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.
Flags: blocking1.9.1+
Priority: -- → P2
Assignee: nobody → chris
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #364448 - Flags: superreview?(roc)
Attachment #364448 - Flags: review?(chris.double)
+  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?
Depends on: 480889
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #364448 - Attachment is obsolete: true
Attachment #365022 - Flags: superreview?(roc)
Attachment #365022 - Flags: review?(chris.double)
Attachment #364448 - Flags: superreview?(roc)
Attachment #364448 - Flags: review?(chris.double)
+    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.
Attached patch Patch v3 (obsolete) — Splinter Review
With review comment addressed - no longer need to QI the child to an nsIDOMNode.
Attachment #365022 - Attachment is obsolete: true
Attachment #365049 - Flags: superreview?(roc)
Attachment #365049 - Flags: review?(chris.double)
Attachment #365022 - Flags: superreview?(roc)
Attachment #365022 - Flags: review?(chris.double)
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.
Attachment #365049 - Flags: superreview?(roc) → superreview+
Attached patch Patch v4 (obsolete) — Splinter Review
* With roc's requested #ifdef DEBUG.
sr+ roc.
Attachment #365049 - Attachment is obsolete: true
Attachment #365054 - Flags: superreview+
Attachment #365054 - Flags: review?(chris.double)
Attachment #365049 - Flags: review?(chris.double)
Attachment #365054 - Attachment is patch: true
Attachment #365054 - Attachment mime type: application/octet-stream → text/plain
Attachment #365054 - Flags: review?(chris.double) → review+
Attached patch Patch v5 - fixed typo (obsolete) — Splinter Review
Fixed a typo. r+ doublec sr+ roc.
Attachment #365054 - Attachment is obsolete: true
Attachment #365078 - Flags: superreview+
Attachment #365078 - Flags: review+
Oops, I found another mistake, I should have changed the QueueLoadTask() call in nsHTMLMediaElement::BindToTree() to a QueueSelectResourceTask().
Attached patch Patch v6 (obsolete) — Splinter Review
QueueSelectResourceTask() in nsHTMLMediaElement::BindToTree(). Since it's a pretty small change, I'm carrying forward r+/sr+, hope that's ok.
Attachment #365078 - Attachment is obsolete: true
Attachment #365119 - Flags: superreview+
Attachment #365119 - Flags: review+
Whiteboard: m0z1llar0x
Whiteboard: m0z1llar0x
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+.
Attachment #365119 - Attachment is obsolete: true
Attachment #365555 - Flags: superreview+
Attachment #365555 - Flags: review+
Keywords: checkin-needed
Whiteboard: [needs landing]
Blocks: 479711
Blocks: 478957
Pushed: http://hg.mozilla.org/mozilla-central/rev/0d2da3338359
Status: NEW → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing] → [baking for 1.9.1]
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.
Keywords: checkin-needed
Whiteboard: [baking for 1.9.1] → [baking for 1.9.1][needs landing]
(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
}
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [baking for 1.9.1][needs landing] → [needs 1.9.1 landing]
Target Milestone: --- → mozilla1.9.2a1
Attachment #365555 - Attachment description: v7 → v7 [Checkin: See comment 12]
test_load.html has been intermittently hanging
Depends on: 494271
(In reply to comment #16)
> test_load.html has been intermittently hanging
bug 494271
You need to log in before you can comment on or make changes to this bug.