Closed Bug 949217 Opened 6 years ago Closed 6 years ago

Collect UITelemetry on how many extra toolbars there are

Categories

(Firefox :: Toolbars and Customization, defect)

x86_64
All
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 29
Tracking Status
firefox27 --- fixed
firefox28 --- fixed

People

(Reporter: mconley, Assigned: mconley)

References

Details

(Whiteboard: [Australis:P-][fixed-in-holly][good first verify])

Attachments

(2 files)

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.
Assignee: nobody → mconley
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.
Attachment #8346219 - Flags: review?(jaws) → review+
(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.
(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.
I don't think I understand the question. Can you please re-phrase?
Flags: needinfo?(jaws)
Talked over IRC. My question is not an issue to worry about.
Flags: needinfo?(jaws)
Thanks jwas - landed in Holly as https://hg.mozilla.org/projects/holly/rev/0d0f14bc4913
Whiteboard: [Australis:P-] → [Australis:P-][fixed-in-holly]
jwas. That's how someone from Brooklyn says jaws.
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.
Attachment #8346219 - Flags: approval-mozilla-beta?
Attachment #8346219 - Flags: approval-mozilla-aurora?
Attachment #8346219 - Flags: approval-mozilla-beta?
Attachment #8346219 - Flags: approval-mozilla-beta+
Attachment #8346219 - Flags: approval-mozilla-aurora?
Attachment #8346219 - Flags: approval-mozilla-aurora+
Whiteboard: [Australis:P-][fixed-in-holly] → [Australis:P-][fixed-in-holly][good first verify]
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 on attachment 8359810 [details] [diff] [review]
Patch v1 - for Australis

Nearing the finish line with these, Gijs!
Attachment #8359810 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8359810 [details] [diff] [review]
Patch v1 - for Australis

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

r=me
Attachment #8359810 - Flags: review?(gijskruitbosch+bugs) → review+
Thanks! Landed on fx-team as https://hg.mozilla.org/integration/fx-team/rev/4bf3d999a962
Whiteboard: [Australis:P-][fixed-in-holly][good first verify] → [Australis:P-][fixed-in-holly][good first verify][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/4bf3d999a962
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P-][fixed-in-holly][good first verify][fixed-in-fx-team] → [Australis:P-][fixed-in-holly][good first verify]
Target Milestone: --- → Firefox 29
You need to log in before you can comment on or make changes to this bug.