Bug 22105

Summary: Removing a media element from the document may trigger last second load()
Product: WebKit Reporter: Tor Arne Vestbø <vestbo>
Component: WebKit Misc.Assignee: Tor Arne Vestbø <vestbo>
Status: RESOLVED FIXED    
Severity: Normal CC: adele, koivisto
Priority: P3    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Preliminary patch for the first load() condition
eric: review-
Patch with test case darin: review+

Description Tor Arne Vestbø 2008-11-06 10:14:02 PST
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?
Comment 1 Tor Arne Vestbø 2008-11-06 10:27:34 PST
Created attachment 24946 [details]
Preliminary patch for the first load() condition
Comment 2 Antti Koivisto 2008-11-06 12:19:23 PST
Comment on attachment 24946 [details]
Preliminary patch for the first load() condition

Looks good. How about a test case?
Comment 3 Eric Seidel (no email) 2008-12-02 14:16:58 PST
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.
Comment 4 Tor Arne Vestbø 2008-12-02 23:40:26 PST
No problem Eric, the patch was just for discussion. Assigning to me, and test case in the pipeline.
Comment 5 Tor Arne Vestbø 2008-12-04 09:05:03 PST
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!
Comment 6 Darin Adler 2008-12-04 09:11:42 PST
Comment on attachment 25740 [details]
Patch with test case

r=me
Comment 7 Brent Fulgham 2009-02-04 22:42:11 PST
Committed in revision r40666.

Confirmed media test cases pass with update.