Closed Bug 949224 Opened 6 years ago Closed 6 years ago

Collect UITelemetry on how many items are in the addon bar

Categories

(Firefox :: Toolbars and Customization, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
Future
Tracking Status
firefox27 --- fixed
firefox28 --- fixed

People

(Reporter: mconley, Assigned: mconley)

References

Details

(Whiteboard: [Non-Australis-Only][Australis:P-][fixed-in-holly][qa-])

Attachments

(1 file)

No description provided.
Whiteboard: [Non-Australis-Only]
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.
Attachment #8346235 - Flags: review?(jaws) → review+
For example, I put a separator in my toolbar on Firefox 25:
> document.getElementById("nav-bar").childNodes[7].id
> /*
> separator13868236745522
> */
(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
Ah, yes that makes sense. Sorry for my confusion. Carry on here :)
(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!
Whiteboard: [Non-Australis-Only] → [Non-Australis-Only][Australis:P-]
Landed on Holly as https://hg.mozilla.org/projects/holly/rev/c82eab28b9c7
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Future
Whiteboard: [Non-Australis-Only][Australis:P-] → [Non-Australis-Only][Australis:P-][fixed-in-holly]
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.
Attachment #8346235 - Flags: approval-mozilla-beta?
Attachment #8346235 - Flags: approval-mozilla-aurora?
Attachment #8346235 - Flags: approval-mozilla-beta?
Attachment #8346235 - Flags: approval-mozilla-beta+
Attachment #8346235 - Flags: approval-mozilla-aurora?
Attachment #8346235 - Flags: approval-mozilla-aurora+
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.
Whiteboard: [Non-Australis-Only][Australis:P-][fixed-in-holly] → [Non-Australis-Only][Australis:P-][fixed-in-holly][qa-]
You need to log in before you can comment on or make changes to this bug.