Closed
Bug 1180448
Opened 9 years ago
Closed 9 years ago
Add APIs to XUL browser for muting and unmuting audio playback i in the document loaded inside it
Categories
(Toolkit :: UI Widgets, defect)
Toolkit
UI Widgets
Tracking
()
RESOLVED
FIXED
mozilla42
Tracking | Status | |
---|---|---|
firefox42 | --- | fixed |
People
(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)
References
Details
Attachments
(2 files)
4.38 KB,
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
4.71 KB,
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8629616 -
Flags: review?(mconley)
Comment 2•9 years ago
|
||
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.
Attachment #8629616 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 3•9 years ago
|
||
(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?
Flags: needinfo?(mconley)
Comment 4•9 years ago
|
||
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•9 years ago
|
||
(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.
Flags: needinfo?(mconley)
Assignee | ||
Comment 6•9 years ago
|
||
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!
Attachment #8631354 -
Flags: review?(mconley)
Comment 7•9 years ago
|
||
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?
Attachment #8631354 -
Flags: review?(mconley) → review+
Comment 9•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/272a2cde6bd1
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in
before you can comment on or make changes to this bug.
Description
•