Closed Bug 1180523 Opened 9 years ago Closed 9 years ago

Enable muting tabs and controlling their volumes in a way that persists across navigations

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

Attachments

(2 files)

This bug covers implementing bug 486262 comment 86 on the audio channel backend side.
This is needed so that we can preserve this information across navigations.
Attachment #8629701 - Flags: review?(amarchesini)
This will ensure that if the window is already in the muted state, for
example, a newly created audio channel agent is correctly muted as well.
Attachment #8629702 - Flags: review?(amarchesini)
Attachment #8629701 - Flags: review?(amarchesini) → review+
Comment on attachment 8629702 [details] [diff] [review]
Part 2: Update the volume of the audio channel agent from the window as when it's created for the first time

Review of attachment 8629702 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think we need this. We should change the volume when it start playing. Can you tell me why this is required?

::: dom/audiochannel/AudioChannelAgent.cpp
@@ +118,5 @@
>  
>    mWithVideo = aWithVideo;
>  
> +  // Update the volume from the window.
> +  WindowVolumeChanged();

This should already happen, right?
Actually 'init' doesn't mean that the media is starting playing.
Attachment #8629702 - Flags: review?(amarchesini) → review-
(In reply to Andrea Marchesini (:baku) from comment #3)
> I don't think we need this. We should change the volume when it start
> playing. Can you tell me why this is required?

Without this, you can get into the following situation:

1. You go to page 1 which plays audio, and you mute the tab.
2. You navigate to page 2.  It so far doesn't have any agents.
3. Page 2 starts to play a new audio element.  Its agent doesn't know that the outer window has the muted flag, so it starts to play audio where it should be muted.

> ::: dom/audiochannel/AudioChannelAgent.cpp
> @@ +118,5 @@
> >  
> >    mWithVideo = aWithVideo;
> >  
> > +  // Update the volume from the window.
> > +  WindowVolumeChanged();
> 
> This should already happen, right?
> Actually 'init' doesn't mean that the media is starting playing.

Hmm, should I add this to StartPlaying()?  That fixes the above issue as well...
Flags: needinfo?(amarchesini)
Keywords: leave-open
ping?
> 3. Page 2 starts to play a new audio element.  Its agent doesn't know that
> the outer window has the muted flag, so it starts to play audio where it
> should be muted.

In this case, the new audio element will call NotifyStartPlaying() and it will receive the new volume and the mute flag.
It should receive 'muted'. Correct?
Flags: needinfo?(amarchesini)
(In reply to Andrea Marchesini (:baku) from comment #8)
> > 3. Page 2 starts to play a new audio element.  Its agent doesn't know that
> > the outer window has the muted flag, so it starts to play audio where it
> > should be muted.
> 
> In this case, the new audio element will call NotifyStartPlaying() and it
> will receive the new volume and the mute flag.
> It should receive 'muted'. Correct?

That was definitely not the case with the old audio channel service.  I tried to test with the new one, and now the mute functionality is completely broken, so I can't test this!  I filed bug 1183356 for that...  Let's punt on this for now.
OK, so with bug 1183356 fixed, the issue now is that if you are in a muted window, a media element will be set to the paused state when it sees that after NotifyStartedPlaying, the window is muted.

I assume this is the desired behavior for b2g?

This is definitely not desired on desktop though, we should consider muted as muting the element, and not preventing its playback.

What do you think?
Flags: needinfo?(amarchesini)
Let's close this one.  I filed bug 1183700 for the remaining issue.
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: needinfo?(amarchesini)
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: