1180448 - Add APIs to XUL browser for muting and unmuting audio playback i in the document loaded inside it

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Comment 1

[(no longer active)]

Attachment: Add APIs to XUL browser for muting and unmuting audio playback i in the document loaded inside it

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8629616 [details] [diff] [review]
Add APIs to XUL browser for muting and unmuting audio playback i in the document loaded inside it

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

::: toolkit/content/browser-content.js
@@ +666,5 @@
>    },
> +  receiveMessage(msg) {
> +    switch (msg.name) {
> +      case "MediaPlaybackMute": {
> +        let utils = global.content.QueryInterface(Ci.nsIInterfaceRequestor)

Generally, we just refer to "content" in here, as opposed to "global.content".

::: toolkit/content/tests/browser/browser_mute.js
@@ +5,5 @@
> +    SpecialPowers.pushPrefEnv({"set": [["media.useAudioChannelService", true]]},
> +                              resolve);
> +  });
> +
> +  let tab = gBrowser.selectedTab = gBrowser.addTab(PAGE);

Should probably use BrowserTestUtils.withNewTab here.

::: toolkit/content/widgets/browser.xml
@@ +683,5 @@
>          </body>
>        </method>
>  
> +      <field name="_audioMuted">false</field>
> +      <property name="audioMuted"

Out of curiosity, why not have a writable property audioMuted that does the job of firing the async messages? I think that'd be preferable over having the read-only property, and then a mute and unmute method.

Comment 3

[(no longer active)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #2)
> ::: toolkit/content/tests/browser/browser_mute.js
> @@ +5,5 @@
> > +    SpecialPowers.pushPrefEnv({"set": [["media.useAudioChannelService", true]]},
> > +                              resolve);
> > +  });
> > +
> > +  let tab = gBrowser.selectedTab = gBrowser.addTab(PAGE);
> 
> Should probably use BrowserTestUtils.withNewTab here.

I expect to hit the same issue as in the other bug here too, since the test works exactly the same way...

> ::: toolkit/content/widgets/browser.xml
> @@ +683,5 @@
> >          </body>
> >        </method>
> >  
> > +      <field name="_audioMuted">false</field>
> > +      <property name="audioMuted"
> 
> Out of curiosity, why not have a writable property audioMuted that does the
> job of firing the async messages? I think that'd be preferable over having
> the read-only property, and then a mute and unmute method.

I think this is a better API, since using a property setter doesn't make it obvious at all that this is sending async messages to do stuff in the other process, so people may end up writing code like:

browser.audioMuted = false;
if (condition)
  browser.audioMuted = true;

which would be perfectly find if this were a normal property, but leads to undesirable results.  I think in general property getters/setters that have these kinds of side effects suffer from a similar issue.

What do you think?

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8629616 [details] [diff] [review]
Add APIs to XUL browser for muting and unmuting audio playback i in the document loaded inside it

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

::: toolkit/content/tests/browser/browser_mute.js
@@ +5,5 @@
> +    SpecialPowers.pushPrefEnv({"set": [["media.useAudioChannelService", true]]},
> +                              resolve);
> +  });
> +
> +  let tab = gBrowser.selectedTab = gBrowser.addTab(PAGE);

See my proposed solution in that bug. :) If BrowserTestUtils.withNewTab ends up being a bust though, I'm willing to drop this.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #3)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #2)
> > ::: toolkit/content/tests/browser/browser_mute.js
> > @@ +5,5 @@
> > > +    SpecialPowers.pushPrefEnv({"set": [["media.useAudioChannelService", true]]},
> > > +                              resolve);
> > > +  });
> > > +
> > > +  let tab = gBrowser.selectedTab = gBrowser.addTab(PAGE);
> > 
> > Should probably use BrowserTestUtils.withNewTab here.
> 
> I expect to hit the same issue as in the other bug here too, since the test
> works exactly the same way...

Above comment was for this.

> 
> > ::: toolkit/content/widgets/browser.xml
> > @@ +683,5 @@
> > >          </body>
> > >        </method>
> > >  
> > > +      <field name="_audioMuted">false</field>
> > > +      <property name="audioMuted"
> > 
> > Out of curiosity, why not have a writable property audioMuted that does the
> > job of firing the async messages? I think that'd be preferable over having
> > the read-only property, and then a mute and unmute method.
> 
> I think this is a better API, since using a property setter doesn't make it
> obvious at all that this is sending async messages to do stuff in the other
> process, so people may end up writing code like:
> 
> browser.audioMuted = false;
> if (condition)
>   browser.audioMuted = true;
> 
> which would be perfectly find if this were a normal property, but leads to
> undesirable results.  I think in general property getters/setters that have
> these kinds of side effects suffer from a similar issue.
> 
> What do you think?

Ah, okay, yes, that reasoning seems sound. Let's keep your methods.

Comment 6

[(no longer active)]

Attachment: Add APIs to XUL browser for muting and unmuting audio playback in the document loaded inside it

I know that you reviewed the previous version, but do you mind quickly going over the test changes once more to make sure I have done what you wanted?  Thanks!

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8631354 [details] [diff] [review]
Add APIs to XUL browser for muting and unmuting audio playback in the document loaded inside it

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

Looks good, thanks ehsan.

::: toolkit/content/browser-content.js
@@ +693,5 @@
>  });
>  
>  let MediaPlaybackListener = {
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver,
> +                                         Ci.nsIMessageListener]),

You don't actually need to have this implement Ci.nsIMessageListener. Any object that implements receiveMessage should work properly.

@@ +719,5 @@
>      }
>    },
> +
> +  receiveMessage(msg) {
> +    switch (msg.name) {

Seems a bit silly to use a switch just for the one case. Can we just wrap this in an if and call it a day?

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/272a2cde6bd1

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/272a2cde6bd1