942274 - Collect counts on customization events in pre-Australis Firefox

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

We're interested in counting how many times the following events occur:

* Count how many times the user brings up the customization dialog
* Count how many times a "button customization event" occurs (buttons are added, moved or removed with customization dialog open)
* Count how many times the user restores toolbars to defaults

Comment 1

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

Attachment: Patch v1

Seems to do the trick - the measurements are showing up in about:telemetry...

Comment 2

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

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

And this one?

Comment 3

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

Attachment: Patch v1.1

Rebased and simplified somewhat.

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8337081 [details] [diff] [review]
Patch v1.1

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

::: browser/base/content/browser.js
@@ +3423,5 @@
> +    case "move":
> +      BrowserUiTelemetry.countToolbarEvent("moveToolbarItem");
> +      break;
> +    case "remove":
> +      BrowserUiTelemetry.countToolbarEvent("removeToolbarItem");

Having "Toolbar" in these names seems redundant, since this method (and the resulting telemetry object) solely exists only for toolbar related info. As a bonus, that means would could just directly pass aType to countToolbarEvent(), and all this can be a one-line change.

::: browser/modules/BrowserUiTelemetry.jsm
@@ -1,1 @@
> -// This Source Code Form is subject to the terms of the Mozilla Public

I suspect you did not intend to delete a third of the license header :)

@@ +17,5 @@
> +const kCountableToolbarEvents = ["addToolbarItem",
> +                                 "moveToolbarItem",
> +                                 "removeToolbarItem",
> +                                 "resetToolbars",
> +                                 "startCustomizing"];

Shouldn't need to define these - it's just adding maintenance burden. If any callers try recording an event that isn't in _toolbarEventCounts, it should just be accepted, and start counting then and there.

Comment 5

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

Attachment: Patch v1.2

(In reply to Blair McBride [:Unfocused] from comment #4)
> Comment on attachment 8337081 [details] [diff] [review]
> Patch v1.1
> 
> Review of attachment 8337081 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/browser.js
> @@ +3423,5 @@
> > +    case "move":
> > +      BrowserUiTelemetry.countToolbarEvent("moveToolbarItem");
> > +      break;
> > +    case "remove":
> > +      BrowserUiTelemetry.countToolbarEvent("removeToolbarItem");
> 
> Having "Toolbar" in these names seems redundant, since this method (and the
> resulting telemetry object) solely exists only for toolbar related info. As
> a bonus, that means would could just directly pass aType to
> countToolbarEvent(), and all this can be a one-line change.
> 

Good call. Done.

> ::: browser/modules/BrowserUiTelemetry.jsm
> @@ -1,1 @@
> > -// This Source Code Form is subject to the terms of the Mozilla Public
> 
> I suspect you did not intend to delete a third of the license header :)
> 

Right indeed. Fixed.

> @@ +17,5 @@
> > +const kCountableToolbarEvents = ["addToolbarItem",
> > +                                 "moveToolbarItem",
> > +                                 "removeToolbarItem",
> > +                                 "resetToolbars",
> > +                                 "startCustomizing"];
> 
> Shouldn't need to define these - it's just adding maintenance burden. If any
> callers try recording an event that isn't in _toolbarEventCounts, it should
> just be accepted, and start counting then and there.

Removed.

I'm also using UITelemetry's internal event counting mechanism instead of hand-rolling my own.

Comment 6

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

Comment on attachment 8338688 [details] [diff] [review]
Patch v1.2

Something more like this?

Comment 7

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

Attachment: Patch v1.3

Whoops - leftover bind that was no longer necessary. Going to give a self-review before I re-request...

Comment 8

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

Comment on attachment 8338794 [details] [diff] [review]
Patch v1.3

Ok, I feel more confident about this one.

Comment 9

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

Comment on attachment 8338794 [details] [diff] [review]
Patch v1.3

Hey Gijs, feel like taking a look at this?

Comment 10

[:Gijs (he/him)]

Comment on attachment 8338794 [details] [diff] [review]
Patch v1.3

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

This looks alright, but I missed how it notifies us when people use "restore defaults". Is that in there or did you not implement that (yet)?

::: browser/base/content/browser.js
@@ +3410,5 @@
>    switch (aType) {
>      case "iconsize":
>      case "mode":
>        retrieveToolbarIconsizesFromTheme();
>        break;

Out of interest, do we not want to know how many people have this customized? What with people screaming about small icons going away? Like with the add-on bar status, maybe we want to just report a static indicator of the mode/iconsize... just a thought!

Comment 11

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

(In reply to :Gijs Kruitbosch from comment #10)
> Comment on attachment 8338794 [details] [diff] [review]
> Patch v1.3
> 
> Review of attachment 8338794 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks alright, but I missed how it notifies us when people use "restore
> defaults". Is that in there or did you not implement that (yet)?

toolkit already calls BrowserToolbarCustomizeChange with "reset" when resetting the toolbars - it's just the other actions that needed to pass change types.

> 
> ::: browser/base/content/browser.js
> @@ +3410,5 @@
> >    switch (aType) {
> >      case "iconsize":
> >      case "mode":
> >        retrieveToolbarIconsizesFromTheme();
> >        break;
> 
> Out of interest, do we not want to know how many people have this
> customized? What with people screaming about small icons going away? Like
> with the add-on bar status, maybe we want to just report a static indicator
> of the mode/iconsize... just a thought!

It's a good idea - I'll add it to the list of probes, and file a separate bug.

Thanks for the review!

Comment 12

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

Landed on Holly as https://hg.mozilla.org/projects/holly/rev/0173dce3d537

Comment 13

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

I'll wait to ensure this doesn't introduce any orange before requesting aurora uplift.

Comment 14

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

Comment on attachment 8338794 [details] [diff] [review]
Patch v1.3

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

None. This just gives BrowserUITelemetry the capability of counting customization start, reset, add, move and remove events.


User impact if declined: 

None.


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

Lots of manual testing on Holly, which has since merged to Aurora.


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

None.


String or IDL/UUID changes made by this patch:

None.

Comment 15

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

Removing Australis:P1 whiteboard tag because these already block an Australis:P1 bug, and being redundant isn't helpful.

Comment 16

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

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