Closed Bug 942274 Opened 6 years ago Closed 6 years ago

Collect counts on customization events in pre-Australis Firefox

Categories

(Firefox :: Toolbars and Customization, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 28
Tracking Status
firefox27 --- fixed

People

(Reporter: mconley, Assigned: mconley)

References

Details

(Whiteboard: [Non-Australis-Only])

Attachments

(1 file, 3 obsolete files)

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
Attached patch Patch v1 (obsolete) — Splinter Review
Seems to do the trick - the measurements are showing up in about:telemetry...
Comment on attachment 8337040 [details] [diff] [review]
Patch v1

And this one?
Attachment #8337040 - Flags: review?(bmcbride)
Attached patch Patch v1.1 (obsolete) — Splinter Review
Rebased and simplified somewhat.
Attachment #8337040 - Attachment is obsolete: true
Attachment #8337040 - Flags: review?(bmcbride)
Attachment #8337081 - Flags: review?(bmcbride)
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.
Attachment #8337081 - Flags: review?(bmcbride) → review-
Attached patch Patch v1.2 (obsolete) — Splinter Review
(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.
Attachment #8337081 - Attachment is obsolete: true
Whiteboard: [Australis:P1] → [Australis:P1][Non-Australis-Only]
Comment on attachment 8338688 [details] [diff] [review]
Patch v1.2

Something more like this?
Attachment #8338688 - Flags: review?(bmcbride)
Attached patch Patch v1.3Splinter Review
Whoops - leftover bind that was no longer necessary. Going to give a self-review before I re-request...
Attachment #8338688 - Attachment is obsolete: true
Attachment #8338688 - Flags: review?(bmcbride)
Comment on attachment 8338794 [details] [diff] [review]
Patch v1.3

Ok, I feel more confident about this one.
Attachment #8338794 - Flags: review?(bmcbride)
Comment on attachment 8338794 [details] [diff] [review]
Patch v1.3

Hey Gijs, feel like taking a look at this?
Attachment #8338794 - Flags: review?(bmcbride) → review?(gijskruitbosch+bugs)
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!
Attachment #8338794 - Flags: review?(gijskruitbosch+bugs) → review+
(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!
Landed on Holly as https://hg.mozilla.org/projects/holly/rev/0173dce3d537
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 28
I'll wait to ensure this doesn't introduce any orange before requesting aurora uplift.
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.
Attachment #8338794 - Flags: approval-mozilla-beta?
Removing Australis:P1 whiteboard tag because these already block an Australis:P1 bug, and being redundant isn't helpful.
Whiteboard: [Australis:P1][Non-Australis-Only] → [Non-Australis-Only]
Attachment #8338794 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.