482885 - Put media channels in the loadgroup, but with LOAD_BACKGROUND flag as needed

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

I just realized earlier today that one way we could get saner behavior out of the whole loadgroup thing is to just start the loads for media in the loadgroup, and then when we get to the point of firing onload remove the load from the loadgroup, set the nsIRequest::LOAD_BACKGROUND flag on it, and readd it.  This will allow the load to affect SSL UI up to the point where we add the flag (not perfect, but better than nothing), allow the load to pick up auth prompts as needed (for HTTP auth), and so forth.  LOAD_BACKGROUND loads don't block onload or spin the throbber or anything like that, which is exactly the effect we want here, right?

This should be mochitestable, even...  Honza might be able to test the SSL UI part of it, at least.

Comment 1

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

Do we want this for 1.9.1?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Er, yes, I think we do.  Sorry for forgetting to flag this...

Comment 3

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

Yeah, I think we do. This will make media loads integrate better with other browser features, which gives us advantages over plugin-based media playing.

Comment 4

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

Attachment: Patch v1

Patch v1 (based on top of media cache patch from bug 475411)
* When we stop delaying the load event, move any active media loads into the background, and cause all subsequent loads to be in background in media element's document's load group,
* Adds comment to nsHTMLMediaElement::mChannel to describe its lifecycle.
* Nulls out nsHTMLMediaElement::mChannel once a decoder has been constructed. The decoder already cancels and closes the channel appropriately, we just don't hang onto the reference longer than necessary.
* Patch works, with this patch the throbber throbs until first frame is loaded and load event fires.
* There is no unit test. I'm not sure how to write a unit test for this.

Comment 5

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

+  nsCOMPtr<nsIDocument> doc = GetOwnerDoc();

This can just be nsIDocument*

+  nsCOMPtr<nsILoadGroup> loadGroup = element->GetDocumentLoadGroup();
+  rv = mChannel->GetLoadGroup(getter_AddRefs(loadGroup));

The result of GetDocumentLoadGroup is ignored. I don't think you needed it anyway.

I guess we still need to call BlockOnLoad/UnblockOnload because a seek operation may make us go through a state where we have no active load and we don't want that to allow the document's load event to run?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 369412 [details] [diff] [review]
Patch v1

>+++ b/content/html/content/public/nsHTMLMediaElement.h
>+   * Returns the load group for this media element's document.

current or owner document?  Document which one (no pun intended), and add an "XXX XBL2" comment, please?

>   nsCOMPtr<nsIChannel> mChannel;

Not relevant to this bug, but isn't the canceling broken if the server responds with a 3xx?

Please file a followup bug on this.

>+++ b/content/html/content/src/nsHTMLMediaElement.cpp
>@@ -498,10 +498,12 @@ nsresult nsHTMLMediaElement::LoadResourc

>+  nsCOMPtr<nsIChannel> channel;

Looks unused.

>+++ b/content/media/video/src/nsMediaStream.cpp
>+void nsMediaStream::MoveLoadsToBackground() {

>+  nsCOMPtr<nsILoadGroup> loadGroup = element->GetDocumentLoadGroup();

As roc said, this should go away.

Again, does this mChannel need posible updating for redirects?  That is, could mChannel not be in the loadgroup by now because it got redirected?

>+  // Note: if (NS_FAILED(status)), the channel won't be in the load group.

Wouldn't it make sense to also check IsPending()?

Comment 7

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

(In reply to comment #6)
> (From update of attachment 369412 [details] [diff] [review])
> >+++ b/content/html/content/public/nsHTMLMediaElement.h
> >+   * Returns the load group for this media element's document.
> 
> current or owner document?  Document which one (no pun intended), and add an
> "XXX XBL2" comment, please?

I'm pretty sure we want the owner document for this right? Please correct me if I'm wrong.

> >   nsCOMPtr<nsIChannel> mChannel;
> 
> Not relevant to this bug, but isn't the canceling broken if the server responds
> with a 3xx?
> 
> Please file a followup bug on this.
> 

Ah, I see. On redirect we create a new channel with the same listeners... So I guess we'd detect redirect by having an nsIChannelEventSink implementation which we'd pass to NS_NewChannel(), similar to NotificationCallbacks in netwerk/test/TestProtocols.cpp?
 
> >+++ b/content/media/video/src/nsMediaStream.cpp
> >+void nsMediaStream::MoveLoadsToBackground() {

> Again, does this mChannel need posible updating for redirects? That is, could
> mChannel not be in the loadgroup by now because it got redirected?
> 

I think the reference would be stale if a redirect created a new channel, so yes.

Comment 8

[Boris Zbarsky [:bzbarsky]]

We want the owner document if we allow loads in disconnected subtrees, yes.

> So I guess we'd detect redirect by having an nsIChannelEventSink implementation

Correct.

Comment 9

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

Attachment: Patch v2

With review comments addressed. I will file a bug for the channel redirect issues.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 369609 [details] [diff] [review]
Patch v2

Looks reasonable.  I assume we already have tests for the onload thing, and that you'll coordinate with Honza to get security UI tests here?

Comment 11

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

We have tests for load event delaying, both explicit mochitests and our video reftests also depend on it.

Comment 12

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

Filed bug 485471 for the channel redirect issues.

(In reply to comment #10)
> I assume we already have tests for the onload thing, and
> that you'll coordinate with Honza to get security UI tests here?

Yeah, as Roc said, the video reftest depend on this. I will liaise with Honza about a Security UI test.

Comment 13

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

Patch for bug 475441 needs to land before this one can.

Comment 14

[chris hofmann]

> do we want this for 1.9.1?
> Er, yes, I think we do.  Sorry for forgetting to flag this...

ought to get P1 if it needs a beta cycle, or P2 if it can be slipped in before RC1

Comment 15

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

P2, but should make beta4 anyway

Comment 16

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

http://hg.mozilla.org/mozilla-central/rev/d07537579b24

Comment 17

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

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4121062ac094