Closed Bug 942244 Opened 6 years ago Closed 6 years ago

Add BrowserUiTelemetry module for collecting toolbar measurements

Categories

(Firefox :: Toolbars and Customization, defect)

x86
All
defect
Not set

Tracking

()

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

People

(Reporter: mconley, Assigned: mconley)

References

Details

(Whiteboard: [fixed-in-holly][qa-])

Attachments

(2 files, 6 obsolete files)

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.
Note that because these measurements are targeted at non-Australis Firefox, I'm applying the patches to the Holly branch.
Attached patch Patchv 1 (obsolete) — Splinter Review
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.
Assignee: nobody → mconley
Status: NEW → ASSIGNED
Attached patch WIP Patch 2 (obsolete) — Splinter Review
Removing the toolbar event count thing for now - trying to keep the patches small and somewhat atomic.
Attachment #8336927 - Attachment is obsolete: true
Attached patch Patch v1 (obsolete) — Splinter Review
Ok, this successfully shows up in about:telemetry.
Attachment #8336949 - Attachment is obsolete: true
Blocks: 942274
Comment on attachment 8336962 [details] [diff] [review]
Patch v1

How do you feel about reviewing this, Blair?
Attachment #8336962 - Flags: review?(bmcbride)
Attached patch Patch v1.1 (obsolete) — Splinter Review
Simplifying the initialization.
Attachment #8336962 - Attachment is obsolete: true
Attachment #8336962 - Flags: review?(bmcbride)
Attachment #8337079 - Flags: review?(bmcbride)
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?
(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?
Flags: needinfo?(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.
Flags: needinfo?(dao)
(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?
Flags: needinfo?(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?
Flags: needinfo?(dao)
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.
Attachment #8337079 - Flags: review?(bmcbride) → review-
(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?
Flags: needinfo?(bmcbride)
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?
Flags: needinfo?(bmcbride)
(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.
Attached patch Patch v1.2 (obsolete) — Splinter Review
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...
Attachment #8337079 - Attachment is obsolete: true
Comment on attachment 8338690 [details] [diff] [review]
Patch v1.2

Gijs volunteered to look at this today.
Attachment #8338690 - Flags: review?(gijskruitbosch+bugs)
Whiteboard: [Australis:P1] → [Australis:P1][Non-Australis-Only]
Whoops, put that on the wrong bug...
Whiteboard: [Australis:P1][Non-Australis-Only] → [Australis:P1]
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?
(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.
Attached patch Patch v1.3 (obsolete) — Splinter Review
What do you think of this, Dao?
Attachment #8338690 - Attachment is obsolete: true
Attachment #8338690 - Flags: review?(gijskruitbosch+bugs)
Attachment #8338711 - Flags: review?(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
Attachment #8338711 - Flags: review?(dao) → review+
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.
Attachment #8338711 - Attachment is obsolete: true
Attachment #8339452 - Flags: review+
Non-Australis variant landed in Holly: https://hg.mozilla.org/projects/holly/rev/bd11e7e8a63e

I'll request Aurora uplift when it turns green.
Whiteboard: [Australis:P1] → [Australis:P1][fixed-in-holly]
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.
Attachment #8339479 - Flags: review+
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.
Attachment #8339452 - Flags: approval-mozilla-aurora?
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.
Attachment #8339452 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Whiteboard: [Australis:P1][fixed-in-holly] → [Australis:P1][fixed-in-holly][fixed-in-fx-team]
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.
Attachment #8339452 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/db0be93f358d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P1][fixed-in-holly][fixed-in-fx-team] → [Australis:P1][fixed-in-holly]
Target Milestone: --- → Firefox 29
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.
Attachment #8339452 - Flags: approval-mozilla-beta?
Removing Australis:P1 whiteboard tag because these already block an Australis:P1 bug, and being redundant isn't helpful.
Whiteboard: [Australis:P1][fixed-in-holly] → [fixed-in-holly]
Attachment #8339452 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [fixed-in-holly] → [fixed-in-holly][qa-]
You need to log in before you can comment on or make changes to this bug.