Bug 44013 - HTMLMediaElement should delay document load event
Summary: HTMLMediaElement should delay document load event
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media Elements (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords:
Depends on:
Blocks: 51249
  Show dependency treegraph
 
Reported: 2010-08-14 11:54 PDT by Eric Carlson
Modified: 2011-01-24 12:19 PST (History)
6 users (show)

See Also:


Attachments
proposed patch (19.45 KB, patch)
2010-08-14 12:15 PDT, Eric Carlson
abarth: review-
Details | Formatted Diff | Diff
proposed patch (20.14 KB, patch)
2010-08-15 22:44 PDT, Eric Carlson
darin: review-
Details | Formatted Diff | Diff
updated patch (20.41 KB, patch)
2010-08-19 13:21 PDT, Eric Carlson
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Carlson 2010-08-14 11:54:47 PDT
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.
Comment 1 Eric Carlson 2010-08-14 12:15:00 PDT
Created attachment 64423 [details]
proposed patch
Comment 2 Adam Barth 2010-08-14 23:04:23 PDT
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...
Comment 3 Eric Carlson 2010-08-15 22:22:05 PDT
(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.
Comment 4 Eric Carlson 2010-08-15 22:44:47 PDT
Created attachment 64466 [details]
proposed patch
Comment 5 Eric Carlson 2010-08-16 08:43:26 PDT
Ignore the change to WebCore.xcodeproj, it is unnecessary and I will remove it before landing.
Comment 6 Eric Seidel 2010-08-17 08:28:02 PDT
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. :)
Comment 7 Darin Adler 2010-08-17 13:40:22 PDT
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.
Comment 8 Darin Adler 2010-08-17 13:40:47 PDT
Document is a better place than Frame, but probably DocumentLoader is even better.
Comment 9 Darin Adler 2010-08-17 13:41:38 PDT
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.)
Comment 10 Eric Carlson 2010-08-19 13:21:19 PDT
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.
Comment 11 Darin Adler 2010-08-24 17:48:36 PDT
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.
Comment 12 Maciej Stachowiak 2010-08-24 17:52:08 PDT
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.
Comment 13 Eric Carlson 2010-08-26 06:00:09 PDT
(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.
Comment 14 Eric Carlson 2010-08-26 09:51:15 PDT
http://trac.webkit.org/changeset/66110
Comment 15 WebKit Review Bot 2010-08-26 10:41:36 PDT
http://trac.webkit.org/changeset/66110 might have broken Leopard Intel Release (Tests)
Comment 16 Darin Adler 2010-08-26 11:41:48 PDT
(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.
Comment 17 Nikolas Zimmermann 2010-08-27 01:56:12 PDT
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
Comment 18 Eric Seidel 2010-12-20 22:31:25 PST
Please track regressions with a separate bug.
Comment 19 Ademar Reis 2011-01-24 12:19:41 PST
Revision r66110 cherry-picked into qtwebkit-2.2 with commit 4733735 <http://gitorious.org/webkit/qtwebkit/commit/4733735>