Last Comment Bug 661881 - Implement about:telemetry
: Implement about:telemetry
Status: RESOLVED FIXED
[sec-assigned:psiinon]
: dev-doc-needed
Product: Toolkit
Classification: Components
Component: Telemetry (show other bugs)
: unspecified
: All All
: -- normal with 7 votes (vote)
: mozilla19
Assigned To: Vladan Djeric (:vladan)
:
:
Mentors:
: 691689 (view as bug list)
Depends on: 661574 769055 1120160
Blocks: 659396 733105 768670
  Show dependency treegraph
 
Reported: 2011-06-03 11:07 PDT by (dormant account)
Modified: 2015-01-11 00:24 PST (History)
41 users (show)
lmandel: sec‑review+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Adds an about:telemetry page to Firefox (20.18 KB, patch)
2012-05-28 16:49 PDT, Vladan Djeric (:vladan)
no flags Details | Diff | Splinter Review
Adds an about:telemetry page to Firefox (20.59 KB, patch)
2012-05-28 16:53 PDT, Vladan Djeric (:vladan)
no flags Details | Diff | Splinter Review
Add an about:telemetry page to Firefox, v2 (20.39 KB, patch)
2012-06-01 16:22 PDT, Vladan Djeric (:vladan)
no flags Details | Diff | Splinter Review
Add an about:telemetry page to Firefox, v3 (19.26 KB, patch)
2012-06-14 13:31 PDT, Vladan Djeric (:vladan)
no flags Details | Diff | Splinter Review
Patch #3 screenshot (151.76 KB, image/png)
2012-06-14 13:32 PDT, Vladan Djeric (:vladan)
no flags Details
Patch #3 screenshot Right-to-Left (143.11 KB, image/png)
2012-06-14 13:33 PDT, Vladan Djeric (:vladan)
no flags Details
Add an about:telemetry page to Firefox, v4 (20.14 KB, patch)
2012-06-25 15:43 PDT, Vladan Djeric (:vladan)
ttaubert: review+
Details | Diff | Splinter Review
Add an about:telemetry page to Firefox, v5 (34.83 KB, patch)
2012-10-03 23:31 PDT, Vladan Djeric (:vladan)
ttaubert: feedback+
Details | Diff | Splinter Review
All sections collapsed (172.24 KB, image/jpeg)
2012-10-03 23:32 PDT, Vladan Djeric (:vladan)
no flags Details
Slow SQL section expanded (372.95 KB, image/jpeg)
2012-10-03 23:34 PDT, Vladan Djeric (:vladan)
no flags Details
Browser hangs section expanded (542.17 KB, image/jpeg)
2012-10-03 23:34 PDT, Vladan Djeric (:vladan)
no flags Details
Add an about:telemetry page to Firefox, v6 (37.37 KB, patch)
2012-11-07 16:24 PST, Vladan Djeric (:vladan)
ttaubert: review+
vladan.bugzilla: checkin+
Details | Diff | Splinter Review

Description (dormant account) 2011-06-03 11:07:15 PDT
about:telemetry should list all of the data that we send to the server. This includes ping metadata, probes and their descriptions.
Comment 1 (dormant account) 2011-10-27 14:51:57 PDT
This should also allow users to opt out of specific telemetry probes. See https://bugzilla.mozilla.org/show_bug.cgi?id=668392#c24
Comment 2 Asa Dotzler [:asa] 2011-10-27 16:55:36 PDT
Taras, is the plan here to take what's in the add-on and build that into Firefox directly? Or is there some other design we want? Can you link to the add-on here?
Comment 3 (dormant account) 2011-10-27 16:58:17 PDT
(In reply to Asa Dotzler [:asa] from comment #2)
> Taras, is the plan here to take what's in the add-on and build that into
> Firefox directly? Or is there some other design we want? Can you link to the
> add-on here?

Yes we'd start from the addon.
I would like to take the addon (https://addons.mozilla.org/en-US/firefox/addon/abouttelemetry/ or http://hg.mozilla.org/users/tglek_mozilla.com/perf-playground/file/1918b55a95cb/telemetry/addon), throw some UX love on it so we can link to it from telemetry prompt.
Comment 4 Dietrich Ayala (:dietrich) 2012-03-22 13:35:33 PDT
If you want to bundle the add-on, I've started a guide for that here:

https://wiki.mozilla.org/Firefox/BundlingAdd-ons
Comment 5 Lawrence Mandel [:lmandel] (use needinfo) 2012-03-23 12:43:30 PDT
(In reply to Dietrich Ayala (:dietrich) from comment #4)
> If you want to bundle the add-on, I've started a guide for that here:
> 
> https://wiki.mozilla.org/Firefox/BundlingAdd-ons

Vlad - Do you have enough information in Dietrich's doc to get started on this?
Comment 6 Vladan Djeric (:vladan) 2012-03-26 12:32:54 PDT
> Vlad - Do you have enough information in Dietrich's doc to get started on
> this?

I think so, I'll start looking into this project this week.
Comment 7 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2012-05-14 09:40:34 PDT
:vladan can we can an update on where this work is? We have an open item for this on the privacy review.
Comment 8 Vladan Djeric (:vladan) 2012-05-28 16:49:30 PDT
Created attachment 627815 [details] [diff] [review]
Adds an about:telemetry page to Firefox
Comment 9 Vladan Djeric (:vladan) 2012-05-28 16:53:52 PDT
Created attachment 627817 [details] [diff] [review]
Adds an about:telemetry page to Firefox
Comment 10 Tim Taubert [:ttaubert] 2012-05-29 04:29:35 PDT
Comment on attachment 627817 [details] [diff] [review]
Adds an about:telemetry page to Firefox

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

You're including only styles for Windows at the moment. I see there's gnomestripe/jar.mn modification, did you forgot to attach the other theme-specific files? aboutTelemetry.xhtml is missing as well.

Please read and apply our best practice rules for CSS in the Firefox frontend: https://wiki.mozilla.org/DevTools/CSSTips

I'll just add some nits for now without seeing the things as a whole. The JavaScript looks very functional and differs a lot from how other features are implemented nowadays. I'd like to see that code being refactored into a couple of objects that interact with each other. I think that would make it more readable and maintainable as well.

::: toolkit/content/aboutTelemetry.js
@@ +15,5 @@
> +const bundle = Services.strings.createBundle(
> +  "chrome://global/locale/aboutTelemetry.properties");
> +
> +const PREF_ENABLED = "toolkit.telemetry.enabled";
> +const DEBUG_SLOW_SQL = "toolkit.telemetry.debugSlowSql";

Nit: PREF_DEBUG_SLOW_SQL

@@ +16,5 @@
> +  "chrome://global/locale/aboutTelemetry.properties");
> +
> +const PREF_ENABLED = "toolkit.telemetry.enabled";
> +const DEBUG_SLOW_SQL = "toolkit.telemetry.debugSlowSql";
> +const HEIGHT = 18;

Nit: Which height? Of what element?

@@ +24,5 @@
> +    let belowEm = Math.round(HEIGHT * (value / max_value)*10)/10;
> +    let aboveEm = HEIGHT - belowEm;
> +    let bar = document.createElement("div");
> +    bar.className = "bar";
> +    // TODO: I can't set the style for createElement()ed elements :(

Really? I'm sure this should work. That would be a serious bug if not.

@@ +39,5 @@
> +  }
> +}
> +
> +function appendColumn(rowElement, colType, colText, className)
> +{

Nit: function () {

@@ +104,5 @@
> +    if (!re.exec(id)) {
> +      e.style.display = "none";
> +      continue;
> +    }
> +    e.style.display = "block";

> if (typeof id == "string")
>   e.style.display = re.exec(id) ? "block" : "none";

This would be a bit more readable and we wouldn't need to use continue at all.

@@ +106,5 @@
> +      continue;
> +    }
> +    e.style.display = "block";
> +  }
> +  window._searched = name;

Please use a global variable declared at the top for this.

@@ +117,5 @@
> +                                     if (input._lastValue == input.value)
> +                                       return;
> +                                     input._lastValue = input.value;
> +                                     do_search(input.value);
> +                                   }, 300);

Nit: That, um, doesn't look nice. Please use fewer indentation.

@@ +137,5 @@
> +    search.value = bundle.GetStringFromName("searchCaption");
> +    parent.appendChild(search);
> +    search.setSelectionRange(0, search.value.length);
> +    search.focus();
> +    search.onkeydown = incremental_search;

Nit: Please use addEventListener().

@@ +143,5 @@
> +
> +    parent.appendChild(document.createTextNode(" | "));
> +    let diff_button = document.createElement("button");
> +    diff_button.appendChild(document.createTextNode(bundle.GetStringFromName("diffCaption")));
> +    diff_button.onclick = diff;

Nit: addEventListener().

@@ +147,5 @@
> +    diff_button.onclick = diff;
> +    parent.appendChild(diff_button);
> +  } else {
> +    parent.appendChild(
> +      document.createTextNode(bundle.GetStringFromName("telemetryDisabled") + " "));

Instead of just linking to about:config, how about adding a button that flips the Telemetry flag? Or maybe open the 'Preferences > Advanced' pane?

@@ +172,5 @@
> +    if (!count)
> +      continue;
> +    if (first) {
> +      first = true;
> +      first = false;

o.O

@@ +194,5 @@
> +  function is_old(name, old_label, old_value) {
> +    if (!(name in old))
> +      return false;
> +    let old_hgram = unpackHistogram(old[name]);
> +    // return true

Nit: legacy code?

@@ +224,5 @@
> +    let slowSqlDiv = document.createElement("div");
> +    if (Telemetry.debugSlowSQL) {
> +      try {
> +        if (Services.prefs.getBoolPref(DEBUG_SLOW_SQL)) {
> +          sql = Telemetry.debugSlowSQL;

The whole try-catch block seems to exist only for Services.prefs.getBoolPref() but covers almost the whole function. Please use it only to read the pref value so that we don't swallow unexpected errors.

@@ +267,5 @@
> +
> +function do_load() {
> +  let body = document.getElementsByTagName("body")[0];
> +  addHeader(body);
> +  window._lastSnapshots = Telemetry.histogramSnapshots;

We don't normally construct caches like this. This should probably just be a global variable declared at the top of the document or maybe a helper function.

@@ +272,5 @@
> +  let content = generate(this._lastSnapshots);
> +  body.appendChild(content);
> +}
> +
> +window.onload = do_load;

Nit: please use window.addEventListener()
Comment 11 Tim Taubert [:ttaubert] 2012-05-29 06:26:04 PDT
Additional nit, please use strict mode for the JavaScript file.
Comment 12 Vladan Djeric (:vladan) 2012-06-01 16:20:57 PDT
(In reply to Tim Taubert [:ttaubert] from comment #10)

> You're including only styles for Windows at the moment. I see there's
> gnomestripe/jar.mn modification, did you forgot to attach the other
> theme-specific files? 

I'm not familiar with the front-end code and how this part of the build system works, so I copied about:support's setup. I think the changes I've made are enough for all the different platforms to include the same aboutTelemetry.css file. I tried out the Linux build and the CSS file is indeed present: 
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/vdjeric@mozilla.com-3521c4923e77/
Should I move aboutTelemetry.css to toolkit/content instead?

> aboutTelemetry.xhtml is missing as well.

I did forget to include this file

> Please read and apply our best practice rules for CSS in the Firefox
> frontend: https://wiki.mozilla.org/DevTools/CSSTips

Updated the CSS

> I'll just add some nits for now without seeing the things as a whole. The
> JavaScript looks very functional and differs a lot from how other features
> are implemented nowadays. I'd like to see that code being refactored into a
> couple of objects that interact with each other. I think that would make it
> more readable and maintainable as well.

I've removed Taras's diff/search functionality which simplifies the code. Let me know if you still want it re-factored.

> Instead of just linking to about:config, how about adding a button that
> flips the Telemetry flag? Or maybe open the 'Preferences > Advanced' pane?

It now opens the Advanced preferences panel.
Comment 13 Vladan Djeric (:vladan) 2012-06-01 16:22:45 PDT
Created attachment 629389 [details] [diff] [review]
Add an about:telemetry page to Firefox, v2
Comment 14 Tim Taubert [:ttaubert] 2012-06-06 08:17:58 PDT
Comment on attachment 629389 [details] [diff] [review]
Add an about:telemetry page to Firefox, v2

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

Thanks for diving into the world of front-end code, Vladan!

I know that this is most probably just the first iteration design-wise... Please bear with all my comments. This code needs some cleanup as it actually doesn't do a lot but it's very hard to read. I tried to give you some guidance for the specific improvements.

The page doesn't handle RTL mode at the moment, we need this. I didn't get the slow SQL tables to show (which might be a good thing) that's why I didn't say much about them. I need to try using my default profile for the trunk build.

Regarding the whole structure of this I'd like to see it separated into a couple of objects so that the onLoad function could look something like this:

> function onLoad() {
>   // do the header stuff and flip the <p>s
>
>   SlowSQL.render();
>   
>   for (let name in histogramSnapshots) {
>     let hgram = histogramSnapshots[name];
>     Histogram.render(hgram);
>   }
> }

Histogram might have the methods 'render', 'renderValues' and 'unpack'. SlowSQL the methods 'render', 'renderTable', 'renderTableHeader' and such. This should make the code a lot more readable.

(In reply to Vladan Djeric (:vladan) from comment #12)
> Should I move aboutTelemetry.css to toolkit/content instead?

Yes, we don't have any theme-specific code so we can remove the jar.mn modifications for the different themes and just include one single CSS file for now.

::: toolkit/content/aboutTelemetry.js
@@ +4,5 @@
> +
> +'use strict';
> +
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;

We don't need those two anymore with the change below.

@@ +8,5 @@
> +const Ci = Components.interfaces;
> +const Cu = Components.utils;
> +
> +const Telemetry =
> +  Cc["@mozilla.org/base/telemetry;1"].getService(Ci.nsITelemetry);

This is accessible using Services.telemetry.

@@ +31,5 @@
> +
> +    let aboveBar = document.createElement("div");
> +    aboveBar.className = "above";
> +    aboveBar.style.height = aboveEm + "em";
> +    barDiv.appendChild(aboveBar);

You can replace the whole 'aboveBar' thing with this:

> barDiv.style.paddingTop = aboveEm + "em";

@@ +58,5 @@
> +function getSqlTable(stats, isMainThread) {
> +  let div = document.createElement("div");
> +  if (Object.keys(stats).length == 0) {
> +    return div;
> +  }

Don't think we need this check if we do it properly before calling this method.

@@ +61,5 @@
> +    return div;
> +  }
> +
> +  let table = document.createElement("table");
> +  table.className = "slow-sql";

Looks like we don't need it for the <table>.

@@ +102,5 @@
> +
> +function addHeader(parent) {
> +  let telemetryEnabled = false;
> +  try {
> +    telemetryEnabled = Services.prefs.getBoolPref(PREF_ENABLED);

Maybe we should create a little helper func for this? Like getBoolPref(prefName, defaultValue)?

@@ +110,5 @@
> +
> +  if (telemetryEnabled) {
> +    parent.appendChild(
> +      document.createTextNode(bundle.GetStringFromName("telemetryEnabled")));
> +  } else {

We could insert those as paragraphs statically before </body> in the xhtml file, like:

> <p id="description-enabled">&aboutTelemetry.telemetryEnabled;</p>
> <p id="description-disabled" class="hidden">&aboutTelemetry.telemetryDisabled;</p>

And then all we do here is:

> if (getBoolPref(PREF_ENABLED, false)) {
>   document.getElementById("description-disabled").classList.add("hidden");
>   [...]
> } else {
>   [...]
> }

Also, please add an observer that flips the visibility of these paragraphs when the pref is changed.

@@ +117,5 @@
> +
> +    let preferencesLink = document.createElement("a");
> +    preferencesLink.href = "javascript:";
> +    preferencesLink.addEventListener(
> +      "click", function () { openAdvancedPreferences("generalTab"); document.location = "about:telemetry"; }, false);

Calling event.preventDefault() in the event listener prevents the browser from following the link.

@@ +158,5 @@
> +function generate(histogramSnapshots) {
> +  let content = document.createElement("div");
> +  content.id = "histograms";
> +
> +  let sql = Telemetry.slowSQL;

How about:

> let debugSlowSQL = getBoolPref(PREF_DEBUG_SLOW_SQL, false);
> let {mainThread, otherThreads} = Telemetry[debugSlowSQL ? "slowSQL" : "debugSlowSQL"];
> if (Object.keys(mainThread).length || Object.keys(otherThreads).length) {
>   if (debugSlowSQL)
>     document.getElementById("warning-slow-sql").classList.remove("hidden");
>   
>   // create tables...
> }

That way we could move the warning to be statically included in the xhtml as well, as a child of #histograms.

@@ +192,5 @@
> +    let div = document.createElement("div");
> +    div.className = "histogram";
> +    div.id = name;
> +    let divTitle = document.createElement("div");
> +    divTitle.className = "title";

Please make this class name "histogram-title".

@@ +198,5 @@
> +    div.appendChild(divTitle);
> +    let divStats = document.createElement("div");
> +    let stats = hgram.sample_count + " " + bundle.GetStringFromName("histogramSamples")
> +      + ", " + bundle.GetStringFromName("histogramAverage") + " = " + hgram.pretty_average
> +      + ", " + bundle.GetStringFromName("histogramSum") + " = " + hgram.sum;

Please move the bundle.GetStringFromName() calls before the loop.

@@ +214,5 @@
> +  let content = generate(Telemetry.histogramSnapshots);
> +  body.appendChild(content);
> +}
> +
> +window.addEventListener("load", do_load, false);

Please remove the event listener after the load happened.

::: toolkit/content/aboutTelemetry.xhtml
@@ +34,5 @@
> +    </h1>
> +
> +    <div>
> +      &aboutTelemetry.pageSubtitle;
> +    </div>

Please use: <h2>&aboutTelemetry.pageSubtitle;</h2>

@@ +36,5 @@
> +    <div>
> +      &aboutTelemetry.pageSubtitle;
> +    </div>
> +
> +    <br/>

We can remove this as we don't need it.

@@ +37,5 @@
> +      &aboutTelemetry.pageSubtitle;
> +    </div>
> +
> +    <br/>
> +

We should have <div id="histograms"> here so that in the JS file we can just inject content here.

::: toolkit/themes/winstripe/global/aboutTelemetry.css
@@ +1,4 @@
> +/* 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/. */
> +

> body {
>   padding: 10px;
> }

Gives the page a little more room to breathe.

> #histograms {
>   overflow: hidden;
> }

This wraps the floating histograms properly.

@@ +5,5 @@
> +.histogram {
> +  float: left;
> +  border-style: solid;
> +  border-color: gray;
> +  border-width: 1px;

border: 1px solid gray;

@@ +10,5 @@
> +  white-space: nowrap;
> +  padding: 10px;
> +}
> +
> +.histogram div:first-child {

This selector should be: .histogram-title

@@ +17,5 @@
> +  white-space: nowrap;
> +  overflow: hidden;
> +}
> +
> +.bar div.above {

(We don't need these rules anymore.)

@@ +23,5 @@
> +  width: 2em;
> +  border-width: 0;
> +}
> +
> +.bar div {

Let's call this .bar-inner

@@ +26,5 @@
> +
> +.bar div {
> +  background-color: blue;
> +  margin: 0;
> +  padding: 0;

We can remove the margin and padding, it's zero for divs anyway.

@@ +29,5 @@
> +  margin: 0;
> +  padding: 0;
> +  border-width: 1px;
> +  border-style: solid;
> +  border-color: #0000b0;

border: 1px solid #0000b0;

@@ +32,5 @@
> +  border-style: solid;
> +  border-color: #0000b0;
> +}
> +
> +.bar {

width: 2em;

@@ +39,5 @@
> +  float: left;
> +  font-family: monospace;
> +}
> +
> +th.slow-sql {

The selector and class name should better be: .header-slow-sql. Alternatively it could just be "th", we don't use <th> elements anywhere else on the page.

@@ +45,5 @@
> +  white-space: nowrap;
> +  text-align: left;
> +}
> +
> +caption.slow-sql {

The selector and class name should better be: .caption-slow-sql. Alternatively it could just be "caption" as we don't use other <caption> elements on the page.
Comment 15 Vladan Djeric (:vladan) 2012-06-14 13:30:45 PDT
(In reply to Tim Taubert [:ttaubert] from comment #14)
> The page doesn't handle RTL mode at the moment, we need this.

I attached a screenshot of the page in RTL mode using the ForceRTL extension. Let me know if anything looks like it needs to be changed.

>I didn't get the slow SQL tables to show (which might be a good thing) that's why I
> didn't say much about them. I need to try using my default profile for the
> trunk build.

See the screenshots. You can also get a build & create slow SQL statement by setting breakpoints on the following two lines:

http://mxr.mozilla.org/mozilla-central/source/storage/src/mozStorageConnection.cpp#882

http://mxr.mozilla.org/mozilla-central/source/storage/src/mozStorageConnection.cpp#958

Build: https://tbpl.mozilla.org/?tree=Try&rev=2c60950a5548
Comment 16 Vladan Djeric (:vladan) 2012-06-14 13:31:46 PDT
Created attachment 633253 [details] [diff] [review]
Add an about:telemetry page to Firefox, v3
Comment 17 Vladan Djeric (:vladan) 2012-06-14 13:32:57 PDT
Created attachment 633255 [details]
Patch #3 screenshot
Comment 18 Vladan Djeric (:vladan) 2012-06-14 13:33:29 PDT
Created attachment 633256 [details]
Patch #3 screenshot Right-to-Left
Comment 19 Tim Taubert [:ttaubert] 2012-06-25 04:40:02 PDT
This will need a security review as you'll add a new privileged about: page.
Comment 20 Tim Taubert [:ttaubert] 2012-06-25 05:09:34 PDT
Comment on attachment 633253 [details] [diff] [review]
Add an about:telemetry page to Firefox, v3

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

Looks quite good to me. I didn't find anything totally wrong besides the observer leak, just a couple of white space nits, DRY stuff and unnamed functions. As I mentioned below I didn't think of about:telemetry being in the toolkit and that it's used for other applications as well (e.g. Thunderbird). We should rather not directly open the pref dialog then because that might look totally different for other apps. I think a button to toggle the pref would do here for a first iteration. That's an easy way to turn on/off the feature.

::: toolkit/content/aboutTelemetry.js
@@ +33,5 @@
> +  return result;
> +}
> +
> +let observer = {
> +  observe: function (aSubject, aTopic, aData) {

observe: function observe(aSubject, aTopic, aData) {

@@ +38,5 @@
> +    if (aData == PREF_TELEMETRY_ENABLED) {
> +      // Notify user whether Telemetry is disabled
> +      if (getBoolPref(PREF_TELEMETRY_ENABLED, false)) {
> +        document.getElementById("description-enabled").classList.remove("hidden");
> +        document.getElementById("description-disabled").classList.add("hidden");

You can get these elements or classLists outside of the if-branches and save them to a variable.

@@ +52,5 @@
> +let SlowSQL = {
> +  /**
> +   * Render slow SQL statistics
> +   */
> +  render: function () {

render: function SlowSQL_render() {

(It helps a lot to have readable stack traces.)

@@ +65,5 @@
> +
> +      let slowSqlDiv = document.getElementById("slow-sql-tables");
> +
> +      // Main thread
> +      if (Object.keys(mainThread).length > 0) {

We should save this value to a variable ...

@@ +76,5 @@
> +        slowSqlDiv.appendChild(document.createElement("hr"));
> +      }
> +
> +      // Other threads
> +      if (Object.keys(otherThreads).length > 0) {

... and this, too.

@@ +94,5 @@
> +   *
> +   * @param aTable Parent table element
> +   * @param aTitle Table's title
> +   */
> +  renderTableHeader: function (aTable, aTitle) {

renderTableHeader: function SlowSQL_renderTableHeader(...) {

@@ +112,5 @@
> +   *
> +   * @param aTable Parent table element
> +   * @param aSql SQL stats object
> +   */
> +  renderTable: function (aTable, aSql) {

renderTable: function SlowSQL_renderTable(...) {

@@ +115,5 @@
> +   */
> +  renderTable: function (aTable, aSql) {
> +    for (let key in aSql) {
> +      let hitCount = aSql[key][0];
> +      let averageTime = aSql[key][1]/hitCount;

Nit: let averageTime = aSql[key][1] / hitCount;

@@ +133,5 @@
> +   * @param aRowElement Parent row element
> +   * @param aColType Column's tag name
> +   * @param aColText Column contents
> +   */
> +  appendColumn: function (aRowElement, aColType, aColText) {

appendColumn: function SlowSQL_appendColumn(...) {

@@ +156,5 @@
> +   * @param aParent Parent element
> +   * @param aName Histogram name
> +   * @param aHgram Histogram information
> +   */
> +  render: function (aParent, aName, aHgram) {

render: function Histogram_render(...) {

@@ +188,5 @@
> +   * @param aHgram Packed histogram
> +   *
> +   * @return Unpacked histogram representation
> +   */
> +  unpackHistogram: function(aHgram) {

unpack: function Histogram_unpack(...) {

(We could probably just call this 'unpack' because the object is already named 'Histogram'.)

@@ +189,5 @@
> +   *
> +   * @return Unpacked histogram representation
> +   */
> +  unpackHistogram: function(aHgram) {
> +    let sample_count = aHgram.counts.reduceRight(function (a,b) a+b);

Nit: let sample_count = aHgram.counts.reduceRight(function (a, b) a + b);

@@ +193,5 @@
> +    let sample_count = aHgram.counts.reduceRight(function (a,b) a+b);
> +    let buckets = [];
> +    if (aHgram.histogram_type == Telemetry.HISTOGRAM_BOOLEAN) {
> +      buckets = [0,1];
> +    } else {

You could do:

> let buckets = [0, 1];
> if (aHgram.histogram_type != Telemetry.HISTOGRAM_BOOLEAN)
>   buckets = aHgram.ranges;

@@ +210,5 @@
> +        continue;
> +      if (first) {
> +        first = false;
> +        if (i) {
> +          values.push([buckets[i-1], 0]);

Nit: values.push([buckets[i - 1], 0]);

@@ +217,5 @@
> +      last = i + 1;
> +      values.push([buckets[i], count]);
> +    }
> +    if (last && last < buckets.length) {
> +      values.push([buckets[last],0]);

Nit: values.push([buckets[last], 0]);

@@ +238,5 @@
> +   * @param aDiv Outer parent div
> +   * @param aValues Histogram values
> +   * @param aMaxValue Largest histogram value in set
> +   */
> +  renderValues: function(aDiv, aValues, aMaxValue) {

renderValues: function Histogram_renderValues(...) {

@@ +265,5 @@
> +  }
> +};
> +
> +function onLoad() {
> +  window.removeEventListener("load", onload);

window.removeEventListener("load", onLoad);

@@ +271,5 @@
> +  if (getBoolPref(PREF_TELEMETRY_ENABLED, false)) {
> +    document.getElementById("description-disabled").classList.add("hidden");
> +  } else {
> +    document.getElementById("description-enabled").classList.add("hidden");
> +  }

That's the same code as the observer has above, we could move that to a function to not repeat ourselves.

@@ +272,5 @@
> +    document.getElementById("description-disabled").classList.add("hidden");
> +  } else {
> +    document.getElementById("description-enabled").classList.add("hidden");
> +  }
> +  Services.prefs.addObserver(PREF_TELEMETRY_ENABLED, observer, false);

We need to remove the observer when the window unloads. Otherwise we'll leak the whole page!

@@ +277,5 @@
> +
> +  document.getElementById("options-link").addEventListener("click",
> +    function (aEvent) {
> +      openAdvancedPreferences("generalTab");
> +      aEvent.preventDefault();

It just occurred to me that the preference dialog will not be the same in different applications (like Thunderbird). Maybe we should just provide a button to toggle the pref itself?

@@ +286,5 @@
> +
> +  // Show histogram data
> +  let hgramDiv = document.getElementById("histograms");
> +  let histograms = Telemetry.histogramSnapshots;
> +  for (let name in histograms) {

for (let [name, hgram] in Iterator(histograms))

::: toolkit/content/aboutTelemetry.xhtml
@@ +35,5 @@
> +
> +    <p id="description-enabled">&aboutTelemetry.telemetryEnabled;</p>
> +    <p id="description-disabled">
> +      &aboutTelemetry.telemetryDisabled;
> +      <a id="options-link" href="about:blank">&aboutTelemetry.optionsLink;</a>

Nit: href="#" would probably be better (but not much).

Please put a full stop after the link so that the sentence looks nicer.

::: toolkit/content/jar.mn
@@ +16,5 @@
>  *  content/global/aboutRights-unbranded.xhtml (aboutRights-unbranded.xhtml)
>  *  content/global/aboutSupport.js
>  *  content/global/aboutSupport.xhtml
> +*  content/global/aboutTelemetry.js
> +*  content/global/aboutTelemetry.xhtml

Nit: We don't need to pre-process those files so you can remove the asterisk from the beginning of the line.
Comment 21 Vladan Djeric (:vladan) 2012-06-25 15:41:52 PDT
(In reply to Tim Taubert [:ttaubert] from comment #20)
> ::: toolkit/content/jar.mn
> @@ +16,5 @@
> >  *  content/global/aboutRights-unbranded.xhtml (aboutRights-unbranded.xhtml)
> >  *  content/global/aboutSupport.js
> >  *  content/global/aboutSupport.xhtml
> > +*  content/global/aboutTelemetry.js
> > +*  content/global/aboutTelemetry.xhtml
> 
> Nit: We don't need to pre-process those files so you can remove the asterisk
> from the beginning of the line.

aboutTelemetry.xhtml needs to be pre-processed to remove the copyright block in the header. I guess I could use an HTML comment instead?
Comment 22 Vladan Djeric (:vladan) 2012-06-25 15:43:11 PDT
Created attachment 636513 [details] [diff] [review]
Add an about:telemetry page to Firefox, v4
Comment 23 Tim Taubert [:ttaubert] 2012-06-25 15:44:12 PDT
(In reply to Vladan Djeric (:vladan) from comment #21)
> aboutTelemetry.xhtml needs to be pre-processed to remove the copyright block
> in the header. I guess I could use an HTML comment instead?

Oh, I see. I think you could just leave it being pre-processed.
Comment 24 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-25 17:20:12 PDT
Please switch to the commented-XML version - preprocessing just to strip license headers is kind of a waste of build cycles, particularly now that the header is only two lines.
Comment 25 Tim Taubert [:ttaubert] 2012-06-26 08:55:47 PDT
Comment on attachment 636513 [details] [diff] [review]
Add an about:telemetry page to Firefox, v4

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

Looks solid to me! r=me with the nits below fixed and the question answered.

As the current design is obviously not ready for prime-time... Are all the necessary people involved and aware of this feature? As it's just a URL that might be passed on by users it might be a little 'too ugly' to have it enabled by default? Do we maybe want to consider pref'ing it off for now? Should we overhaul its design before landing it? Is it okay and acceptable to land it right away? AFAIK, Telemetry has been driven by mostly technical people so far (please correct me if I'm wrong) and I wondered what's the strategy for it.

::: toolkit/content/aboutTelemetry.js
@@ +83,5 @@
> +        this.renderTableHeader(table, title);
> +        this.renderTable(table, mainThread);
> +
> +        slowSqlDiv.appendChild(table);
> +        slowSqlDiv.appendChild(document.createElement("hr"));

How about moving this to a method? Seems to be about the same for both branches.

::: toolkit/content/jar.mn
@@ +16,5 @@
>  *  content/global/aboutRights-unbranded.xhtml (aboutRights-unbranded.xhtml)
>  *  content/global/aboutSupport.js
>  *  content/global/aboutSupport.xhtml
> +   content/global/aboutTelemetry.js
> +*  content/global/aboutTelemetry.xhtml

What Gavin said :)
Comment 26 Vladan Djeric (:vladan) 2012-06-26 15:32:16 PDT
Our initial plan was to land about:telemetry as is on Nightly and then the UX people could clean it up to meet our style guidelines. This plan has now changed and we'll wait for the UX redesign before landing it.

Also, I just realized we should probably show users all of the Telemetry data we gather, including the boring stuff like hardware specs, startup timings, etc. I'll put up another patch that includes that info later in the week.
Comment 27 Vladan Djeric (:vladan) 2012-08-16 10:06:01 PDT
Should we hold off on sec-review until I add the rest of the Telemetry info (e.g. hardware configuration, startup times, etc) to the about page?
Comment 28 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-17 12:14:12 PDT
That doesn't seem like something that should block security-review. I don't know why this needs explicit security-review at all, really, since it's just exposing telemetry data that we already collect, AIUI.
Comment 29 Simon Bennetts [:psiinon] 2012-08-22 03:00:32 PDT
OK, now I'm confused :)
The sec review for this was 769055 which I marked as resolved.
I thought that the development was small, well defined and relatively safe so just had a call with the developers rather than going for a full review.
Documented here (also linked off the sec bug): https://wiki.mozilla.org/Security/Reviews/AboutTelemetry

Am I missing something, or have I got the process wrong (which wouldnt surprise me)?
Comment 30 Lawrence Mandel [:lmandel] (use needinfo) 2012-08-22 08:14:56 PDT
Perhaps dveditz didn't see that a sec review had already been conducted when he added the request. ccing him for comment.
Comment 31 Lawrence Mandel [:lmandel] (use needinfo) 2012-09-11 08:32:57 PDT
I spoke with dveditz who said the sec-review? was added as part of a mass update. Marking as sec-review+.
Comment 32 Nathan Froyd [:froydnj] 2012-09-12 01:08:54 PDT
*** Bug 691689 has been marked as a duplicate of this bug. ***
Comment 33 Vladan Djeric (:vladan) 2012-10-03 23:31:23 PDT
Created attachment 667822 [details] [diff] [review]
Add an about:telemetry page to Firefox, v5

- Added "Chrome Hangs", "Simple Measurements", "System Information", "Addon Histograms" sections
- Applied UX feedback

See attached screenshots
Comment 34 Vladan Djeric (:vladan) 2012-10-03 23:32:47 PDT
Created attachment 667823 [details]
All sections collapsed
Comment 35 Vladan Djeric (:vladan) 2012-10-03 23:34:06 PDT
Created attachment 667824 [details]
Slow SQL section expanded
Comment 36 Vladan Djeric (:vladan) 2012-10-03 23:34:55 PDT
Created attachment 667825 [details]
Browser hangs section expanded
Comment 37 Vladan Djeric (:vladan) 2012-10-04 11:07:13 PDT
Builds here: https://tbpl.mozilla.org/?tree=Try&rev=02bff42645c3
Comment 38 Tim Taubert [:ttaubert] 2012-10-30 03:09:01 PDT
Comment on attachment 667822 [details] [diff] [review]
Add an about:telemetry page to Firefox, v5

At a first glance some quick usability issues with this:

1) Should we show empty sections at all? I think we could hide them or maybe do something like: "Browser Hangs (empty)".

2) I think probably the whole div.data-section should be clickable and give a little hint by using 'cursor: pointer'?

You could maybe divide the page into <section>s with their own <header> or <h1> elements. That would make the code a little more correct, semantically.

Apart from that the page looks good to me. Still need to take a look at the code.
Comment 39 Tim Taubert [:ttaubert] 2012-10-30 06:44:25 PDT
Comment on attachment 667822 [details] [diff] [review]
Add an about:telemetry page to Firefox, v5

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

This looks good! Lots of nits I'd like to see fixed but nothing really wrong with this patch.

::: toolkit/content/aboutTelemetry.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +'use strict';
> +
> +Components.utils.import("resource://gre/modules/Services.jsm");

Instead of using the whole thing all the time you could just do:

const Ci = Components.interfaces;                                                
const Cc = Components.classes;                                                   
const Cu = Components.utils;

@@ +28,5 @@
> + */
> +function getPref(aPrefName, aDefault) {
> +  let result = aDefault;
> +  try {
> +    if (typeof aDefault == "boolean") {

Woah... that's... pretty tricky :) I think we should rather use Services.prefs.getPrefType().

@@ +50,5 @@
> +  /**
> +   * Observer is called whenever Telemetry is enabled or disabled
> +   */
> +  observe: function observe(aSubject, aTopic, aData) {
> +    if (aData == PREF_TELEMETRY_ENABLED) {

We don't actually need this check but it doesn't hurt, I guess...

@@ +151,5 @@
> +   * @param aTable Parent table element
> +   * @param aSql SQL stats object
> +   */
> +  renderTable: function SlowSQL_renderTable(aTable, aSql) {
> +    for (let key in aSql) {

for (let [hitCount, averageTime] of aSql) { ...

[You of course still have to do (averageTime / hitCount).]

@@ +207,5 @@
> +      document.getElementById("fetch-symbols").classList.add("hidden");
> +      return;
> +    }
> +
> +    for (let hangIndex in hangs) {

A for (let i = 0; ...)  loop would probably better here as that wouldn't iterate over unexpected properties.

@@ +261,5 @@
> +  renderMemoryMap: function ChromeHangs_renderMemoryMap(aDiv, aMap) {
> +    aDiv.appendChild(document.createTextNode(this.memoryMapTitle));
> +    aDiv.appendChild(document.createElement("br"));
> +
> +    for (let moduleIndex in aMap) {

for (let currentModule of aMap) { ...

@@ +285,5 @@
> +    this.symbolRequest.setRequestHeader("Content-type", "application/json");
> +    this.symbolRequest.setRequestHeader("Content-length", chromeHangsJSON.length);
> +    this.symbolRequest.setRequestHeader("Connection", "close");
> +
> +    this.symbolRequest.onreadystatechange = this.onReadyStateChange;

Why not:

this.symbolRequest.onreadystatechange = this.handleSymbolResponse.bind(this);

@@ +305,5 @@
> +    document.getElementById("fetch-symbols").classList.add("hidden");
> +    document.getElementById("hide-symbols").classList.remove("hidden");
> +
> +    if (this.symbolRequest.readyState != 4)
> +      return;

Shouldn't this be at the top of the function so that we don't fetch elements and add/remove classes multiple times?

@@ +324,5 @@
> +      return;
> +    }
> +
> +    let hangs = Telemetry.chromeHangs;
> +    for (let reportIndex in jsonResponse) {

I'd also like a for (let i = 0; ...) loop here.

@@ +385,5 @@
> +    let divStats = document.createElement("div");
> +    divStats.appendChild(document.createTextNode(stats));
> +    outerDiv.appendChild(divStats);
> +
> +    if (window.getComputedStyle(document.body).direction == "rtl") {

Histogram.render is called multiple times. How about moving this to a separate function that caches the value?

@@ +450,5 @@
> +   * @param aValues Histogram values
> +   * @param aMaxValue Largest histogram value in set
> +   */
> +  renderValues: function Histogram_renderValues(aDiv, aValues, aMaxValue) {
> +    for each ([label, value] in aValues) {

for (let [label, value] of aValues) { ...

(for each is deprecated.)

@@ +515,5 @@
> +   * @param aTable Table element
> +   * @param aMeasurements Key/value map
> +   */
> +  renderBody: function KeyValueTable_renderBody(aTable, aMeasurements) {
> +    for (let key in aMeasurements) {

for (let [key, value] in Iterator(aMeasurements)) { ...

@@ +539,5 @@
> +/**
> + * Initializes load/unload, pref change and mouse-click listeners
> + */
> +function setupListeners() {
> +  window.removeEventListener("load", onLoad);

This should be moved to the onLoad() function.

@@ +574,5 @@
> +      if (aElement.className.indexOf("hidden") >= 0) {
> +        aElement.classList.remove("hidden");
> +      } else {
> +        aElement.classList.add("hidden");
> +      }

You can use aElement.classList.toggle("hidden");

@@ +588,5 @@
> +  }
> +
> +  // Clicking on the section name will toggle its state
> +  let sectionHeaders = document.getElementsByClassName("section-name");
> +  for (let index = 0; index < sectionHeaders.length; index++) {

for (let sectionHeader of sectionHeaders) { ...

@@ +594,5 @@
> +  }
> +
> +  // Clicking on the "collapse"/"expand" text will also toggle section's state
> +  let toggleLinks = document.getElementsByClassName("toggle-caption");
> +  for (let index = 0; index < toggleLinks.length; index++) {

Same here.

::: toolkit/content/aboutTelemetry.xhtml
@@ +35,5 @@
> +
> +      <button id="toggle-telemetry" type="button"/>
> +    </div>
> +
> +    <div class="data-section">

Like I said in the previous comment, I think these should be <section>s.

::: toolkit/locales/en-US/chrome/global/aboutTelemetry.dtd
@@ +9,5 @@
> +  This information is submitted anonymously to Mozilla to help improve &brandShortName;.
> +">
> +
> +<!ENTITY aboutTelemetry.telemetryEnabled "
> +  Telemetry is <span id='highlight-telemetry-enabled'>enabled</span>.

I don't care so much about the HTML but assigning an ID attribute in here feels wrong to me. I know that's a little effort but I think you should assign a class to the container and then use that to assign different colors.

::: toolkit/locales/en-US/chrome/global/aboutTelemetry.properties
@@ +15,5 @@
> +# Note to translators: Please leave the question marks in the translated string.
> +# They will be replaced with values programmaticaly, e.g. "Hang Report #3 (5 seconds)"
> +# The single question mark is replaced with the number of the hang.
> +# The double question mark will be replaced with the duration of the hang.
> +hangTitle = Hang Report #? (?? seconds)

Please use '%S' as placeholder here. You can then use bundle.formatStringFromName() to replace that. That's how we do it in the rest of the browser :)
Comment 40 Vladan Djeric (:vladan) 2012-11-07 16:24:58 PST
Created attachment 679439 [details] [diff] [review]
Add an about:telemetry page to Firefox, v6

(In reply to Tim Taubert [:ttaubert] from comment #38)
> Comment on attachment 667822 [details] [diff] [review]
> 2) I think probably the whole div.data-section should be clickable and give
> a little hint by using 'cursor: pointer'?

Making the whole section clickable would make it tricky to copy-paste text from this page

Applied the rest of the review comments
Comment 41 Tim Taubert [:ttaubert] 2012-11-08 06:58:31 PST
Comment on attachment 679439 [details] [diff] [review]
Add an about:telemetry page to Firefox, v6

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

Thank you, this looks good!

::: toolkit/content/aboutTelemetry.js
@@ +638,5 @@
> +function colorElement(aElementID, aClassName) {
> +  let outerElement = document.getElementById(aElementID);
> +  let wordElement = outerElement.getElementsByTagName("span")[0];
> +  wordElement.classList.add(aClassName);
> +}

I actually meant to apply the class to the outerElement and then have a CSS rule like:

> .highlight-enabled > span { color: red }

If you could fix this last little issue, that would be great!
Comment 42 Vladan Djeric (:vladan) 2012-11-08 12:36:50 PST
Comment on attachment 679439 [details] [diff] [review]
Add an about:telemetry page to Firefox, v6

https://hg.mozilla.org/integration/mozilla-inbound/rev/c0713f3e03f6

Note You need to log in before you can comment on or make changes to this bug.