949277 - Track usage of about: pages

Status: RESOLVED WONTFIX

(Firefox for Metro Graveyard :: General, defect)

Description

[Sam Foster [:sfoster] (he/him)]

It would help to have some data to inform decisions about future work on about: pages (e.g see 937234). A telemetry probe in each of the about: pages should suffice

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Bug 932092 recently landed and allows JS code to add not only "events" (counters) for when some action happens, it also allows us to track "sessions" or how long we spend using a specific feature.

We could use "events" to track the number of times we enter an about: page, but we coudl also use "sessions" to track how long a user spends on an about: page.

Something to think about. The Android team is working with UX to start using these probes. We could get Yuan and Ian to start talking about Metro too. The concepts should be the same.

Comment 2

[Sam Foster [:sfoster] (he/him)]

Discussed in IRC with mconley and irving. We made a provisional decision to group this data under a  "about-page-accesses" key, under the UITelemetry simple measures. 

I'm now having 2nd thoughts about whether we should record just the access count, or load time (time to DOMContentLoaded) or similar. But, assuming page accesses gives us the data we need initially, I'll keep it minimal and simple for now.

Comment 3

[Sam Foster [:sfoster] (he/him)]

Attachment: New AboutPageUtils module and about.js script to record page loads

This is just the module and script that will provide the data collection mechanism to the various about pages. A content.js <script> will get added to each page we want to implement this, in separate patches by component.

Comment 4

[Sam Foster [:sfoster] (he/him)]

Attachment: Add about.js script to toolkit/content's about: pages to record page accesses

Builds on attachment 8349693 [details] [diff] [review], adds the about.js script to record page loads of those about: pages within toolkit/content

Comment 5

[Sam Foster [:sfoster] (he/him)]

Attachment: Add about.js script to browser/metro's about: pages to record page accesses

Builds on attachment 8349693 [details] [diff] [review], adds the about.js script to record page loads of those about: pages within browser/metro

Comment 6

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

Comment on attachment 8349693 [details] [diff] [review]
New AboutPageUtils module and about.js script to record page loads

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

The idea is sound, but I just have some stylistic / implementation suggestions. See below.

::: toolkit/content/about.js
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +'use strict';

Double quotes please.

::: toolkit/modules/AboutPageUtils.jsm
@@ +17,5 @@
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "UITelemetry",
> +                                  "resource://gre/modules/UITelemetry.jsm");
> +
> +let consoleService = Cc["@mozilla.org/consoleservice;1"].

If we use Log.jsm (see below), this will no longer be needed.

@@ +21,5 @@
> +let consoleService = Cc["@mozilla.org/consoleservice;1"].
> +     getService(Components.interfaces.nsIConsoleService);
> +
> +this.AboutPageUtils = {
> +  _inited: false,

"inited" -> "initted"

@@ +30,5 @@
> +    // register function to provide simple measures data for TelemetryPing
> +    try {
> +      UITelemetry.addSimpleMeasureFunction("about-page-accesses", () => this._pageAccessCount);
> +    } catch (ex) {
> +      // already registered, swallow exception

If you're logging, we might want to WARN here.

@@ +32,5 @@
> +      UITelemetry.addSimpleMeasureFunction("about-page-accesses", () => this._pageAccessCount);
> +    } catch (ex) {
> +      // already registered, swallow exception
> +    }
> +    consoleService.logStringMessage("/AboutPageUtils init");

This looks like debug-only noise that probably shouldn't be emitted normally.

Some options:

1) Just flat-out remove it.
2) Use something like Log.jsm to log the message[1]
3) Have some global boolean set to false normally that you can set to true when doing development, and a log
   function that only writes to the console or stdout when that boolean is set to true.

[1]:  Incomplete doc: https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Log.jsm
      Example usage: https://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Sqlite.jsm

I think #2 is probably the best.

@@ +37,5 @@
> +    this._inited = true;
> +  },
> +
> +  _pageAccessCount: {},
> +  recordPageAccess: function(url) {

aUrl

@@ +42,5 @@
> +    if (!this._inited) {
> +      this.init();
> +    }
> +
> +    let count = (url in this._pageAccessCount) ? this._pageAccessCount[url] :

The formatting here is a little hard to read. I think I'd prefer:

if (!(aUrl in this._pageAccessCount)) {
  this._pageAccessCount[aUrl] = 0;
}

this._pageAccessCount[aUrl]++;

@@ +48,5 @@
> +    this._pageAccessCount[url] = ++count;
> +    consoleService.logStringMessage("AboutPageUtils recordPageAccess for url: "+url+": "+ count);
> +  }
> +};
> +

What you can do instead of putting an if (!this._initted) guard for each function, is to just call init as soon as the module is loaded.

You can do that by putting AboutPageUtils.init(); at the bottom of this function. This will ensure that by the time something needs AboutPageUtils, it'll have initted, and it will only init once (you won't need to guard against re-entry).

Comment 7

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

Comment on attachment 8349698 [details] [diff] [review]
Add about.js script to toolkit/content's about: pages to record page accesses

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

I'm guessing the about: pages from browser will need to be done too?

Comment 8

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

Comment on attachment 8349693 [details] [diff] [review]
New AboutPageUtils module and about.js script to record page loads

It's occurred to me that I'm not a toolkit reviewer - so while I can give feedback on this stuff, you'll need somebody from this list to give the final OK:

https://wiki.mozilla.org/Modules/Toolkit

Comment 9

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

Comment on attachment 8349698 [details] [diff] [review]
Add about.js script to toolkit/content's about: pages to record page accesses

See above.

Comment 10

[Sam Foster [:sfoster] (he/him)]

Attachment: toolkit pieces: New AboutPageUtils module , about.js script to record page loads, modify toolkit/content's about: pages to record page accesses

I addressed review comments and folded the 2 toolkit bits together. I just removed the debug/log stuff - I had missed a qrefresh and I don't see any real value in logging here. For the init() call in AboutPageUtils, I made it "private" and added the inline call. As all it does is wrapped in a try/catch (UITelemetry.addSimpleMeasureFunction throws if the a function with that key is already registered) I think there's no risk here. 

Yes, I'm preparing a separate catch for the other about: pages in browser/base/content. And I'm sure there are more. My goal is to hit everything in about:about.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

Hmm, looks like this patch conflicts with the one in bug 948879 (attachment 8349774 [details] [diff] [review]).

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8350285 [details] [diff] [review]
toolkit pieces: New AboutPageUtils module , about.js script to record page loads, modify toolkit/content's about: pages to record page accesses

I suppose you should just rename your about.js to something else (aboutTracker.js?).

I'm somewhat skeptical that this will give us actionable data (numbers from telemetry are skewed, and raw counts don't really paint the full picture, etc.), but I suppose it doesn't much hurt.

Comment 13

[Sam Foster [:sfoster] (he/him)]

Ah yeah it would conflict with that. As to the value, I have started to have my own doubts as the scope has grown. I would worry less if it meant touching fewer files, so I'm going to see if there's another approach perhaps using the AboutRedirector(s). I may also push it back into the backlog to let it stew if there's no clear winning solution. As you say, its all about actionable data.

Comment 14

[Sam Foster [:sfoster] (he/him)]

I backed up and jotted down the end goal as a user story: 

"As a (Metro/whatever) browser user, I need to be able to access information about my browser - local and specific to my use and particular browser install - and clearly distinguish it from similar data I might find online."

Its something like that. But the key point is about presenting "system", local data differently to web content to avoid any confusion - of which the about: pages are one example. Given that, I think its clear that usage patterns and telemetry to gather them isn't the answer here. 

I propose WONTFIXing this bug and creating a new story along these lines with work tickets as necessary to design and implement the solution. Sorry for the noise people.

Comment 15

[Marco Mucci [:MarcoM]]

Hey Sam, thanks very much for the update and recommendation.  I'm going to remove it from the Iteration and return it to the Backlog for evaluation at the next team meeting.

(In reply to Sam Foster [:sfoster] from comment #14)
> I backed up and jotted down the end goal as a user story: 
> 
> "As a (Metro/whatever) browser user, I need to be able to access information
> about my browser - local and specific to my use and particular browser
> install - and clearly distinguish it from similar data I might find online."
> 
> Its something like that. But the key point is about presenting "system",
> local data differently to web content to avoid any confusion - of which the
> about: pages are one example. Given that, I think its clear that usage
> patterns and telemetry to gather them isn't the answer here. 
> 
> I propose WONTFIXing this bug and creating a new story along these lines
> with work tickets as necessary to design and implement the solution. Sorry
> for the noise people.