Removing a media element from the document may trigger last second load()

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Tor Arne Vestbø]

Removing a media element from the document may trigger last-second load()

The current spec says:

"When a media element is removed from a Document, if the media element's networkState attribute has a value other than NETWORK_EMPTY then the user agent must act as if the pause() method had been invoked."

And for pause():

"If the media element's networkState attribute has the value NETWORK_EMPTY, then the user agent must invoke the load()  method and wait for it to return."

So in effect, if we add a guard for NETWORK_EMPTY in HTMLMediaElement::removedFromDocument() we can get rid of one of the conditions where we load() during removedFromDocument().  

The other condition is when there's no m_player. What I'm wondering is if we really need to load() if m_player is nil in pause() (and play for that matter) and/or if the networkState() == EMPTY check enough? Ie., will there be cases where networkState() > EMPTY but m_player is 0?

[Tor Arne Vestbø]

Created attachment 24946 [details]
Preliminary patch for the first load() condition

[Antti Koivisto]

Comment on attachment 24946 [details]
Preliminary patch for the first load() condition

Looks good. How about a test case?

[Eric Seidel (no email)]

Comment on attachment 24946 [details]
Preliminary patch for the first load() condition

Clearing review flag since this patch is lacking a test case and changelog.

[Tor Arne Vestbø]

No problem Eric, the patch was just for discussion. Assigning to me, and test case in the pipeline.

[Tor Arne Vestbø]

Created attachment 25740 [details]
Patch with test case

Same check for networkState() != EMPTY but with test case. 

There's still a code path that can trigger this condition: if the element is removed from the document while m_player is 0, but I'm not sure when that will happen. 

We could guard for that too in removedFromDocument(), but would then miss out on the timeupdateEvent and pauseEvent sent from pause() (if that's even a code path we could hit). Any thoughts about this Antti?

Thanks!

[Darin Adler]

Comment on attachment 25740 [details]
Patch with test case

r=me

[Brent Fulgham]

Committed in revision r40666.

Confirmed media test cases pass with update.