943606 - Add simple measurement to BrowserUITelemetry for toolbar contents

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

We're interested in collecting:

* The list of default items that are not in their default toolbars
* The list of default items that are in the palette
* The list of non-default items that are not in the palette

We'll be checking for only the built-in default and non-default items. Items supplied by add-ons will be skipped.

Comment 1

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

Attachment: Patch v1

Comment 2

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

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

I'm actually OK with this, taking a second glance. Taking out of WIP.

What do you think of this, Dao?

Comment 3

[Dão Gottwald [:dao]]

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

>+const kDefaultToolbarSets = {
>+  'toolbar-menubar': ['menubar-items'],
>+  'nav-bar': ['unified-back-forward-button',
>+              'urlbar-container',
>+              'reload-button',
>+              'stop-button',
>+              'search-container',
>+              'webrtc-status-button',
>+              'bookmarks-menu-button',
>+              'downloads-button',
>+              'home-button',
>+              'window-controls'],
>+  'PersonalToolbar': ['personal-bookmarks'],
>+  'TabsToolbar': [
>+#ifndef CAN_DRAW_IN_TITLEBAR
>+                  'appmenu-toolbar-button',
>+#endif
>+                  'tabbrowser-tabs',
>+                  'new-tab-button',
>+                  'alltabs-button',
>+                  'tabs-closebutton'],
>+  'addon-bar': ['addonbar-closebutton',
>+                'spring',
>+                'status-bar'],
>+};
>+
>+const kPaletteItems = [
>+  'print-button',
>+  'history-button',
>+  'bookmarks-button',
>+  'new-window-button',
>+  'fullscreen-button',
>+  'zoom-controls',
>+  'feed-button',
>+  'cut-button',
>+  'copy-button',
>+  'paste-button',
>+  'sync-button',
>+  'navigator-throbber',
>+  'tabview-button',
>+];

Isn't that data available elsewhere in the code base? The prospect of having to maintain these lists here doesn't seem very attractive.

Please use double quotes.

Comment 4

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

(In reply to Dão Gottwald [:dao] from comment #3)
> Isn't that data available elsewhere in the code base? The prospect of having
> to maintain these lists here doesn't seem very attractive.
> 

I was originally planning on using the defaultset of each toolbar, but this can be mucked about with by add-ons (and I've seen that done, as a way of getting a toolbar item into a toolbar).

I know of no other authoritative place by which we define what items are in which toolbars by default. :/ It's not ideal, but this code is also not going to persist after Australis is released.

> Please use double quotes.

Can do.

Comment 5

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

Attachment: Patch v1.1

Single quotes replaced by double quotes.

Comment 6

[Dão Gottwald [:dao]]

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

>+  "toolbar-menubar": ["menubar-items"],

We're probably not interested in this on OS X.

>+const kPaletteItems = [

>+XPCOMUtils.defineLazyGetter(this, "gDefaultItems", function() {

I don't like the different prefixes (k and g) in this module. The only difference is that some objects are constructed differently, but having to think about that when referencing these objects is annoying. You don't really care whether they're constant or not; you're not supposed to modify any of them anyway. You could stop declaring the constants as const and consistently use the g prefix or no prefix at all (maybe making the first letter uppercase), or you could use the ALL_UPPERCASE naming convention that's also fine for constants.

>+      let currentset = toolbar.currentSet.split(',');

double quotes

>+      for (let item of currentset) {
>+        // Is this a default item?
>+        if (gDefaultItems.indexOf(item) != -1) {
>+          // Ok, it's a default item - but is it in its default
>+          // toolbar?
>+          if (kDefaultToolbarSets[toolbarID].indexOf(item) != -1) {
>+            // The item is in its default toolbar
>+            keptDefault.push(item);
>+          } else {
>+            movedDefault.push(item);

What if an item was moved to a toolbar that's not in kDefaultToolbarSets?

>+    result.keptDefault = keptDefault;
>+    result.movedDefault = movedDefault;
>+    result.addedNonDefault = addedNonDefault;
>+    result.removedDefault = removedDefault;

defaultKept, defaultMoved, nondefaultAdded and defaultRemoved would probably sort the results more nicely.

Comment 7

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

Attachment: Patch v1.2

Comment 8

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

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

(In reply to Dão Gottwald [:dao] from comment #6)
> Comment on attachment 8340015 [details] [diff] [review]
> Patch v1.1
> 
> >+  "toolbar-menubar": ["menubar-items"],
> 
> We're probably not interested in this on OS X.

Good call. I've put this behind an ifdef.

> 
> >+const kPaletteItems = [
> 
> >+XPCOMUtils.defineLazyGetter(this, "gDefaultItems", function() {
> 
> I don't like the different prefixes (k and g) in this module. The only
> difference is that some objects are constructed differently, but having to
> think about that when referencing these objects is annoying. You don't
> really care whether they're constant or not; you're not supposed to modify
> any of them anyway. You could stop declaring the constants as const and
> consistently use the g prefix or no prefix at all (maybe making the first
> letter uppercase), or you could use the ALL_UPPERCASE naming convention
> that's also fine for constants.
> 

Alright, I've opted for ALL_UPPERCASE from here on out in this module.

> >+      let currentset = toolbar.currentSet.split(',');
> 
> double quotes
> 

Fixed.

> >+      for (let item of currentset) {
> >+        // Is this a default item?
> >+        if (gDefaultItems.indexOf(item) != -1) {
> >+          // Ok, it's a default item - but is it in its default
> >+          // toolbar?
> >+          if (kDefaultToolbarSets[toolbarID].indexOf(item) != -1) {
> >+            // The item is in its default toolbar
> >+            keptDefault.push(item);
> >+          } else {
> >+            movedDefault.push(item);
> 
> What if an item was moved to a toolbar that's not in kDefaultToolbarSets?
> 

Good question. I've altered the patch to query for all toolbars matching toolbar[customizable=true]. That seems to take care of this case.


> >+    result.keptDefault = keptDefault;
> >+    result.movedDefault = movedDefault;
> >+    result.addedNonDefault = addedNonDefault;
> >+    result.removedDefault = removedDefault;
> 
> defaultKept, defaultMoved, nondefaultAdded and defaultRemoved would probably
> sort the results more nicely.

Good idea. Fixed.

Comment 9

[:Gijs (he/him)]

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

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

Bunch of nits, but this generally looks alright. The only really dire thing is the spring, which shouldn't be in there. :-)

::: browser/modules/BrowserUITelemetry.jsm
@@ +150,5 @@
> +              "webrtc-status-button",
> +              "bookmarks-menu-button",
> +              "downloads-button",
> +              "home-button",
> +              "window-controls"],

Nit: can you make all these multiline list have the [] on their own line and make all the contents line up across this block?

@@ +161,5 @@
> +                  "new-tab-button",
> +                  "alltabs-button",
> +                  "tabs-closebutton"],
> +  "addon-bar": ["addonbar-closebutton",
> +                "spring",

This is going to be problematic. Springs have a generated IDs, and so the ID will never match. I think you should leave this out, and discount generated items entirely. In order to do the latter, you don't need to do anything else in the code below, AFAICT, as they'll be presumed to be add-on originated, which means they don't get taken into account. You may want to update the comment there, though.

@@ +165,5 @@
> +                "spring",
> +                "status-bar"],
> +};
> +
> +const PALETTE_ITEMS = [

Nit: this should be a Set, IMO, even if the other items can't easily be in order to concat everything.

@@ +177,5 @@
> +  "cut-button",
> +  "copy-button",
> +  "paste-button",
> +  "sync-button",
> +  "navigator-throbber",

Have we considered that this used to be in the default set, so might still be in old profiles?

@@ +183,5 @@
> +];
> +
> +XPCOMUtils.defineLazyGetter(this, "DEFAULT_ITEMS", function() {
> +  let result = [];
> +  for (let toolbarId in DEFAULT_TOOLBAR_SETS) {

Nit:

for (let [, buttons] of Iterator(DEFAULT_TOOLBAR_SETS)) {
  result = result.concat(buttons);
}

@@ +295,5 @@
> +
> +    let toolbars = document.querySelectorAll("toolbar[customizable=true]");
> +    for (let toolbar of toolbars) {
> +      let toolbarID = toolbar.id;
> +      let currentset = toolbar.currentSet.split(",");

In my experience with add-ons, I would not trust toolbar.currentSet to not be null.
Please use an intermediate to check, though, because non-messed with it is a getter and not super-cheap. Id est:

let currentset = toolbar.currentSet;
currentset = (currentset && currentset.split(",")) || [];

@@ +302,5 @@
> +        // Is this a default item?
> +        if (DEFAULT_ITEMS.indexOf(item) != -1) {
> +          // Ok, it's a default item - but is it in its default
> +          // toolbar?
> +          if (toolbarID in DEFAULT_TOOLBAR_SETS &&

I will be an annoying add-on and define <toolbar id="toString" customizable="true"/> because that's how I roll.

How about: Array.isArray(DEFAULT_TOOLBAR_SETS[toolbarID]) ? :-)

(that'll be false for null/undefined/etc.)

@@ +320,5 @@
> +
> +    // Now go through the items in the palette to see what default
> +    // items are in there.
> +    let defaultRemoved = [];
> +    let paletteChildren = win.gNavToolbox.palette.childNodes;

let defaultRemoved = [node.id for (let node of paletteChildren) if (DEFAULT_ITEMS.indexOf(node.id) != -1)];

Comment 10

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

Thanks - made all changes except for the Set suggestion, because (as mentioned in IRC), there are later patches coming that need to concatenate that Array, and Sets don't concatenate easily.

https://hg.mozilla.org/projects/holly/rev/3831bd01eb41

Comment 11

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

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

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

None. This just gives BrowserUITelemetry the capability of recording the contents of the toolbars and palettes in a privacy-conscious way.


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 12

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

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

Comment 13

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

Landed on mozilla-beta as https://hg.mozilla.org/releases/mozilla-beta/rev/c587a37ab70b