(dormant account) Reporter
	Description • 13 years ago

about:telemetry should list all of the data that we send to the server. This includes ping metadata, probes and their descriptions.

	(dormant account) Reporter
	Comment 1 • 13 years ago

This should also allow users to opt out of specific telemetry probes. See https://bugzilla.mozilla.org/show_bug.cgi?id=668392#c24

	Asa Dotzler [:asa]
	Comment 2 • 13 years ago

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?

	(dormant account) Reporter
	Comment 3 • 13 years ago

(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.

	Dietrich Ayala (:dietrich)
	Comment 4 • 13 years ago

If you want to bundle the add-on, I've started a guide for that here:

https://wiki.mozilla.org/Firefox/BundlingAdd-ons

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 5 • 13 years ago

(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?

	Vladan Djeric (:vladan) Assignee
	Comment 6 • 13 years ago

> 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.

	Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]
	Comment 7 • 13 years ago

:vladan can we can an update on where this work is? We have an open item for this on the privacy review.

	Tim Taubert [:ttaubert] (inactive)
	Comment 10 • 12 years ago

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()

	Tim Taubert [:ttaubert] (inactive)
	Comment 11 • 12 years ago

Additional nit, please use strict mode for the JavaScript file.

	Vladan Djeric (:vladan) Assignee
	Comment 12 • 12 years ago

(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.

	Tim Taubert [:ttaubert] (inactive)
	Comment 14 • 12 years ago

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.

	Vladan Djeric (:vladan) Assignee
	Comment 15 • 12 years ago

(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

	Tim Taubert [:ttaubert] (inactive)
	Comment 19 • 12 years ago

This will need a security review as you'll add a new privileged about: page.

	Tim Taubert [:ttaubert] (inactive)
	Comment 20 • 12 years ago

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.

	Vladan Djeric (:vladan) Assignee
	Comment 21 • 12 years ago

(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?

	Tim Taubert [:ttaubert] (inactive)
	Comment 23 • 12 years ago

(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.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 24 • 12 years ago

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.

	Tim Taubert [:ttaubert] (inactive)
	Comment 25 • 12 years ago

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 :)

	Vladan Djeric (:vladan) Assignee
	Comment 26 • 12 years ago

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.

	Vladan Djeric (:vladan) Assignee
	Comment 27 • 12 years ago

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?

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 28 • 12 years ago

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.

	Simon Bennetts [:psiinon]
	Comment 29 • 12 years ago

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)?

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 30 • 12 years ago

Perhaps dveditz didn't see that a sec review had already been conducted when he added the request. ccing him for comment.

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 31 • 12 years ago

I spoke with dveditz who said the sec-review? was added as part of a mass update. Marking as sec-review+.

	Vladan Djeric (:vladan) Assignee
	Comment 33 • 12 years ago

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

See attached screenshots

	Vladan Djeric (:vladan) Assignee
	Comment 37 • 12 years ago

Builds here: https://tbpl.mozilla.org/?tree=Try&rev=02bff42645c3

	Tim Taubert [:ttaubert] (inactive)
	Comment 38 • 12 years ago

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.

	Tim Taubert [:ttaubert] (inactive)
	Comment 39 • 12 years ago

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 :)

	Vladan Djeric (:vladan) Assignee
	Comment 40 • 12 years ago

(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

	Tim Taubert [:ttaubert] (inactive)
	Comment 41 • 12 years ago

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!

	Vladan Djeric (:vladan) Assignee
	Comment 42 • 12 years ago

Comment on attachment 679439 [details] [diff] [review]
Add an about:telemetry page to Firefox, v6

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

	(no longer active)
	Comment 43 • 12 years ago

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