1172459 - The about:telemetry environment section should render addons & plugins better

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P4)

Description

[Georg Fritzsche [:gfritzsche]]

The about:telemetry environment section currently shows odd & hard-to-read values for the "Name" fields.

It would be much better to add a new subsection for "activeAddons", "activePlugins", "activeGMPlugins", "activeExperiment" & "theme".

For "activeAddons", "activePlugins" & "activeGMPlugins" we can show the data much better per addon/plugin with a structure like this:

> activeAddons
>   <addon id> ------------
>   blocklisted | false
>   description | Just some addon
>   ...
>   <addon id> ------------
>   ...
> 
> activePlugins
>   <plugin name> ---------
>   version     | 1.0
>   blocklisted | true
>   ...

The rendering for the environment happens here:
https://hg.mozilla.org/mozilla-central/annotate/7d4ab4a9febd/toolkit/content/aboutTelemetry.js#l490

We can filter out the mentioned subsections there and do special rendering for them.

Comment 1

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

I'm going to be putting more telemetry into the NPAPI stuff in Bug 722110, might be worth fixing this at the same time since I'll need to visualize that somehow anyways.

Comment 2

[Penh Lenh]

I'd like to work on it, please assign to me.

Comment 3

[Georg Fritzsche [:gfritzsche]]

Thanks, done.

Comment 4

[Penh Lenh]

Attachment: bug-1172459-fix.patch

Hi, Please review this patch.

Comment 5

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8691829 [details] [diff] [review]
bug-1172459-fix.patch

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

Thanks for the patch!
I wanted to try this out to see how it looks, but it doesn't apply properly to my source here.
Is this based on another patch?

::: toolkit/content/aboutTelemetry.js
@@ -527,5 @@
>  
>      let data = sectionalizeObject(ping.environment);
>  
>      for (let [section, sectionData] of data) {
> -      let subSection = document.createElement("section");

I think the code here will be much clearer if we:
* basically leave this section as-is
* skip the "addons" section here
* add additional code after this loop to handle the "addons" section

That way we can avoid adding special cases for the addons rendering and instead treat them as separate cases.

Looking at the documentation for the data here:
http://gecko.readthedocs.org/en/latest/toolkit/components/telemetry/telemetry/environment.html

"persona" is just a string, so it's easy to add from "environment.addons.persona".

"theme", "activeExperiment" could just be key/value tables, we could use e.g.:
let addons = environment.addons;
for (let section of [addons.theme, addons.activeExperiment]) {
  let data = sectionalizeObject(section);
  ...
}

For "activeAddons" and "activePlugins" it gets a little more interesting, but we can display them both with a similar approach.
This would be nice to move into separate functions.

Addons can be captioned by their id, plugins with their name, so we could e.g. end up with something like:
for (let id of Object.keys(environment.activeAddons)) {
  let addon = environment.activeAddons[id];
  // add the addon with id and its entries
  // ...
}

for (let plugin of environment.activePlugins) {
  // add the plugin with plugin.name and its entries
  // ...
}

Comment 6

[Penh Lenh]

Attachment: bug-1172459-fix-v2.patch

I have updated the patch accordingly. Please review again.

Comment 7

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8692545 [details] [diff] [review]
bug-1172459-fix-v2.patch

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

Thanks, this looks very good now!
Sorry, i was occupied end of last week, following up here now.

High-level, looking at my about:telemetry with this patch:
* the addon subsections need some margin between them, they are really close to each other: http://imgur.com/9c0gX0g
* for activeAddons, activePlugins & activeGMPlugins we want to clearly show the addon/plugin name or id the values belong to - e.g. like this: http://imgur.com/AZ9E2J8
* the theme subsection shows some properties as empty, but i can see it has values in the raw data: http://imgur.com/sEMgXxQ

::: toolkit/content/aboutTelemetry.js
@@ +207,5 @@
>  
> +/**
> + * This returns a concatenate string of the map object Map { 0: "T", 1: "h", 2: "e" }
> + */
> +function getValueObject(obj) {

You can use the existing function explodeObject() instead.
This should make things also look nicer, as it e.g. turns arrays into strings like "[1, 2, 3]".

@@ +543,5 @@
>  
>      let data = sectionalizeObject(ping.environment);
>  
>      for (let [section, sectionData] of data) {
> +      if (section == "addons") break;

Minor: Let's stay with the coding style here:
if (section == "addons") {
  break;
}

...

@@ +549,5 @@
>        let caption = document.createElement("caption");
>        caption.appendChild(document.createTextNode(section + "\n"));
>        table.appendChild(caption);
>  
> +      this.appendHeading(table);

Nice cleanup.

@@ +563,5 @@
>      }
> +
> +    this.addonSection(dataDiv);
> +
> +    let dataAddon = document.getElementById("addons-data");

I think this is more readable as:
let addonSection = this.createAddonSection(dataDiv);
...

@@ +569,5 @@
> +    let addons = ping.environment.addons;
> +
> +    this.renderKeyObjectAddons(addons.activeAddons, dataAddon, "activeAddons");
> +    this.renderActivePlugins(addons.activePlugins, dataAddon, "activePlugins");
> +    this.renderKeyValue(addons.theme, dataAddon, "themes");

The name should be "theme" - we only record the single active theme in this data.

@@ +577,5 @@
>    },
>  
> +  renderPersona: function(addonObj, dataAddon, sectionTitle) {
> +      let table = document.createElement("table");
> +      table.setAttribute("class", sectionTitle);

I think we should use "id" here, we only have one of each of those sections per document.

@@ +584,5 @@
> +      let persona = addonObj == null ? "null" : addonObj.persona
> +
> +      let row = document.createElement("tr");
> +      this.appendColumn(row, "td", "persona\t");
> +      this.appendColumn(row, "td", persona + "\t");

I don't think we should put "\t" there or similar.
If we need specialized margins or padding, we should do it in CSS.

@@ +592,5 @@
> +  },
> +
> +  renderActivePlugins: function(addonObj, dataAddon, sectionTitle) {
> +      let table = document.createElement("table");
> +      table.setAttribute("class", sectionTitle);

Lets use "id".

@@ +600,5 @@
> +      for (let plugin of addonObj) {
> +        for(let name in plugin) {
> +          let row = document.createElement("tr");
> +          this.appendColumn(row, "td", name + "\t");
> +          this.appendColumn(row, "td", plugin[name] + "\t");

No "\t" needed here.

@@ +610,5 @@
> +  },
> +
> +  renderKeyObjectAddons: function(addonObj, dataAddon, sectionTitle) {
> +      let table = document.createElement("table");
> +      table.setAttribute("class", sectionTitle);

Lets use "id".

@@ +621,5 @@
> +
> +        for(let key in addon) {
> +          let row = document.createElement("tr");
> +          this.appendColumn(row, "td", key + "\t");
> +          this.appendColumn(row, "td", addon[key] + "\t");

No "\t" needed here.

@@ +630,5 @@
> +      dataAddon.appendChild(table);
> +  },
> +
> +  renderKeyValue: function(addonObj, dataAddon, sectionTitle) {
> +    for (let section of [addonObj]) {

This is a loop that always run only once - [addonObj] is an array with the single content of |addonObj|.
I think this outer for-loop here is redundant?

@@ +641,5 @@
> +
> +      for (let [path, value] of data) {
> +        let row = document.createElement("tr");
> +        this.appendColumn(row, "td", path + "\t");
> +        this.appendColumn(row, "td", getValueObject(value) + "\t");

No "\t" needed here.

@@ +652,5 @@
> +
> +  appendAddonID: function(table, addonID) {
> +    let row = document.createElement("tr");
> +    this.appendColumn(row, "td", "id\t");
> +    this.appendColumn(row, "td", addonID + "\t");

No "\t" needed here.

@@ +659,5 @@
> +
> +  appendHeading: function(table) {
> +    let headings = document.createElement("tr");
> +    this.appendColumn(headings, "th", bundle.GetStringFromName("environmentDataHeadingName") + "\t");
> +    this.appendColumn(headings, "th", bundle.GetStringFromName("environmentDataHeadingValue") + "\t");

No "\t" needed here.

@@ +665,5 @@
> +  },
> +
> +  appendAddonTitle: function(section, table) {
> +    let caption = document.createElement("caption");
> +    caption.appendChild(document.createTextNode(section + "\n"));

No "\n" needed here.

@@ +670,5 @@
> +    table.appendChild(caption);
> +  },
> +
> +  addonSection: function(dataDiv) {
> +    let tableAddon = document.createElement("table");

I don't think we need to use a table here - a simple div would be enough.

@@ +673,5 @@
> +  addonSection: function(dataDiv) {
> +    let tableAddon = document.createElement("table");
> +    tableAddon.setAttribute("id", "addons-data");
> +    let caption = document.createElement("caption");
> +    caption.appendChild(document.createTextNode("addons" + "\n"));

No "\n" needed here.

Comment 8

[Penh Lenh]

Attachment: I have updated the new patch, please review again.

Comment 9

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8695338 [details] [diff] [review]
I have updated the new patch, please review again.

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

I'm failing to apply this locally here - is it based on another patch?
If you ended up with your changes in separate patches then you can fold them together - e.g. using "hg qfold <patch number>".

Note: There are a few lines with trailing whitespace, lets remove that.
You can see them highlighted in red by following the "review" link on the patch.

::: toolkit/content/aboutTelemetry.css
@@ +251,5 @@
>  }
>  
>  /* addon subsection style */
> +#activeAddons caption, #activePlugins caption, .theme caption,
> +.activeExperiment caption, #activeGMPlugins caption, #persona caption {

Let's give those captions a common class, say "addon-caption", then we don't need to list them all out there.

::: toolkit/content/aboutTelemetry.js
@@ +567,1 @@
>        let table = document.createElement("table");

The indentation here is off, it uses two spaces too much.

@@ +569,3 @@
>        this.appendAddonTitle(sectionTitle, table);
>        
>        let persona = addonObj == null ? "null" : addonObj.persona

I don't think you need this here - null should just convert to string below, so we can just do:
this.appendColumn(row, "td", addonObj.persona);

@@ +576,1 @@
>        table.appendChild(row);

Side-note: I think we should refactor all the table usage in aboutTelemetry.js in a future bug, all that could be much shorter :)

@@ +588,5 @@
> +    for (let plugin of addonObj) { 
> +      let data = explodeObject(plugin);
> +
> +      for (let [key, value] of data) {
> +        if (key.contains("name")){

I think you should just do this before this loop:
let data = ...
this.appendHeadingName(table, data.get("name"));

@@ +610,5 @@
> +    
> +    for (let id of Object.keys(addonObj)) { 
> +      let addon = addonObj[id];
> +      if (addon.name) {
> +        this.appendHeadingName(table, addon.name);

Does this mean that entries in activeGMPlugins don't get individual headings?
They should get headings too, we can just use the id instead here:
let addon = ...
this.appendHeadingName(table, addon.name || id);

@@ +653,1 @@
>      table.appendChild(row);

Can we change this into a appendRow() function?
Then you can just use this as |appendRow(table, "id", idValue);| and re-use it for the other functions too.

@@ +667,4 @@
>      table.appendChild(headings);
>    },
>  
>    appendAddonTitle: function(section, table) {

Lets name this "appendAddonSubsectionTitle".

Comment 10

[Penh Lenh]

Attachment: bug-1172459-fix-v4.patch

I updated by your suggestion, please review.

Comment 11

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8696700 [details] [diff] [review]
bug-1172459-fix-v4.patch

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

I tested this out locally and visually this looks fine, thanks!
This is nearly done, the comments below are more about style and some cleanup.

::: toolkit/content/aboutTelemetry.js
@@ +532,5 @@
>  
>      for (let [section, sectionData] of data) {
> +      if (section == "addons") {
> +        break;
> +      } 

Nit: Trailing whitespace.

@@ +543,5 @@
>  
>        for (let [path, value] of sectionData) {
> +        let row = document.createElement("tr");
> +        this.appendColumn(row, "td", path + "\t");
> +        this.appendColumn(row, "td", value + "\t");

I know they were here before, but lets drop the "\t"s while we're changing code here.
They should not be needed at all.

@@ +550,5 @@
>  
>        dataDiv.appendChild(table);
>      }
> +
> +    let addonSection = this.createAddonSection(dataDiv);

Lets add a comment before this line to make it clear what we are doing:
// We use specialized rendering here to make the addon and plugin listings
// more readable.

@@ +575,5 @@
> +    let table = document.createElement("table");
> +    table.setAttribute("id", sectionTitle);
> +    this.appendAddonSubsectionTitle(sectionTitle, table);
> +
> +    for (let plugin of addonObj) { 

There is more trailing whitespace here and below.
You can e.g. see it highlighted in red when you follow the "review" link for this patch.
You can also check into configuring your editor to highlight this.

@@ +587,5 @@
> +
> +    addonSection.appendChild(table);
> +  },
> +
> +  renderKeyObjectAddons: function(addonObj, addonSection, sectionTitle) {

Lets call this renderAddonsObject().

@@ +606,5 @@
> +
> +    addonSection.appendChild(table);
> +  },
> +
> +  renderKeyValue: function(addonObj, addonSection, sectionTitle) {

Lets call this renderKeyValueObject().

@@ +652,5 @@
> +    let caption = document.createElement("caption");
> +    caption.appendChild(document.createTextNode("addons"));
> +    divAddon.appendChild(caption);
> +    dataDiv.appendChild(divAddon);
> +    return document.getElementById("addons-data");

Can't you just |return divAddon|?

Comment 12

[Penh Lenh]

Attachment: bug-1172459-fix-v5.patch

I have cleaned it up. Please review.

Comment 13

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8698054 [details] [diff] [review]
bug-1172459-fix-v5.patch

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

Thanks, this looks good now, only three nits below.

Lets fix those, then upload the patch with "r=gfritzsche" added to the end of the message, then we can get it landed.

::: toolkit/content/aboutTelemetry.js
@@ +551,5 @@
>        dataDiv.appendChild(table);
>      }
> +
> +    // We use specialized rendering here to make the addon and plugin listings
> +    // more readable

Nit: "." at the end of the sentence.

@@ +593,5 @@
> +  renderAddonsObject: function(addonObj, addonSection, sectionTitle) {
> +    let table = document.createElement("table");
> +    table.setAttribute("id", sectionTitle);
> +    this.appendAddonSubsectionTitle(sectionTitle, table);
> +    

Nit: Trailing whitespace here.

@@ +656,5 @@
> +    divAddon.appendChild(caption);
> +    dataDiv.appendChild(divAddon);
> +    return divAddon;
> +  },
> +  

Nit: trailing whitespace.

Comment 14

[Penh Lenh]

Attachment: bug-1172459-fix-v6.patch

Thanks, Finally I did it. "r=gfritzsche"

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/154c2eaad666

Comment 16

[Georg Fritzsche [:gfritzsche]]

Landed with updated commit message per comment 15.

Thanks for the persistence, this change will be helpful for engineers, QA and users.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/154c2eaad666