Bug 44013 – HTMLMediaElement should delay document load event

• Component: Media
• Status: RESOLVED FIXED

[Eric Carlson]

The HTML5 spec requires that a media element with a potential source delay the document's load event until the media file fails to load or the readyState reaches HAVE_CURRENT_DATA.

[Eric Carlson]

Created attachment 64423 [details]
proposed patch

[Adam Barth]

Comment on attachment 64423 [details]
proposed patch

Why is this going on Frame?  We already have this mechanism on Document.  It seems more like Document-related state than Frame-related state...

[Eric Carlson]

(In reply to comment #2)
> (From update of attachment 64423 [details])
> Why is this going on Frame?  We already have this mechanism on Document.  
The existing mechanism to prevent the Document 'load' event is DocLoader::incrementRequestCount. We could piggyback on that, but it is used by other methods where we don't want media loading to be considered, eg. DocumentLoader::isLoadingInAPISense, FrameView::checkStopDelayingDeferredRepaints, Loader::cancelRequests, etc.

> It seems more like Document-related state than Frame-related state...
Fair enough, the methods do make more sense on Document.

[Eric Carlson]

Created attachment 64466 [details]
proposed patch

[Eric Carlson]

Ignore the change to WebCore.xcodeproj, it is unnecessary and I will remove it before landing.

[Eric Seidel (no email)]

Comment on attachment 64466 [details]
proposed patch

Eric asked me over irc if this patch counted as "pooping" on Document (per the webkit-dev thread).  It does, sorta.  But no more than any of hundreds of patches which came before it, each adding just one member and one function. :)  Eventually one of us has to pay the piper and refactor Document into smaller pieces.  Including clearly one class who's job it is to dispatch load events and handle all the state associated with them.  If someone wanted to do that refactoring before this patch, that'd be great.  But I'm certainly not going to stand in the way of this patch. :)

[Darin Adler]

Comment on attachment 64466 [details]
proposed patch

> +    , m_elementsBlockingLoad(0)

The name for this data member is not great. It contains a count, not elements, so it's not good that it's named "elements".

> +void Document::setDelayLoadEvent(bool delay)

I don't like the name for this. It makes it sound like it sets something named "delay load event". But it doesn't. It actually bumps a count. And it's important that it's clear from the caller's point of view that each call to delay is balanced by a call to cancel the delay. I suggest having two functions instead of one, maybe one with the names add/remove or register/unregister or added/removed.

> +    if (!frame() || !frame()->loader())
> +        return;

There is no need to check frame()->loader() for 0. Every frame always has a loader.

I think you also need a comment that says that a document is either always in a frame or never, to make it clear this does not dynamically change.

> +    // Used to allow element that loads data without using a FrameLoader to delay the 'load' event.
> +    void setDelayLoadEvent(bool);
> +    bool isDelayingLoadEvent() const { return m_elementsBlockingLoad; }

It seems unfortunate to have to add a new mechanism for this that is unrelated to the other loading, and also a bit far away. Perhaps this counter can go into a loading-related object instead of the document. I'm thinking DocumentLoader would be a good place for it. That way you wouldn't have to touch Document.h at all.

> +    if (m_delayingTheLoadEvent)
> +        setDelayLoadEvent(false);

There's no need for the if statement here. The setDelayLoadEvent already does that check. Same issue in the HTMLMediaElement destructor.

> +    // The spec doesn't say to block the load event until we actually run the asynchronous section
> +    // algorithm, but do it now because we won't start that until after the timer fires and the 
> +    // event may have already fired by then.
> +    setDelayLoadEvent(true);
> +
>  }

You should remove that extra blank line here.

> +void HTMLMediaElement::setDelayLoadEvent(bool delay)

I think this should be named setShouldDelayLoadEvent.

> +    if (m_delayingTheLoadEvent == delay || !delay && !m_delayingTheLoadEvent)
> +        return;

There's no need for the second part of this expression. It's already covered by the first half.

> +    m_delayingTheLoadEvent = delay;

I think this should be named m_shouldDelayLoadEvent.

This looks good and is probably nearly OK to land already, but I had enough minor nitpick comments that I think I'll say review- for now.

[Darin Adler]

Document is a better place than Frame, but probably DocumentLoader is even better.

[Darin Adler]

However, I don’t see an easy way to get from the Document to its DocumentLoader, so that might be a slight snag in the plan. Leaving it on Document would not be the end of the world. (Note that I am saying DocumentLoader, not DocLoader.)

[Eric Carlson]

Created attachment 64894 [details]
updated patch

(In reply to comment #7)
> (From update of attachment 64466 [details])
> > +    , m_elementsBlockingLoad(0)
> 
> The name for this data member is not great. It contains a count, not elements, so it's not good that it's named "elements".
> 
I ended up using m_loadEventDelayCount,

> > +void Document::setDelayLoadEvent(bool delay)
> 
> I don't like the name for this. It makes it sound like it sets something named "delay load event". But it doesn't. It actually bumps a count. And it's important that it's clear from the caller's point of view that each call to delay is balanced by a call to cancel the delay. I suggest having two functions instead of one, maybe one with the names add/remove or register/unregister or added/removed.
> 
I changed the method names to incrementLoadEventDelayCount and decrementLoadEventDelayCount.

> > +    // Used to allow element that loads data without using a FrameLoader to delay the 'load' event.
> > +    void setDelayLoadEvent(bool);
> > +    bool isDelayingLoadEvent() const { return m_elementsBlockingLoad; }
> 
> It seems unfortunate to have to add a new mechanism for this that is unrelated to the other loading, and also a bit far away. Perhaps this counter can go into a loading-related object instead of the document. I'm thinking DocumentLoader would be a good place for it. That way you wouldn't have to touch Document.h at all.
> 
I tried this, but the DocumentLoader is sometimes NULL when the element needs to change the delay count (depending on the timing of the media engine state changes), so I ended up leaving it on Document for now.

[Darin Adler]

Comment on attachment 64894 [details]
updated patch

> +    // Used to allow element that loads data without using a FrameLoader to delay the 'load' event.

I think you mean without using a ResourceLoader. Or without going through the FrameLoader.

> +    // Still waiting for elements that don't use a FrameLoader?

Same comment here.

[Maciej Stachowiak]

Comment on attachment 64894 [details]
updated patch

> @@ -140,6 +139,7 @@ HTMLMediaElement::~HTMLMediaElement()
>  {
>      if (m_isWaitingUntilMediaCanStart)
>          document()->removeMediaCanStartListener(this);
> +    setShouldDelayLoadEvent(false);
>      document()->unregisterForDocumentActivationCallbacks(this);
>      document()->unregisterForMediaVolumeCallbacks(this);
>  }

WebCore/html/HTMLMediaElement.cpp:145
 +  }
Is it really right to have this in the destructor? Perhaps it should be in removedFromDocument.Reasons for thinking so: 

(1) In general it's a good idea to minimize the amount of substantive work that goes on in the destructor.
(2) Media elements that are not in the document should not (I think) delay the load event.

Otherwise looks ok, so r=me assuming the above is either fixed or has a good reason.

[Eric Carlson]

(In reply to comment #12)
> (From update of attachment 64894 [details])
> > @@ -140,6 +139,7 @@ HTMLMediaElement::~HTMLMediaElement()
> >  {
> >      if (m_isWaitingUntilMediaCanStart)
> >          document()->removeMediaCanStartListener(this);
> > +    setShouldDelayLoadEvent(false);
> >      document()->unregisterForDocumentActivationCallbacks(this);
> >      document()->unregisterForMediaVolumeCallbacks(this);
> >  }
> 
> WebCore/html/HTMLMediaElement.cpp:145
>  +  }
> Is it really right to have this in the destructor? Perhaps it should be in removedFromDocument.Reasons for thinking so: 
> 
> (1) In general it's a good idea to minimize the amount of substantive work that goes on in the destructor.
> (2) Media elements that are not in the document should not (I think) delay the load event.
> 
> Otherwise looks ok, so r=me assuming the above is either fixed or has a good reason.

The spec doesn't specify different behavior when an element is not in the document. I checked with Hixie and he confirmed that this is by design, media and image elements should delay the load event even when they are not in the document.

[Eric Carlson]

http://trac.webkit.org/changeset/66110

[WebKit Review Bot]

http://trac.webkit.org/changeset/66110 might have broken Leopard Intel Release (Tests)

[Darin Adler]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 64894 [details] [details])
> > Is it really right to have this in the destructor? Perhaps it should be in removedFromDocument.Reasons for thinking so: 
> > 
> > (1) In general it's a good idea to minimize the amount of substantive work that goes on in the destructor.
> > (2) Media elements that are not in the document should not (I think) delay the load event.
> > 
> > Otherwise looks ok, so r=me assuming the above is either fixed or has a good reason.
> 
> The spec doesn't specify different behavior when an element is not in the document. I checked with Hixie and he confirmed that this is by design, media and image elements should delay the load event even when they are not in the document.

The destructor will be called when the last reference to the media element goes away; that includes JavaScript wrappers that go away when garbage collection triggers. It doesn’t make sense to me that a media element would trigger a load event (stop delaying it) when the last wrapper to it is garbage collected; that's essentially an unpredictable time. So there must be a well-defined time to stop delaying the load event other than the actual destruction of the media element. If it’s not removal from the document it still must be some other kind of well-defined time.

[Nikolas Zimmermann]

Eric, that also broke media/video-source-error.html.
I'm adding it to the platform/mac/Skipped file, which already lists a bunch of tests (that you are working on, I suppose):

# Skipped while Eric Carlson works on a fix.
# https://bugs.webkit.org/show_bug.cgi?id=28221
fast/layers/video-layer.html
media/audio-delete-while-step-button-clicked.html
media/video-controls-transformed.html
media/video-controls-zoomed.html

[Eric Seidel (no email)]

Please track regressions with a separate bug.

[Ademar Reis]

Revision r66110 cherry-picked into qtwebkit-2.2 with commit 4733735 <http://gitorious.org/webkit/qtwebkit/commit/4733735>