Closed Bug 1183700 Opened 10 years ago Closed 10 years ago

Don't prevent playback of media elements in a muted tab on desktop

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla42
Tracking Flags:
Tracking Status
firefox42 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: baku)

References

Details

Attachments

(1 file)

desktop.patch
12.05 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
STR: 1. Apply my UI patches in bug 486262. 2. Go to a tab with an audio element, start playing it, and mute the tab. 3. Navigate to a new page, and play a new audio element. The element remains paused, but it should keep making progress. What is happening is because HTMLMediaElement::WindowVolumeChanged() prevents the playback of the element if aMuted. Instead, we need to just mute the audio and let the playback proceed as normal. We should also have a test for this, something like browser_mediaPlayback.js and browser_mute.js.
Attached patch desktop.patch — — Splinter Review
Attachment #8634155 - Flags: review?(ehsan)
The previous patch does this: 1. expose mozinterruptbegin/end only when AudioChannelAPI is enabled 2. Window refresh works only with outer window 3. testing for all the desktop tab/muting features 4. expose computedVolume/computedMuted for testing
Comment on attachment 8634155 [details] [diff] [review] desktop.patch Review of attachment 8634155 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsGlobalWindow.cpp @@ +3738,5 @@ > void > nsPIDOMWindow::RefreshMediaElements() > { > nsRefPtr<AudioChannelService> service = AudioChannelService::GetOrCreate(); > + service->RefreshAgentsVolume(this); Why are you passing this? I think you should pass GetOuterWindow(), which I did in bug 1183356. Please just remove this hunk. ::: dom/html/HTMLMediaElement.cpp @@ +4481,5 @@ > DispatchAsyncEvent(NS_LITERAL_STRING("mozinterruptend")); > } > } > > +#ifdef MOZ_B2G I wonder if MOZ_B2G is the right condition here.. Please watch out for mulet. I honestly don't know what MOZ_B2G means these days! @@ +4782,5 @@ > + > +bool > +HTMLMediaElement::ComputedMuted() const > +{ > + return mMuted; Did you need to & with MUTED_BY_AUDIO_CHANNEL? ::: toolkit/content/tests/browser/browser_mediaPlayback_mute.js @@ +8,5 @@ > }); > } > > +function get_audio_element(browser) { > + var list = browser.contentDocument.getElementsByTagName('audio'); This is not very e10s friendly, even though it might work because of the existing CPOW shims. I asked mconley, and the better way to do this is to use ContentTask.spawn(). That will let you run a function in the context of the child process. ::: toolkit/content/tests/browser/file_mediaPlayback2.html @@ +1,2 @@ > <!DOCTYPE html> > +<body><div id="content"></div><body> Please remove this. document.body exists even without this!
Attachment #8634155 - Flags: review?(ehsan) → review+
> Please remove this. document.body exists even without this! not really. It doesn't work for the iframe. Pushed on try.
(In reply to Andrea Marchesini (:baku) from comment #4) > > Please remove this. document.body exists even without this! > > not really. It doesn't work for the iframe. OK, no big deal if you keep it!
https://hg.mozilla.org/mozilla-central/rev/43912de433e3
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox42: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Depends on: 1192818
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: