949224 - Collect UITelemetry on how many items are in the addon bar

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Comment 1

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

Attachment: Patch v1

Comment 2

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

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

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

r=me with the isSpecialToolbarItem function fixed. On a sidenote, it would be nice if we had tests for this... :)

::: browser/modules/BrowserUITelemetry.jsm
@@ +626,5 @@
> + */
> +function isSpecialToolbarItem(aItemID) {
> +  return aItemID == "spring" ||
> +         aItemID == "spacer" ||
> +         aItemID == "separator";

The IDs of special items are unique, so this won't work. http://mxr.mozilla.org/mozilla-central/source/browser/components/customizableui/src/CustomizableUI.jsm#931 checks to see if the ID _startsWith_ those values.

Comment 3

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

For example, I put a separator in my toolbar on Firefox 25:
> document.getElementById("nav-bar").childNodes[7].id
> /*
> separator13868236745522
> */

Comment 4

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

(In reply to Jared Wein [:jaws] (Away 20 Dec to 2 Jan) from comment #3)
> For example, I put a separator in my toolbar on Firefox 25:
> > document.getElementById("nav-bar").childNodes[7].id
> > /*
> > separator13868236745522
> > */

Yes, this is totally true - but I'm not iterating by node IDs. I'm iterating the split currentset from the toolbar and iterating the individual springs.

Here's the currentset from a toolbar that has some special items:

spacer,spring,spacer,personal-bookmarks,paste-button,separator

Comment 5

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

Ah, yes that makes sense. Sorry for my confusion. Carry on here :)

Comment 6

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

(In reply to Jared Wein [:jaws] (Away 20 Dec to 2 Jan) from comment #5)
> Ah, yes that makes sense. Sorry for my confusion. Carry on here :)

No problem at all.

(In reply to Jared Wein [:jaws] (Away 20 Dec to 2 Jan) from comment #2)
> Comment on attachment 8346235 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 8346235 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the isSpecialToolbarItem function fixed. On a sidenote, it would
> be nice if we had tests for this... :)

Yeah, totally. :/ Had a need-for-speed on these probes, which meant I couldn't spend time writing tests. Now that I'm reaching the end of the must-land-on-Aurora probes, I can start writing the tests. I have a bug open for that (bug 944481). Thanks for the review!

Comment 7

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

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

Comment 8

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

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

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

None. Adds capability for BrowserUITelemetry to read in how many non-default items have been added to the add-on bar.


User impact if declined: 

None.


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

All manual.


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

Very low.


String or IDL/UUID changes made by this patch:

None.

Comment 9

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

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

Comment 10

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

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

Comment 11

[u279076]

This fix will not be manually verified by QA. If you believe this warrants extra QA attention please nominate for testing by removing this whiteboard tag and adding the verifyme keyword. Please also provide any details you have that may inform our testing.