949217 - Collect UITelemetry on how many extra toolbars there are

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

Extra toolbars include:

1) Add-on supplied toolbars
2) Toolbars added via the "Add New Toolbar" option in pre-Australis customize mode.

We probably want counts for each.

Comment 1

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

Attachment: Patch v1

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8346219 [details] [diff] [review]
Patch v1

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

Tentative r=me as long as you look in to the possibility about breaking an assumption that a different probe is making in regards to the absence of the toolbar-menubar on OSX.

::: browser/modules/BrowserUITelemetry.jsm
@@ -138,5 @@
>    'appmenu_charset',
>  ];
>  
>  const DEFAULT_TOOLBAR_SETS = {
> -#ifndef XP_MACOSX

Is changing this going to break some numbers for one of the other probes?

@@ +547,5 @@
>                            if (DEFAULT_ITEMS.indexOf(node.id) != -1)];
>  
> +    // Now subtract the default toolbars and the custom toolbars from
> +    // the total number of toolbars to produce the list of add-on provided
> +    // toolbars.

This comment seems redundant as it describes *what* the code is doing. A comment here would only be necessary/useful if it described *why* this code was needed, but in this case I think it is pretty straight-forward.

Comment 3

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

(In reply to Jared Wein [:jaws] (Away 20 Dec to 2 Jan) from comment #2)
> Comment on attachment 8346219 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 8346219 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Tentative r=me as long as you look in to the possibility about breaking an
> assumption that a different probe is making in regards to the absence of the
> toolbar-menubar on OSX.
> 
> ::: browser/modules/BrowserUITelemetry.jsm
> @@ -138,5 @@
> >    'appmenu_charset',
> >  ];
> >  
> >  const DEFAULT_TOOLBAR_SETS = {
> > -#ifndef XP_MACOSX
> 
> Is changing this going to break some numbers for one of the other probes?
> 

No - the menubar is actually there on OS X, it's just hidden. The only thing that this will affect is that menubar-items is going to appear in the defaultKept items for OS X, which I think is OK. If UX / UR is not interested in that, we can still filter it out in post since we know which OS the ping came from.

> @@ +547,5 @@
> >                            if (DEFAULT_ITEMS.indexOf(node.id) != -1)];
> >  
> > +    // Now subtract the default toolbars and the custom toolbars from
> > +    // the total number of toolbars to produce the list of add-on provided
> > +    // toolbars.
> 
> This comment seems redundant as it describes *what* the code is doing. A
> comment here would only be necessary/useful if it described *why* this code
> was needed, but in this case I think it is pretty straight-forward.

Yeah, good call. This comment adds nothing - just me chattering. I'll remove it.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Mike Conley (:mconley) from comment #3)
> (In reply to Jared Wein [:jaws] (Away 20 Dec to 2 Jan) from comment #2)
> > Comment on attachment 8346219 [details] [diff] [review]
> > Patch v1
> > 
> > Review of attachment 8346219 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Tentative r=me as long as you look in to the possibility about breaking an
> > assumption that a different probe is making in regards to the absence of the
> > toolbar-menubar on OSX.
> > 
> > ::: browser/modules/BrowserUITelemetry.jsm
> > @@ -138,5 @@
> > >    'appmenu_charset',
> > >  ];
> > >  
> > >  const DEFAULT_TOOLBAR_SETS = {
> > > -#ifndef XP_MACOSX
> > 
> > Is changing this going to break some numbers for one of the other probes?
> > 
> 
> No - the menubar is actually there on OS X, it's just hidden. The only thing
> that this will affect is that menubar-items is going to appear in the
> defaultKept items for OS X, which I think is OK. If UX / UR is not
> interested in that, we can still filter it out in post since we know which
> OS the ping came from.

Are we going to be able to connect one number to another? In my experience with telemetry we were not able to know how two different probes intersected unless the probe was designed to intersect values from the beginning.

Comment 5

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

I don't think I understand the question. Can you please re-phrase?

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Talked over IRC. My question is not an issue to worry about.

Comment 7

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

Thanks jwas - landed in Holly as https://hg.mozilla.org/projects/holly/rev/0d0f14bc4913

Comment 8

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

jwas. That's how someone from Brooklyn says jaws.

Comment 9

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

Comment on attachment 8346219 [details] [diff] [review]
Patch v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

None. Gives BrowserUITelemetry the capability of recording a count of how many toolbars are in the UI - both add-on provided, and custom toolbars added via the customization dialog.


User impact if declined: 

None.


Testing completed (on m-c, etc.): 

All manual at this point.


Risk to taking this patch (and alternatives if risky): 

Very low.


String or IDL/UUID changes made by this patch:

None.

Comment 10

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

Landed in mozilla-beta as https://hg.mozilla.org/releases/mozilla-beta/rev/e3b60ef085ee

Comment 11

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

Landed in mozilla-aurora as https://hg.mozilla.org/releases/mozilla-aurora/rev/b20cdabdfb5b

Comment 12

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

Attachment: Patch v1 - for Australis

The Australis port was basically a re-write, but a simple one thankfully. I took the occasion to rename DEFAULT_TOOLBAR_PLACEMENTS to DEFAULT_AREA_PLACEMENTS, which makes much more sense.

Comment 13

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

Comment on attachment 8359810 [details] [diff] [review]
Patch v1 - for Australis

Nearing the finish line with these, Gijs!

Comment 14

[:Gijs (he/him)]

Comment on attachment 8359810 [details] [diff] [review]
Patch v1 - for Australis

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

r=me

Comment 15

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

Thanks! Landed on fx-team as https://hg.mozilla.org/integration/fx-team/rev/4bf3d999a962

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4bf3d999a962