Bug 21116 - Audio from <audio> or <video> element continues after tab is closed
Summary: Audio from <audio> or <video> element continues after tab is closed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-09-25 12:15 PDT by Eric Carlson
Modified: 2009-02-19 11:06 PST (History)
3 users (show)

See Also:


Attachments
proposed patch (15.95 KB, patch)
2008-09-25 12:54 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
revised patch (20.36 KB, patch)
2008-09-26 09:18 PDT, Eric Carlson
koivisto: 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 2008-09-25 12:15:47 PDT
If a page with an <audio> or <video> element has been in the page cache, audio continues to play for several seconds after the tab with the element is closed.

To reproduce:
1. Open Safari
2. Open two blank tabs
3. In one tab, open a page with a media element that has audio.
4. Go to http://webkit.org/ in the tab with the media element
5. After the page loads, click the Back button
6. Start the media element playing and close the tab

Audio continues to play for a few seconds.
Comment 1 Eric Carlson 2008-09-25 12:54:32 PDT
Created attachment 23816 [details]
proposed patch
Comment 2 Adele Peterson 2008-09-25 13:01:01 PDT
the basic concept seems ok to me.  I'd like to get comments from Hyatt or Brady.  CC'ing them.
Comment 3 Adele Peterson 2008-09-25 13:05:01 PDT
Another option is to add the notification to Frame::pageDestroyed-- if we didn't want to change the willSaveToCache/didRestoreFromCache concept.
Comment 4 Eric Carlson 2008-09-25 13:13:17 PDT
FWIW, Antti suggested this approach. CC'ing him as well.
Comment 5 Antti Koivisto 2008-09-25 22:21:50 PDT
I noticed that there are still some references to page cache left in connection with these callbacks. I see these in the patch:

Document::m_pageCacheCallbackElements
HTMLInputElement ::needsCacheCallback()
HTMLMediaElement::m_inPageCache

For consistency it would be good to rename them as well and check that semantics still make sense (that is they really care about whether document is active or not rather than specifically about being in page cache or not). At least something like HTMLMediaElement::m_inActiveDocument makes total sense. 
Comment 6 Eric Carlson 2008-09-26 09:18:20 PDT
Created attachment 23851 [details]
revised patch

Revised patch per Antti's comments.
Comment 7 Antti Koivisto 2008-09-26 09:31:44 PDT
Comment on attachment 23851 [details]
revised patch

This should still be renamed to match variable name for consistency:

bool inPageCache() const { return !m_inActiveDocument; }

It should return the value without negation and call sites and should be reversed instead. (or alternatively find a good variable name that is true when inactive, m_inInactiveDocument sounds bad).

This is minor, r=me with or without the change.
Comment 8 Eric Carlson 2008-09-26 10:30:36 PDT
Revised per last comment and committed as r36957.
Comment 9 Simon Fraser (smfr) 2009-02-19 11:06:05 PST
See also bug 24031.