942244 - Add BrowserUiTelemetry module for collecting toolbar measurements

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

Bug 940807 modifies the UITelemetry JSM to allow it to collect simple measurements.

Now we need a module specialized for Desktop Firefox for gathering things like toolbar customization event counts, toolbar counts, etc, to feed into these simple measurements.

Comment 1

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

Note that because these measurements are targeted at non-Australis Firefox, I'm applying the patches to the Holly branch.

Comment 2

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

Attachment: Patchv 1

This adds a simple BrowserUiTelemetry JSM, which inits on browser startup, and should return a simple measure of whether or not the add-on bar is enabled.

Just a simple start, and we can build up the measurements afterward.

Comment 3

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

Attachment: WIP Patch 2

Removing the toolbar event count thing for now - trying to keep the patches small and somewhat atomic.

Comment 4

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

Attachment: Patch v1

Ok, this successfully shows up in about:telemetry.

Comment 5

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

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

How do you feel about reviewing this, Blair?

Comment 6

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

Attachment: Patch v1.1

Simplifying the initialization.

Comment 7

[Dão Gottwald [:dao]]

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

>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -138,16 +138,19 @@ XPCOMUtils.defineLazyModuleGetter(this, 
> XPCOMUtils.defineLazyModuleGetter(this, "SessionStore",
>   "resource:///modules/sessionstore/SessionStore.jsm");
> 
> #ifdef MOZ_CRASHREPORTER
> XPCOMUtils.defineLazyModuleGetter(this, "TabCrashReporter",
>   "resource:///modules/TabCrashReporter.jsm");
> #endif
> 
>+XPCOMUtils.defineLazyModuleGetter(this, "BrowserUiTelemetry",
>+  "resource:///modules/BrowserUiTelemetry.jsm");
>+
> let gInitialPages = [
>   "about:blank",
>   "about:newtab",
>   "about:home",
>   "about:privatebrowsing",
>   "about:welcomeback",
>   "about:sessionrestore"
> ];
>@@ -927,16 +930,18 @@ var gBrowserInit = {
>   _delayedStartup: function(mustLoadSidebar) {
>     let tmp = {};
>     Cu.import("resource://gre/modules/TelemetryTimestamps.jsm", tmp);
>     let TelemetryTimestamps = tmp.TelemetryTimestamps;
>     TelemetryTimestamps.add("delayedStartupStarted");
> 
>     this._cancelDelayedStartup();
> 
>+    Cu.import("resource:///modules/BrowserUiTelemetry.jsm", tmp);

replace this line with BrowserUiTelemetry.init(); ...

>+++ b/browser/modules/BrowserUiTelemetry.jsm

>+BrowserUiTelemetry.init();

... and remove this.

>+    // If there are no such windows, we're out of luck. :(
>+    if (!win) {
>+      Cu.reportError("BrowserUiTelemetry could not find a browser window to analyze.");
>+      return {};
>+    }

Why should this be reported as an error?

Comment 8

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

(In reply to Dão Gottwald [:dao] from comment #7)
> Comment on attachment 8337079 [details] [diff] [review]
> Patch v1.1
> 
> >--- a/browser/base/content/browser.js
> >+++ b/browser/base/content/browser.js
> >@@ -138,16 +138,19 @@ XPCOMUtils.defineLazyModuleGetter(this, 
> > XPCOMUtils.defineLazyModuleGetter(this, "SessionStore",
> >   "resource:///modules/sessionstore/SessionStore.jsm");
> > 
> > #ifdef MOZ_CRASHREPORTER
> > XPCOMUtils.defineLazyModuleGetter(this, "TabCrashReporter",
> >   "resource:///modules/TabCrashReporter.jsm");
> > #endif
> > 
> >+XPCOMUtils.defineLazyModuleGetter(this, "BrowserUiTelemetry",
> >+  "resource:///modules/BrowserUiTelemetry.jsm");
> >+
> > let gInitialPages = [
> >   "about:blank",
> >   "about:newtab",
> >   "about:home",
> >   "about:privatebrowsing",
> >   "about:welcomeback",
> >   "about:sessionrestore"
> > ];
> >@@ -927,16 +930,18 @@ var gBrowserInit = {
> >   _delayedStartup: function(mustLoadSidebar) {
> >     let tmp = {};
> >     Cu.import("resource://gre/modules/TelemetryTimestamps.jsm", tmp);
> >     let TelemetryTimestamps = tmp.TelemetryTimestamps;
> >     TelemetryTimestamps.add("delayedStartupStarted");
> > 
> >     this._cancelDelayedStartup();
> > 
> >+    Cu.import("resource:///modules/BrowserUiTelemetry.jsm", tmp);
> 
> replace this line with BrowserUiTelemetry.init(); ...
> 

This will result in me attempting to init the BrowserUiTelemetry module each time per window, which means having to modify BrowserUiTelemetry to guard against re-entry on init.

With the original method, I guarantee that initialize is only run once - when the module is first imported.

I'm curious - what advantage is there to changing this?

Comment 9

[Dão Gottwald [:dao]]

Oh, I thought this was in nsBrowserGlue.js, not browser.js. Why is it in browser.js to begin with?

The advantage would be less magical, easier to follow behavior.

Comment 10

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

(In reply to Dão Gottwald [:dao] from comment #9)
> Oh, I thought this was in nsBrowserGlue.js, not browser.js. Why is it in
> browser.js to begin with?
> 

Planning ahead - each window is going to have to get event listeners hooked up for things like the appmenu-button, so they're going to have to get passed in to BrowserUiTelemetry at some point - I figured we'd do it around here. Does that sound reasonable to you?

Comment 11

[Dão Gottwald [:dao]]

Presumably BrowserUiTelemetry.jsm could do this automatically after being initialized, by observing browser-delayed-startup-finished for instance. Does browser.js need to know about that at all?

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

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

*twitch* BrowserUITelemetry please. Please. PLEASE.

What Dao said - browser.js shouldn't have to worry about initializing the module.

::: browser/modules/BrowserUiTelemetry.jsm
@@ +29,5 @@
> +    });
> +
> +    // If there are no such windows, we're out of luck. :(
> +    if (!win) {
> +      Cu.reportError("BrowserUiTelemetry could not find a browser window to analyze.");

And if the module is initializing itself, this shouldn't be possible.

Comment 13

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

(In reply to Blair McBride [:Unfocused] from comment #12)
> Comment on attachment 8337079 [details] [diff] [review]
> Patch v1.1
> 
> Review of attachment 8337079 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> *twitch* BrowserUITelemetry please. Please. PLEASE.
> 
> What Dao said - browser.js shouldn't have to worry about initializing the
> module.
> 
> ::: browser/modules/BrowserUiTelemetry.jsm
> @@ +29,5 @@
> > +    });
> > +
> > +    // If there are no such windows, we're out of luck. :(
> > +    if (!win) {
> > +      Cu.reportError("BrowserUiTelemetry could not find a browser window to analyze.");
> 
> And if the module is initializing itself, this shouldn't be possible.

Couldn't the TelemetryPing event occur if only the OS X hidden window is open? Or if there's only a single pop-up open?

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

If the app has started with no browser window open, the module would have no data to report to telemetry anyway. If the app is started with a browser window, and thenthe window is closed, the module will still be alive to report any data it collected from that window to telemetry. Or am I missing something?

Comment 15

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

(In reply to Blair McBride [:Unfocused] from comment #14)
> If the app has started with no browser window open, the module would have no
> data to report to telemetry anyway. If the app is started with a browser
> window, and thenthe window is closed, the module will still be alive to
> report any data it collected from that window to telemetry. Or am I missing
> something?

There are some measurements that are only taken at the time of the ping - for example, the boolean to indicate whether or not the add-on bar is open. Without a window / document, we can't take that measurement.

Comment 16

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

Attachment: Patch v1.2

I've moved the initialization into nsBrowserGlue.js as suggested.

I've kept the window check, since I'm not 100% convinced that it's the right call just yet... what if a private browsing window is the only window around when the TelemetryPing comes? win would be null in that case, and we probably want to bail out there...

Comment 17

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

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

Gijs volunteered to look at this today.

Comment 18

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

Whoops, put that on the wrong bug...

Comment 19

[Dão Gottwald [:dao]]

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

>+const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;

You don't seem to use these, except for Cu, which you only use here:

>+Cu.import("resource://gre/modules/Services.jsm");
>+Cu.import("resource://gre/modules/XPCOMUtils.jsm");

>+    // If there are no such windows, we're out of luck. :(
>+    if (!win) {
>+      Cu.reportError("BrowserUITelemetry could not find a browser window to analyze.");
>+      return {};
>+    }

Why is this reported as an error?

Comment 20

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

(In reply to Dão Gottwald [:dao] from comment #19)
> Comment on attachment 8338690 [details] [diff] [review]
> Patch v1.2
> 
> >+const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
> 
> You don't seem to use these, except for Cu, which you only use here:
> 
> >+Cu.import("resource://gre/modules/Services.jsm");
> >+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> 

Good point! I'll remove the others.

> >+    // If there are no such windows, we're out of luck. :(
> >+    if (!win) {
> >+      Cu.reportError("BrowserUITelemetry could not find a browser window to analyze.");
> >+      return {};
> >+    }
> 
> Why is this reported as an error?

Good question - I guess that doesn't give us a whole lot of value, does it? I'll remove it.

Comment 21

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

Attachment: Patch v1.3

What do you think of this, Dao?

Comment 22

[Dão Gottwald [:dao]]

Comment on attachment 8338711 [details] [diff] [review]
Patch v1.3

>+this.BrowserUITelemetry = {
>+  init: function() {
>+    UITelemetry.addSimpleMeasureFunction("toolbars", this.getToolbarMeasures);

this.getToolbarMeasures.bind(this) should probably be used here

Comment 23

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

Attachment: Patch v1.4 (r+'d by Dao) - for non-Australis

Thanks Dao!

Unless there are any objections, I'm not going to land this for Australis builds until we start building Australis-related probes.

I will, however, land it on Holly and request Aurora uplift.

Comment 24

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

Non-Australis variant landed in Holly: https://hg.mozilla.org/projects/holly/rev/bd11e7e8a63e

I'll request Aurora uplift when it turns green.

Comment 25

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

Attachment: Patch v1.4 (r+'d by Dao) - for Australis

This basically registers a no-op with UITelemetry.jsm. I'll land this once we have some Australis-friendly probes to lay on top of it.

Comment 26

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

Comment on attachment 8339452 [details] [diff] [review]
Patch v1.4 (r+'d by Dao) - for non-Australis

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

None. This will add the browser-specific module for UITelemetry gathering, and adds our first probe (whether or not the add-on bar is collapsed).


User impact if declined: 

None.


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

Manual testing, plus a green tree on Holly (well, green minus the intermittent oranges that are unrelated to this patch).


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

Very little.


String or IDL/UUID changes made by this patch:

None.

Comment 27

[bhavana bajaj [:bajaj]]

Comment on attachment 8339452 [details] [diff] [review]
Patch v1.4 (r+'d by Dao) - for non-Australis

Requesting to verify the measurements are collected as expected.

Comment 28

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

Finally was able to get a probe landed for Australis (bug 944114) - so had a good reason to land the Australis version of this patch on mozilla-central:

https://hg.mozilla.org/integration/fx-team/rev/db0be93f358d

Comment 29

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

Comment on attachment 8339452 [details] [diff] [review]
Patch v1.4 (r+'d by Dao) - for non-Australis

I've also realized that this never landed on Aurora (which has now become Beta).

I'll re-request approval for beta shortly.

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/db0be93f358d

Comment 31

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

Comment on attachment 8339452 [details] [diff] [review]
Patch v1.4 (r+'d by Dao) - for non-Australis

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

None - this just adds the BrowserUITelemetry module needed to gather counts on various events happening in the UI as part of the Australis measuring project.

This was originally requested to land on Aurora, but missed landing before the uplift.


User impact if declined: 

None.


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

Manual testing, and having baked on Holly / Aurora for several days.


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

None.


String or IDL/UUID changes made by this patch:

None.

Comment 32

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

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

Comment 33

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

Landed in mozilla-beta as https://hg.mozilla.org/releases/mozilla-beta/rev/083dd14d4659.