Closed Bug 949277 Opened 6 years ago Closed 6 years ago

Track usage of about: pages

Categories

(Firefox for Metro Graveyard :: General, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: sfoster, Unassigned)

Details

Attachments

(2 files, 2 obsolete files)

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
Whiteboard: [triage]
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.
Whiteboard: [triage] → [release28]
Assignee: nobody → sfoster
Blocks: metrov1it21
No longer blocks: metrov1backlog
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
Whiteboard: [release28] → [beta28] p=3
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.
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.
Attachment #8349693 - Flags: review?(mconley)
Builds on attachment 8349693 [details] [diff] [review], adds the about.js script to record page loads of those about: pages within toolkit/content
Attachment #8349698 - Flags: review?(mconley)
Builds on attachment 8349693 [details] [diff] [review], adds the about.js script to record page loads of those about: pages within browser/metro
Attachment #8349700 - Flags: review?(mbrubeck)
Attachment #8349700 - Flags: review?(mbrubeck) → review+
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).
Attachment #8349693 - Flags: review?(mconley) → review-
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?
Attachment #8349698 - Flags: review?(mconley) → review+
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
Attachment #8349693 - Flags: review-
Comment on attachment 8349698 [details] [diff] [review]
Add about.js script to toolkit/content's about: pages to record page accesses

See above.
Attachment #8349698 - Flags: review+ → feedback+
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.
Attachment #8349693 - Attachment is obsolete: true
Attachment #8349698 - Attachment is obsolete: true
Hmm, looks like this patch conflicts with the one in bug 948879 (attachment 8349774 [details] [diff] [review]).
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.
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.
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.
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.
Assignee: sfoster → nobody
Blocks: metrov1backlog
No longer blocks: metrov1it21
Status: ASSIGNED → NEW
Priority: P2 → --
QA Contact: jbecerra
Whiteboard: [beta28] p=3 → [triage]
No longer blocks: metrov1backlog
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
Whiteboard: [triage]
You need to log in before you can comment on or make changes to this bug.