Add UITelemetry for the tab-audio control.

RESOLVED FIXED in Firefox 47

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bwinton, Assigned: u562192, Mentored)

Tracking

unspecified
Firefox 47
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: [qx:link:spec] [lang=js] [good-first-bug])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
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.
Flags: needinfo?(bwinton)
(Reporter)

Comment 2

2 years ago
Yeah, let me just look up some stuff, and I'll paste a couple of links for you!  :)  Thanks!
Flags: needinfo?(bwinton)
(Reporter)

Comment 3

2 years ago
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.
(Assignee)

Comment 4

2 years ago
Yes, I will look into both. Thank you for the details.
(Assignee)

Comment 5

2 years ago
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.
(Assignee)

Comment 6

2 years ago
Created attachment 8720988 [details] [diff] [review]
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.
Attachment #8720988 - Flags: feedback?(bwinton)
(Reporter)

Comment 7

2 years ago
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.  :)
(Reporter)

Comment 8

2 years ago
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.)
Attachment #8720988 - Flags: feedback?(bwinton) → feedback+
(Assignee)

Comment 9

2 years ago
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.
Attachment #8720988 - Attachment is obsolete: true
Attachment #8721235 - Flags: feedback?(bwinton)
(Reporter)

Comment 10

2 years ago
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.  :)
Attachment #8721235 - Flags: feedback?(bwinton) → feedback+
(Assignee)

Comment 11

2 years ago
I've found more than one person with the Gijs alias, I don't know which one to request review from.
(Reporter)

Comment 12

2 years ago
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 
(Assignee)

Comment 13

2 years ago
Created attachment 8721336 [details] [diff] [review]
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.
Attachment #8721235 - Attachment is obsolete: true
Attachment #8721336 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 14

2 years ago
(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

2 years ago
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)
Attachment #8721336 - Flags: review?(gijskruitbosch+bugs) → feedback+
(Assignee)

Comment 16

2 years ago
Created attachment 8721461 [details] [diff] [review]
Followup on Gijs Kruitbosch's review
Attachment #8721336 - Attachment is obsolete: true
Attachment #8721461 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 17

2 years ago
(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

2 years ago
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?gijskruitbosch+bugs@gmail.com
```

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)
Attachment #8721461 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 19

2 years ago
Created attachment 8721770 [details] [diff] [review]
Fixed commit message
Attachment #8721461 - Attachment is obsolete: true
Attachment #8721770 - Flags: review?(gijskruitbosch+bugs)

Comment 20

2 years ago
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!
Attachment #8721770 - Flags: review?(gijskruitbosch+bugs) → review+
(Reporter)

Comment 22

2 years ago
(Assigning it to Benjamin, because it's clear he's been working on it.  ;)
Assignee: nobody → benhum.bd
(Assignee)

Comment 23

2 years ago
When is / should this bug be closed and marked as resolved?
Flags: needinfo?(bwinton)

Comment 24

2 years ago
(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! :-)
Flags: needinfo?(bwinton)

Comment 25

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d5153540fc53
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
You need to log in before you can comment on or make changes to this bug.