Closed Bug 479711 Opened 11 years ago Closed 11 years ago

Media elements should delay the document 'load' event

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, 3 obsolete files)

See http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2009-February/018624.html

We should do this because it will make media elements easier to use, it integrates media elements into HTML in a useful way that plugins can't, and it will let us simplify our reftests.

Boris, what's the best way to do this? If we put media loads in the document loadgroup, is there a way to take them out when the decoder reaches the HAVE_CURRENT_DATA state? Or do we need to put a synthetic request into the loadgroup so that we can pretend it's finished when we reach HAVE_CURRENT_DATA?
Flags: blocking1.9.1+
Moving loads in and out of the loadgroups sounds error-prone to me; I assume that right now those are background loads, right?

It might be that the right answer here is to just make nsIDocument::BlockOnload and UnblockOnload calls at the right times.
Right now these loads aren't associated with any loadgroup.

BlockOnLoad/UnblockOnLoad sounds fine, we can use that.
Priority: -- → P2
> Right now these loads aren't associated with any loadgroup.

That's really bad, actually.  What stops them when the user closes the tab or navigates to a different page or some such?  Or heck hits the Stop button?
nsHTMLMediaElement::DestroyContent and PresShell::Freeze. Stop button doesn't do anything.
And we make sure that we don't ever start those loads for nodes that are in disconnected fragments?  That works, I guess.  Is not exposing various info about the media file through the DOM in that situation ok?

In any case, I guess that's all not that relevant to this bug.
(In reply to comment #5)
> And we make sure that we don't ever start those loads for nodes that are in
> disconnected fragments?

No, we can start loads for disconnected elements, as per spec. The fact that PresShell::Freeze doesn't find them is a filed bug I think. Well, it's known anyway.

Hmm, I didn't think about whether a load in a disconnected media element should delay the load event in a document. I guess it shouldn't.
Putting that into PresShell::Freeze doesn't make sense; the document might not even have a presshell...

It seems to me that loads should go in the document loadgroup, unless you coalesce them across documents, with the LOAD_BACKGROUND flag if you don't want them affecting the throbber or directly affecting onload.  Then all the load-canceling stuff will Just Work for free; that's the point of loadgroups.  You'll still need to manually manage onload, of course.
OK. However, we still need to do something to stop the media playing when the document goes into the bfcache, even if the load is automatically suspended. So we still have a bug there.
Documents with active loads don't go into bfcache.
Active as far as the loadgroup goes, that is.
That's not good. Media elements often keep loading forever, but we don't want such pages to not be bf-cachable.
We took that hit for other pages with loading forever elements (e.g. images that are using JPEG push), precisely because we had no good solution for stopping and restarting the network loads as needed....

Maybe the media situation is better because you can always use byte-range requests or the like to restart?
Is now a good time to ask what nsIRequest::Suspend/Resume actually does?
For a lot of requests, "not much".

For HTTP, I _think_ it just makes us not read data off the socket, which works great until the server times us out.  But biesi or darin would know better...
OK, then I think using those for bf-cached pages is reasonable for us; that's what we're currently doing.

We should probably detect a dropped connection somehow and restart it as necessary, at least if the server supports byte-range requests.

Thanks. I think we should probably proceed with not having our requests in the loadgroup, and have an alternative solution for freezing, based on tracking a list of all playing elements for a given document.
(and we should use the same approach for freezing plugins too really, since display:none plugins really should be supported)
I mean, plugins in display:none IFRAMEs
Alternately, we should have a way to flag requests that bfcache should not bail on but should instead suspend/resume.  Or something.  Centralize that logic and such...
Assignee: nobody → chris
Attached patch Patch v1 (obsolete) — Splinter Review
Adds delaying-the-load-event flag to media element. Delays document load event until metadataloaded, or load error.

We set the flag to true (and so delay the doc's load event) in:
* QueueSelectResourceTask()
* QueueLoadFromSourceTask()
* LoadWithChannel()

This is actually stretching the spec a bit. The spec says we call the media-selection algorithm asynchronously, and we don't set the flag until we're in the media-selection algorithm. But if we do that, the load can finish before the async-call event fires, so we need to set the flag before we start the async call. I've posted to WHATWG about this, I'm still awaiting reply:

http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2009-March/018679.html

The delaying-the-load-event flag is set to false as per the spec:
* After loadedmetadata.
* When we wait for src/source to be added.
* On error.

I also added a test, and removed the reftest-wait for ogg reftests.
Attachment #365573 - Flags: superreview?(roc)
Attachment #365573 - Flags: review?(chris.double)
(In reply to comment #19)
> The delaying-the-load-event flag is set to false as per the spec:
> * After loadedmetadata.

This should be after FirstFrameLoaded. The spec doesn't currently say that but Hixie agreed, I think:
http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2009-February/018663.html

One question is whether an element that is not in the document should delay the load event. I kinda think it shouldn't. The spec suggests it should, but isn't explicit about it. What do you think?
(In reply to comment #20)
> One question is whether an element that is not in the document should delay the
> load event. I kinda think it shouldn't. The spec suggests it should, but isn't
> explicit about it. What do you think?

I think media not in a document should not delay the load event. The purpose of delaying the load event was primarily to allow layout to stabilize before firing the load event, and things not in the document won't affect layout... If they're added to the document before load fires, and they start to load, the load event will be delayed anyway, and if they aren't added to the doc, they won't affect the initial reflow.

Do we delay the load event for images outside of the document?

Hmm... I wonder if load still fires if we remove a loading media during load...
(In reply to comment #21)
> Hmm... I wonder if load still fires if we remove a loading media during load...

It seems so, but only once the detached media loads enough to set the delaying-the-load-event to false. If we're don't delay the load event for things not in the document, then I should probably set the delaying-the-load-event flag to false in UnbindFromTree() as well.
(In reply to comment #21)
> (In reply to comment #20)
> > One question is whether an element that is not in the document should delay the
> > load event. I kinda think it shouldn't. The spec suggests it should, but isn't
> > explicit about it. What do you think?
> 
> I think media not in a document should not delay the load event. The purpose of
> delaying the load event was primarily to allow layout to stabilize before
> firing the load event, and things not in the document won't affect layout... If
> they're added to the document before load fires, and they start to load, the
> load event will be delayed anyway, and if they aren't added to the doc, they
> won't affect the initial reflow.

I agree. But the patch you have here does delay the load event for not-in-doc elements and needs to be updated, right?

> Do we delay the load event for images outside of the document?

I don't know.

> Hmm... I wonder if load still fires if we remove a loading media during load...

We should stop delaying the load event in UnbindFromTree, yes.
Attached patch Patch v2 (obsolete) — Splinter Review
As patch v1, except:
* Stops delaying load event in UnbindFromTree().
* Stops delaying load event in FirstFrameLoaded() instead of MetadataLoaded().
* ChangeDelayLoadStatus() bails before changing state if !IsInDoc().
Attachment #365573 - Attachment is obsolete: true
Attachment #365591 - Flags: superreview?(roc)
Attachment #365591 - Flags: review?(chris.double)
Attachment #365573 - Flags: superreview?(roc)
Attachment #365573 - Flags: review?(chris.double)
+    is(gLoadedMetaData, true, "onload was not delayed until after metadataloaded");

This should be loadeddata now I guess...

You might also want a test to verify that if an element not in a document starts loading, but hasn't finished loading before we bind it to a loading document, it blocks onload until it finishes loading. And another test to verify that if an element in a document starts loading, but hasn't finished loading before we remove it from the document, it doesn't block onload. And another test to verify that if an element isn't delaying the load event, but we start a new load via various means (e.g. adding a <source>), then it does delay the load event if necessary.

(In reply to comment #24)
> * ChangeDelayLoadStatus() bails before changing state if !IsInDoc().

I think this isn't right. We should change our internal state always, we just don't want to change the document load blocking state when we're not in the document. We need something in BindToTree to start blocking the load event if our delay-the-load-event flag is set ... and UnbindFromTree shouldn't change our delay-the-load-event flag, it should just unblock the load event if our flag is set. Make sense?
> Do we delay the load event for images outside of the document?

Yes.  Typically, those are image preloads and pages depend on those being loaded before onload fires.

> Hmm... I wonder if load still fires if we remove a loading media during load...

Write a test, please.  And yes, write the tests comment 25 mentions.
Hmm. I think being consistent with images trumps other considerations. So let's make out-of-doc elements block their owner document's load event, after all.
But we'll have to be careful that when the owner document changes for an out-of-document media element, we transfer our block correctly. We'll need tests for that.
Or you could do what images do: don't change which document you're blocking on.  That is, when you block save the pointer to the document you blocked on, and make sure to unblock that same document.
OK then, let's do that.
Attached patch Patch v3 (obsolete) — Splinter Review
Similar to patch v1, but blocks loads on the document that was its owner when load started. Does not transfer blocking over to another document if the element is moved between documents. Also added guard to prevent multiple async events to call SelectResource() being set at the same time. This means that there can only ever be one execution of SelectResource() happening at once. Added some more tests too.
Attachment #365591 - Attachment is obsolete: true
Attachment #365805 - Flags: superreview?(roc)
Attachment #365805 - Flags: review?(chris.double)
Attachment #365591 - Flags: superreview?(roc)
Attachment #365591 - Flags: review?(chris.double)
Might be worth dropping the pointer to the document after you unblock.
Comment on attachment 365805 [details] [diff] [review]
Patch v3

Yeah, null out mLoadBlockedDoc after unblocking.
Attachment #365805 - Flags: superreview?(roc) → superreview+
As patch v3, with nulling out mLoadBlockedDoc. Still seem to need the cycle collection to traverse mLoadBlockedDoc though.
Attachment #365805 - Attachment is obsolete: true
Attachment #365814 - Flags: superreview+
Attachment #365814 - Flags: review?(chris.double)
Attachment #365805 - Flags: review?(chris.double)
Bug 479859 should land before this.
Depends on: 479859
Whiteboard: [needs landing]
Blocks: 478957
Attachment #365814 - Flags: review?(chris.double) → review+
http://hg.mozilla.org/mozilla-central/rev/37b4cf75a6a5
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] → [baking for 1.9.1]
Keywords: checkin-needed
Whiteboard: [baking for 1.9.1] → [baking for 1.9.1][needs landing]
Flags: in-testsuite+
Whiteboard: [baking for 1.9.1][needs landing] → [needs 1.9.1 landing: after bug 479859]
Target Milestone: --- → mozilla1.9.2a1
Attachment #365814 - Attachment description: Patch v4 → Patch v4 [Checkin: Comment 36]
checkin-needed: can't be landed as blocking bug misses updated patch.
Keywords: checkin-needed
Whiteboard: [needs 1.9.1 landing: after bug 479859] → [needs 1.9.1 landing]
You need to log in before you can comment on or make changes to this bug.