1238311 - Implement browser.tabs muted tabs functionality

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P4)

Description

[Kris Maglione [:kmag]]

This includes:

 * The `muted` property of `tabs.update` and `tabs.query`.
 * The `audible` function of `tabs.query`.
 * The `audible` and `mutedInfo` properties of Tab objects.
 * The `audible` and `mutedInfo` functionality of onUpdated events.

Comment 1

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1238311: Part 1 - Add "mutedinfo" tab attribute, and related getters. r?ehsan

Review commit: https://reviewboard.mozilla.org/r/32343/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32343/

Comment 2

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1238311: Part 2 - Persist tab mutedInfo in sessionstore. r?ehsan

Review commit: https://reviewboard.mozilla.org/r/32345/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32345/

Comment 3

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1238311: Part 3 - [webext] Add audible and muted support to browser.tabs API. r?gabor

Review commit: https://reviewboard.mozilla.org/r/32347/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32347/

Comment 4

[(no longer active)]

Hi Kris,

Nice patches, thanks for writing them!

Before I start reviewing, I have a couple of high level questions.  I'm assuming that the API you're trying to implement is the one documented here: <https://developer.chrome.com/extensions/tabs>

1. It seems weird to store the MuteInfo json object on the tab itself.  It seems to me that instead, we can only store a muteReason field with the following values:

  * null indicating that the mute was initiated by the user.
  * <string> the ID of the extension last modifying the mute state, if any.

It seems to me that based on that, we can synthesize the correct MutedInfo when we need it (since we at least for now don't support MutedInfoReason "capture" value.)

2. The audible attribute is documented as "Whether the tab has produced sound over the past couple of seconds (but it might not be heard if also muted). Equivalent to whether the speaker audio indicator is showing.".  That doesn't match what the soundplaying attribute currently implements (since it will turn to false as soon as the tab stops emitting audio.)  I think you'd need to do something more sophisticated, perhaps by managing another attribute (recentlyAudible) on the tab which you'd manage using a timer...

(Also note that parts 1 and 2 will require tests too; see browser_audioTabIcon.js.  The get_tab_attributes() calls there are testing the sessionstore stored state.)

Comment 5

[Kris Maglione [:kmag]]

(In reply to :Ehsan Akhgari from comment #4)
> Hi Kris,
> 
> Nice patches, thanks for writing them!
> 
> Before I start reviewing, I have a couple of high level questions.  I'm
> assuming that the API you're trying to implement is the one documented here:
> <https://developer.chrome.com/extensions/tabs>

Yup.

> 1. It seems weird to store the MuteInfo json object on the tab itself.  It
> seems to me that instead, we can only store a muteReason field with the
> following values:
> 
>   * null indicating that the mute was initiated by the user.
>   * <string> the ID of the extension last modifying the mute state, if any.
> 
> It seems to me that based on that, we can synthesize the correct MutedInfo
> when we need it (since we at least for now don't support MutedInfoReason
> "capture" value.)

I think the problem is that we need to be able to determine how the tab ended
up in the given mute state. I.e., there's a difference between a tab that's
never been muted, and one that's been unmuted by the user. The API requires
that the "reason" for the former be null, and for the latter be "user".

> 2. The audible attribute is documented as "Whether the tab has produced
> sound over the past couple of seconds (but it might not be heard if also
> muted). Equivalent to whether the speaker audio indicator is showing.".
> That doesn't match what the soundplaying attribute currently implements
> (since it will turn to false as soon as the tab stops emitting audio.)  I
> think you'd need to do something more sophisticated, perhaps by managing
> another attribute (recentlyAudible) on the tab which you'd manage using a
> timer...

I think the key is "Equivalent to whether the speaker audio indicator is
showing". If Chrome's icon really does stay visible for a couple of seconds
after sound has stopped (I've learned not to trust the Chrome extension docs),
then we're going to have to choose one of those two things to be incompatible
with. I'd rather it match our UI behavior unless it turns out that there are
extensions that depend on it doing otherwise.

> (Also note that parts 1 and 2 will require tests too; see
> browser_audioTabIcon.js.  The get_tab_attributes() calls there are testing
> the sessionstore stored state.)

I added tests in part 3, in the webextensions component.

I was planning to add tests for the session store behavior, but there weren't
any existing tests for the muted attribute (except to check that it doesn't
get restored), so there was no obvious place to add them.

I'll add more tests for the attributes and accessors to browser_audioTabIcon.js

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8711838 [details]
MozReview Request: Bug 1238311: Part 1 - Add "mutedinfo" tab attribute, and related getters. r?ehsan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32343/diff/1-2/

Comment 7

[Kris Maglione [:kmag]]

Comment on attachment 8711839 [details]
MozReview Request: Bug 1238311: Part 2 - Persist tab mutedInfo in sessionstore. r?ehsan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32345/diff/1-2/

Comment 8

[Kris Maglione [:kmag]]

Comment on attachment 8711840 [details]
MozReview Request: Bug 1238311: Part 3 - [webext] Add audible and muted support to browser.tabs API. r?gabor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32347/diff/1-2/

Comment 9

[Kris Maglione [:kmag]]

I added tests to parts 1 and 2

Comment 10

[(no longer active)]

(In reply to Kris Maglione [:kmag] from comment #5)
> > 1. It seems weird to store the MuteInfo json object on the tab itself.  It
> > seems to me that instead, we can only store a muteReason field with the
> > following values:
> > 
> >   * null indicating that the mute was initiated by the user.
> >   * <string> the ID of the extension last modifying the mute state, if any.
> > 
> > It seems to me that based on that, we can synthesize the correct MutedInfo
> > when we need it (since we at least for now don't support MutedInfoReason
> > "capture" value.)
> 
> I think the problem is that we need to be able to determine how the tab ended
> up in the given mute state. I.e., there's a difference between a tab that's
> never been muted, and one that's been unmuted by the user. The API requires
> that the "reason" for the former be null, and for the latter be "user".

Good point, but that's only one more bit, so we can have:

  * undefined: indicating that the tab was never muted
  * null and <string> as before

> > 2. The audible attribute is documented as "Whether the tab has produced
> > sound over the past couple of seconds (but it might not be heard if also
> > muted). Equivalent to whether the speaker audio indicator is showing.".
> > That doesn't match what the soundplaying attribute currently implements
> > (since it will turn to false as soon as the tab stops emitting audio.)  I
> > think you'd need to do something more sophisticated, perhaps by managing
> > another attribute (recentlyAudible) on the tab which you'd manage using a
> > timer...
> 
> I think the key is "Equivalent to whether the speaker audio indicator is
> showing". If Chrome's icon really does stay visible for a couple of seconds
> after sound has stopped (I've learned not to trust the Chrome extension
> docs),
> then we're going to have to choose one of those two things to be incompatible
> with. I'd rather it match our UI behavior unless it turns out that there are
> extensions that depend on it doing otherwise.

Ah, interesting.  So Chrome's icon actually fades away around 10 seconds after audio playback has finished.

Hmm...   I can read this both ways.  Up to you on which behavior we want to implement.

> > (Also note that parts 1 and 2 will require tests too; see
> > browser_audioTabIcon.js.  The get_tab_attributes() calls there are testing
> > the sessionstore stored state.)
> 
> I added tests in part 3, in the webextensions component.

I prefer to have tests for each part separately, since this code has been changing pretty heavily and we've had breakages which could have been prevented by moar tests.  :-)

> I was planning to add tests for the session store behavior, but there weren't
> any existing tests for the muted attribute (except to check that it doesn't
> get restored), so there was no obvious place to add them.

You want to put both tests in browser_audioTabIcon.js.

> I'll add more tests for the attributes and accessors to
> browser_audioTabIcon.js

Cool!

Comment 11

[Kris Maglione [:kmag]]

(In reply to :Ehsan Akhgari from comment #10)
> Good point, but that's only one more bit, so we can have:
> 
>   * undefined: indicating that the tab was never muted
>   * null and <string> as before

Hm. I guess that should work, yeah. I'll give it a try.

> Ah, interesting.  So Chrome's icon actually fades away around 10 seconds
> after audio playback has finished.
> 
> Hmm...   I can read this both ways.  Up to you on which behavior we want to
> implement.

I think I'm going to stick with the current behavior for now and see what kind
of feedback I get. We can always add a timeout later.

Comment 12

[(no longer active)]

(In reply to Kris Maglione [:kmag] from comment #11)
> (In reply to :Ehsan Akhgari from comment #10)
> > Good point, but that's only one more bit, so we can have:
> > 
> >   * undefined: indicating that the tab was never muted
> >   * null and <string> as before
> 
> Hm. I guess that should work, yeah. I'll give it a try.

Thanks!

> > Ah, interesting.  So Chrome's icon actually fades away around 10 seconds
> > after audio playback has finished.
> > 
> > Hmm...   I can read this both ways.  Up to you on which behavior we want to
> > implement.
> 
> I think I'm going to stick with the current behavior for now and see what
> kind
> of feedback I get. We can always add a timeout later.

That's fine by me, I do like the simpler approach.

Comment 13

[Kris Maglione [:kmag]]

Comment on attachment 8711838 [details]
MozReview Request: Bug 1238311: Part 1 - Add "mutedinfo" tab attribute, and related getters. r?ehsan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32343/diff/2-3/

Comment 14

[Kris Maglione [:kmag]]

Comment on attachment 8711839 [details]
MozReview Request: Bug 1238311: Part 2 - Persist tab mutedInfo in sessionstore. r?ehsan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32345/diff/2-3/

Comment 15

[Kris Maglione [:kmag]]

Comment on attachment 8711840 [details]
MozReview Request: Bug 1238311: Part 3 - [webext] Add audible and muted support to browser.tabs API. r?gabor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32347/diff/2-3/

Comment 16

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8711840 [details]
MozReview Request: Bug 1238311: Part 3 - [webext] Add audible and muted support to browser.tabs API. r?gabor

https://reviewboard.mozilla.org/r/32347/#review29553

Thanks, this looks great.

Comment 17

[(no longer active)]

Comment on attachment 8711838 [details]
MozReview Request: Bug 1238311: Part 1 - Add "mutedinfo" tab attribute, and related getters. r?ehsan

https://reviewboard.mozilla.org/r/32343/#review29613

Looks great!

::: browser/base/content/tabbrowser.xml:6089
(Diff revision 3)
> +      <field name="muteReason">undefined</field>

Can you please add some comments here explaining the three expected values?

Comment 18

[(no longer active)]

Comment on attachment 8711839 [details]
MozReview Request: Bug 1238311: Part 2 - Persist tab mutedInfo in sessionstore. r?ehsan

https://reviewboard.mozilla.org/r/32345/#review29615

Comment 19

[Kris Maglione [:kmag]]

Thanks for the reviews.

Comment 20

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/82e9bd573e04ceae1c017176c2173a21f189856a
Bug 1238311: Part 1 - Add "mutedinfo" tab attribute, and related getters. r=ehsan

https://hg.mozilla.org/integration/fx-team/rev/73e65a175d8cff088daeac5d0eb5c3559b830cd9
Bug 1238311: Part 2 - Persist tab mutedInfo in sessionstore. r=ehsan

https://hg.mozilla.org/integration/fx-team/rev/843c860178d36432b2d021e433ce4a2c07097347
Bug 1238311: Part 3 - [webext] Add audible and muted support to browser.tabs API. r=gabor

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/82e9bd573e04
https://hg.mozilla.org/mozilla-central/rev/73e65a175d8c
https://hg.mozilla.org/mozilla-central/rev/843c860178d3

Comment 22

[Will Bamberg [:wbamberg]]

I've updated the browser compat information in:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/MutedInfo
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/MutedInfoReason

On "audible", I don't think it's correct to say that this corresponds to whether the speaker icon is displayed. On Chrome and Firefox, it seems to corresponds to:

* if the tab is not muted, whether the tab is playing audio
* if the tab is muted, whether the tab would be playing audio if it were not muted

So if you load a page that's playing audio:
* audible === true, speaker icon is displayed

If you then mute the audio:
* audible === true, muted speaker icon is displayed

If you then navigate to a new page that doesn't play audio:
* audible === false, muted speaker icon is displayed

So that's what I've put: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/Tab.

Comment 23

[Kris Maglione [:kmag]]

Looks good to me. Thanks

Comment 24

[Vasilica Tamas, QA [:vtamas]]

Does this bug need to be covered by manual testing? Is there any webextension to verify this fix?

Comment 25

[Kris Maglione [:kmag]]

I don't think it needs any more manual verification.