1249281 - Add UITelemetry for the tab-audio control.

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Blake Winton (:bwinton) (:☕️)]

We went to a bunch of work to get that wonderful icon showing up and letting us click on it to mute a tab, and it would be great to know whether anyone is actually using the new feature…

I figure a line in the toggleMuteAudio function at https://dxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#6264 which logged whether the tab was being muted or unmuted and the value of aMuteReason in UITelemetry would give us that information.

Comment 1

[u562192]

I would like to have a go at this.

I have had a brief look at https://dxr.mozilla.org/mozilla-central/source/browser/modules/BrowserUITelemetry.jsm but I am still at a loss for how you would log UITelemetry data. Would you mind telling me or linking me to some code to see how it's done, whichever is easier for you.

I'm a first time contributor by the way, but I have Firefox Nightly built.

Comment 2

[Blake Winton (:bwinton) (:☕️)]

Yeah, let me just look up some stuff, and I'll paste a couple of links for you!  :)  Thanks!

Comment 3

[Blake Winton (:bwinton) (:☕️)]

Okay, so it looks like we could do a couple of different things here…  The easiest might be to just add 'soundplaying-icon' to the end of the SPECIAL_CASES list at https://dxr.mozilla.org/mozilla-central/source/browser/modules/BrowserUITelemetry.jsm#136  That won't tell us whether people are muting or unmuting, but it will make the control show up just like all the other controls, and consistency is a nice thing.

The other option (and we could do both!) would be to create a new function just underneath countPanicEvent in https://dxr.mozilla.org/mozilla-central/source/browser/modules/BrowserUITelemetry.jsm#609 called something like "countTabMutingEvent".  It would look very similar to countPanicEvent, but with an extra parameter, and "forget-button" changed to "tab-audio-control" or something.  Then we would call that function in a manner something like https://dxr.mozilla.org/mozilla-central/source/browser/components/customizableui/CustomizableWidgets.jsm#1205

Did you want to give both a try, and see how they go?  To test that your code works, you should be able to go to "about:telemetry", click the "Simple Measurements" section, and look for the new stuff in the "UITelemetry" blob.

Comment 4

[u562192]

Yes, I will look into both. Thank you for the details.

Comment 5

[u562192]

After implementing the first of your suggetions, I had to set toolkit.telemetry.enabled to true in about:config for the UITelemetry to be visible. Just leaving it for future reference.

Comment 6

[u562192]

Attachment: Initial attempt. Tab-muting-reason not tested. Doesn't always count keyboard shortcut.

I have not tested if the reason is ever passed as not-null, but from what I have been able to find it is only supposed to be a string if the tab was muted from an extension. I don't have one to test it with. From your first suggestion, Blake, it does not count if you toggle the mute by ctrl+m shortcut so I don't know how useful it is since it doesn't necessarily indicate the total number of toggles.

Comment 7

[Blake Winton (:bwinton) (:☕️)]

Cool.  I'll take a look at the patch later tonight…  In general, not many people use keyboard shortcuts, so I think the counts will still be fairly useful.  :)

Comment 8

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8720988 [details] [diff] [review]
Initial attempt. Tab-muting-reason not tested. Doesn't always count keyboard shortcut.

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

All in all, I like it!  For bonus points, can you write a short set of steps that our QA team can use to verify it works as designed?
And once that's done, let's try asking Gijs for the review.  (florian would probably be another good choice…)

Thanks!

::: browser/base/content/tabbrowser.xml
@@ +6274,4 @@
>            } else {
>              browser.mute();
>              this.setAttribute("muted", "true");
> +            BrowserUITelemetry.countTabMutingEvent("mute", aMuteReason || null);

Maybe do the check for null in the countTabMutingEvent?

::: browser/modules/BrowserUITelemetry.jsm
@@ +609,5 @@
>  
>    countPanicEvent: function(timeId) {
>      this._countEvent(["forget-button", timeId]);
>    },
> +  

There are some extra spaces here that should be removed.

@@ +611,5 @@
>      this._countEvent(["forget-button", timeId]);
>    },
> +  
> +  countTabMutingEvent: function(action, reason) {
> +    this._countEvent(["tab-audio-control", action, reason]);

I _think_ we also want to convert a null value to the string "null", to make _countEvent happy…  (Maybe not, though.)

Comment 9

[u562192]

Attachment: Followup to initial patch to (hopefully) accommodate bwinton's feedback

QA verification steps:

1: Toggle the mute state of a tab.

2: Look for "UITelemetry" in about:telemetry under "Simple Measurements"
2.1: https://bugzilla.mozilla.org/show_bug.cgi?id=1249281#c5

3: muted tab: "soundplaying-icon" should be counted up and so should "mute" under "tab-audio-control". "null" in "mute" should be counted up if the tab was muted through the UI, otherwise a string containing the ID of the extension that muted the tab should be counted up.
3: unmuted tab: "soundplaying-icon" should be counted up and so should "unmute" under "tab-audio-control". "null" in "unmute" should be counted up if the tab was muted through the UI, otherwise a string containing the ID of the extension that unmuted the tab should be counted up.
3.1: in either case, only "soundplaying-icon" is not counted up if the mute was toggled by ctrl-m shortcut.

Comment 10

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8721235 [details] [diff] [review]
Followup to initial patch to (hopefully) accommodate bwinton's feedback

(This looks like a patch on top of the previous patch, so I think you need to merge the two patches, and you can replace "reason == null" with just "reason", to also catch undefined.  But I'm confident you can do that without asking me for feedback again, so the next step will be to just straight to asking Gijs for an actual review.  :)

Comment 11

[u562192]

I've found more than one person with the Gijs alias, I don't know which one to request review from.

Comment 12

[Blake Winton (:bwinton) (:☕️)]

Ah, so Mozilla Pro-Tip: If you put a ":" in front, that'll reduce it to one person (and the correct one, in this case 

Comment 13

[u562192]

Attachment: The two previous patches merged into one.

bwinton suggested that I replace "reason == null" with just "reason" to also catch undefined, but I have tested this code as it is and it handles undefined correctly since (undefined == null) evaluates to true.

Comment 14

[u562192]

(In reply to Blake Winton (:bwinton) (:☕️) from comment #12)
> Ah, so Mozilla Pro-Tip: If you put a ":" in front, that'll reduce it to one
> person (and the correct one, in this case

Thanks ! As you can tell I'm just getting used to Bugzilla and Mercurial :)

Comment 15

[:Gijs (he/him)]

Comment on attachment 8721336 [details] [diff] [review]
The two previous patches merged into one.

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

Almost there. One nit below, and for the next patch, can you please make sure the commit message is in the usual format of:

Bug 1234567 - Summary of what you're changing, r?<reviewer>

(see: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions )

::: browser/modules/BrowserUITelemetry.jsm
@@ +611,5 @@
>      this._countEvent(["forget-button", timeId]);
>    },
>  
> +  countTabMutingEvent: function(action, reason) {
> +    this._countEvent(["tab-audio-control", action, (reason == null ? "null" : reason)]);

While you're right that

reason == null

works here, it's a little confusing, because nobody actually ever passes null through this code path. The parameter is either not given (which results in the value being undefined) or it's an add-on UUID. Reporting it as "null" to telemetry in the "not given" case is maybe not the best either. Please can we use:

this._countEvent(["tab-audio-control", action, reason || "no reason given"])

(we don't need () brackets around the || or ternary expression here)

Comment 16

[u562192]

Attachment: Followup on Gijs Kruitbosch's review

Comment 17

[u562192]

(In reply to Benjamin Dahse from comment #9)
> Created attachment 8721235 [details] [diff] [review]
> Followup to initial patch to (hopefully) accommodate bwinton's feedback
> 
> QA verification steps:
> 
> 1: Toggle the mute state of a tab.
> 
> 2: Look for "UITelemetry" in about:telemetry under "Simple Measurements"
> 2.1: https://bugzilla.mozilla.org/show_bug.cgi?id=1249281#c5
> 
> 3: muted tab: "soundplaying-icon" should be counted up and so should "mute"
> under "tab-audio-control". "null" in "mute" should be counted up if the tab
> was muted through the UI, otherwise a string containing the ID of the
> extension that muted the tab should be counted up.
> 3: unmuted tab: "soundplaying-icon" should be counted up and so should
> "unmute" under "tab-audio-control". "null" in "unmute" should be counted up
> if the tab was muted through the UI, otherwise a string containing the ID of
> the extension that unmuted the tab should be counted up.
> 3.1: in either case, only "soundplaying-icon" is not counted up if the mute
> was toggled by ctrl-m shortcut.

Corrections to accommodate https://bugzilla.mozilla.org/show_bug.cgi?id=1249281#c15

3: muted tab: "soundplaying-icon" should be counted up and so should "mute" under "tab-audio-control". "no reason given" in "mute" should be counted up if the tab was muted through the UI, otherwise a string containing the UUID of the add-on that muted the tab should be counted up.
3: unmuted tab: "soundplaying-icon" should be counted up and so should "unmute" under "tab-audio-control". "no reason given" in "unmute" should be counted up if the tab was muted through the UI, otherwise a string containing the UUID of the add-on that unmuted the tab should be counted up.

Comment 18

[:Gijs (he/him)]

Comment on attachment 8721461 [details] [diff] [review]
Followup on Gijs Kruitbosch's review

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

Great! Unfortunately now the commit message looks like:

```
Make UITelemetry count the number of times tabs have been muted or unmuted, and include a potential reason
* * *
By feedback, whitespace has been removed, null checks have been moved to a central location, the string null is passed instead of the explicit value null
* * *
Bug 1249281 - Replaced ternary expression with ||, r?[e-mail]
```

Can you replace it with just:

Bug 1249281 - add UITelemetry for tab audio mute control, r=gijs

?

With that, r=me.

(Apologies for the weekend-induced delay)

Comment 19

[u562192]

Attachment: Fixed commit message

Comment 20

[:Gijs (he/him)]

Comment on attachment 8721770 [details] [diff] [review]
Fixed commit message

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

r=me . I'll land this in a little bit. Thanks!

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d5153540fc53

Comment 22

[Blake Winton (:bwinton) (:☕️)]

(Assigning it to Benjamin, because it's clear he's been working on it.  ;)

Comment 23

[u562192]

When is / should this bug be closed and marked as resolved?

Comment 24

[:Gijs (he/him)]

(In reply to Benjamin Dahse (UTC+1) from comment #23)
> When is / should this bug be closed and marked as resolved?

When the changeset from comment #21 is merged to mozilla-central, which will happen sometime in 24 hours after it was landed, normally speaking, it will (sort of) automatically be marked as FIXED and so on. Nothing you need to do! :-)

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d5153540fc53

Comment 26

[Wikipage]

When is / should this bug be closed and marked as resolved? Facing issue when try to visit this website:
https://umairahmad.me/fake-wikipedia-page-creator/