Refactor about:telemetry to use generic rendering function

RESOLVED FIXED in Firefox 55

Status

()

P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: flyingrub, Assigned: flyingrub, Mentored)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(5 attachments)

Comment hidden (empty)
(Assignee)

Updated

a year ago
Assignee: nobody → flyinggrub
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8872609 - Flags: review?(chutten)
(Assignee)

Comment 2

a year ago
https://dxr.mozilla.org/mozilla-central/source/toolkit/content/aboutTelemetry.js?q=file%3AaboutTelemetry.js&redirect_type=single#2149-2172

I feel that SimpleMeasurements and SessionInfo could be moved to their own object for better code readability.
SortStartupMilestone could be in the SimpleMeasurement Object.

Also should we keep KeyValueTable or just always use GenericTable ?
Flags: needinfo?(chutten)

Comment 3

a year ago
mozreview-review
Comment on attachment 8872609 [details]
Bug 1368667 - Refactor General Data

https://reviewboard.mozilla.org/r/144148/#review147878
Attachment #8872609 - Flags: review?(chutten) → review+
(Assignee)

Comment 4

a year ago
So for now what I planned is to check each different section and make them use GenericTable Object to render them.
I think that keeping an Object per section to keep track of it's rendering is more meaningful than having it in the general DisplayPingData (now renamed displayRichPingData). So the next step will be move all the code that is relevant to rendering a section to it's own Object. 
From that point we will have a better base, and I will have a good perception of the current state of the source code.

We could then clearly define how we should display the childprocess in a way that isn't confusing for the user. But I think this could be another issue on bugzilla. 


Does this little explanation clear up my plan ?
Flags: needinfo?(gfritzsche)
Yes, thanks! Moving the section-specific code into their own objects sounds great.

(In reply to flyingrub from comment #4)
> So for now what I planned is to check each different section and make them
> use GenericTable Object to render them.

How will you use GenericTable for sections that use specialized rendering, like histograms?
Flags: needinfo?(gfritzsche)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(In reply to Georg Fritzsche [:gfritzsche] from comment #5)
> (In reply to flyingrub from comment #4)
> > So for now what I planned is to check each different section and make them
> > use GenericTable Object to render them.
> 
> How will you use GenericTable for sections that use specialized rendering,
> like histograms?
Flags: needinfo?(flyinggrub)

Comment 9

a year ago
mozreview-review
Comment on attachment 8872974 [details]
Bug 1368667 - Move Section Specific code to their own objects

https://reviewboard.mozilla.org/r/144502/#review148370

Mostly there, just a few small things.

::: toolkit/content/aboutTelemetry.js:1521
(Diff revision 1)
>    }
>  };
>  
>  var GenericTable = {
> +
> +  defaultHeadings: [

These used to be const.

::: toolkit/content/aboutTelemetry.js:1532
(Diff revision 1)
>     * Returns a n-column table.
>     * @param rows An array of arrays, each containing data to render
>     *             for one row.
>     * @param headings The column header strings.
>     */
> -  render(rows, headings) {
> +  render(rows, headings = this.defaultHeadings) {

I remember a time when JS didn't have default values for function parameters. It was a dark time.

::: toolkit/content/aboutTelemetry.js:1971
(Diff revision 1)
> +        Histogram.filterHistograms(hgramDiv, filterBox.value);
> +      }
> +
> +      setHasData("histograms-section", true);
> +    }
> +  }

Should probably add a defensive comma here and in the other section objects.

::: toolkit/content/aboutTelemetry.js:2244
(Diff revision 1)
>    if (payloadIndex > 0) {
>      payload = ping.payload.childPayloads[payloadIndex - 1];
>    }
>  
>    // Show thread hang stats
>    ThreadHangStats.render(payload);

I suppose the next step might be to convert to

[ThreadHangStats,
 CapturedStacks,
 ...].forEach(renderer => renderer.render(payload));
 
 Or not. Might not be worth it.
Attachment #8872974 - Flags: review?(chutten) → review-

Comment 10

a year ago
mozreview-review
Comment on attachment 8872974 [details]
Bug 1368667 - Move Section Specific code to their own objects

https://reviewboard.mozilla.org/r/144502/#review148382

::: toolkit/content/aboutTelemetry.js:1523
(Diff revision 1)
>  
>  var GenericTable = {
> +
> +  defaultHeadings: [
> +    bundle.GetStringFromName("keysHeader"),
> +    bundle.GetStringFromName("valuesHeader")

Oh, and isn't this part of the KeyValue vs Generic table refactor instead?

Comment 11

a year ago
mozreview-review
Comment on attachment 8873007 [details]
Bug 1368667 - Replace KeyValueTable with GenericTable

https://reviewboard.mozilla.org/r/144508/#review148380

Basically fine, just want some guarantee that lint won't complain and that all the l10n boxes have been ticked.

::: toolkit/content/aboutTelemetry.js:1630
(Diff revision 1)
>        let providerSection = document.createElement("h2");
>        let titleText = bundle.formatStringFromName("addonProvider", [provider], 1);
>        providerSection.appendChild(document.createTextNode(titleText));
>        addonSection.appendChild(providerSection);
> -      addonSection.appendChild(
> -        KeyValueTable.render(addonDetails[provider],
> +      let table = GenericTable.render(explodeObject(addonDetails[provider]),
> +                              [this.tableIDTitle, this.tableDetailsTitle ]);

Does this indent pass lint? I would think it would have to align with the e in `explodeObject`

::: toolkit/locales/en-US/chrome/global/aboutTelemetry.properties
(Diff revision 1)
>  # - %2$S will be replaced with the value of the toolkit.telemetry.server_owner preference
>  pageSubtitle = This page shows the information about performance, hardware, usage and customizations collected by Telemetry. This information is submitted to %1$S to help improve %2$S.
>  
>  generalDataTitle = General Data
>  
> -generalDataHeadingName = Name

This string is changing from "Name" to "Property". Are you reasonably certain that the translations of this string will also be synonymous? (They probably are, but it pays to check)

Also, have you checked with the l10n team to make sure there isn't anything extra you need to do when you remove translation strings?
Attachment #8873007 - Flags: review?(chutten) → review-

Updated

a year ago
Flags: needinfo?(chutten)
(Assignee)

Comment 12

a year ago
mozreview-review-reply
Comment on attachment 8872974 [details]
Bug 1368667 - Move Section Specific code to their own objects

https://reviewboard.mozilla.org/r/144502/#review148370

> I suppose the next step might be to convert to
> 
> [ThreadHangStats,
>  CapturedStacks,
>  ...].forEach(renderer => renderer.render(payload));
>  
>  Or not. Might not be worth it.

I think the code is clearer as is. :)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 17

a year ago
mozreview-review
Comment on attachment 8872974 [details]
Bug 1368667 - Move Section Specific code to their own objects

https://reviewboard.mozilla.org/r/144502/#review148482
Attachment #8872974 - Flags: review?(chutten) → review+
(Assignee)

Comment 18

a year ago
(In reply to Georg Fritzsche [:gfritzsche] from comment #8)
> > How will you use GenericTable for sections that use specialized rendering,
> > like histograms?

Histograms already have their generic renderer, I think I will not modify them for now.

Next step is to create a generic subsection renderer (needed for environment data).
Flags: needinfo?(flyinggrub)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 21

a year ago
mozreview-review
Comment on attachment 8872609 [details]
Bug 1368667 - Refactor General Data

https://reviewboard.mozilla.org/r/144148/#review148760

::: commit-message-286f7:1
(Diff revision 1)
> +Bug 1368667 - Refactor General Data

The r?chutten should be on the first line.

Then an explainer on line 3 about why this is a good change. "GenericTable already provides the functionality, so we may as well use it"

In general, our commit messages follow the guide at https://chris.beams.io/posts/git-commit/ but with extra cruft (bug numbers and r?reviewer annotations) because of mozilla process.

Comment 22

a year ago
mozreview-review
Comment on attachment 8872974 [details]
Bug 1368667 - Move Section Specific code to their own objects

https://reviewboard.mozilla.org/r/144502/#review148766

::: commit-message-6b976:1
(Diff revision 3)
> +Bug 1368667 - Move Section Specific code to their own object; r?chutten

*their own objects

Also, no need for punctuation at the end of the summary.

Also also, a good habit is to have some explanation text in the body. Something like "Every section besides Histogram, KeyedHistogram, Addon, Session, and Simple Measurements were already in their own objects. This just brings them into line."

This is a good way to reassure stakeholders that you  aren't just messing about :)

Comment 23

a year ago
mozreview-review
Comment on attachment 8873007 [details]
Bug 1368667 - Replace KeyValueTable with GenericTable

https://reviewboard.mozilla.org/r/144508/#review148768

::: commit-message-d1a5e:1
(Diff revision 4)
> +Replace KeyValueTable by GenericTable r?chutten

s/by/with/

Also, you're missing the bug# at the front of the summary.

Also also, some explanation "They have essentially the same capabilities" would be nice to have in the body.
Attachment #8873007 - Flags: review?(chutten) → review-

Comment 24

a year ago
mozreview-review
Comment on attachment 8873364 [details]
Bug 1368667 - Remove KeyValueTable as it is similar to GenericTable

https://reviewboard.mozilla.org/r/144806/#review148770

Fix the commit message and all's good. I like patches that have a lot of red :)

::: commit-message-318ea:1
(Diff revision 1)
> +Remove KeyValueTable as it is similar to GenericTable r?chutten

Missing a bug#.

Doesn't _need_ a body, but might benefit from one illustrating the ways that KVT is different from GT (so we know what we're losing by removing these 60+ lines)
Attachment #8873364 - Flags: review?(chutten) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 33

a year ago
mozreview-review-reply
Comment on attachment 8873007 [details]
Bug 1368667 - Replace KeyValueTable with GenericTable

https://reviewboard.mozilla.org/r/144508/#review148380

> This string is changing from "Name" to "Property". Are you reasonably certain that the translations of this string will also be synonymous? (They probably are, but it pays to check)
> 
> Also, have you checked with the l10n team to make sure there isn't anything extra you need to do when you remove translation strings?

I know you chatted with l10n about this, but I don't see it mentioned in the bug.

Comment 34

a year ago
mozreview-review
Comment on attachment 8873429 [details]
Bug 1368667 - Generalize Subsection Renderer

https://reviewboard.mozilla.org/r/144834/#review148800

::: commit-message-b4c46:3
(Diff revision 2)
> +Bug 1368667 - Generalize Subsection Renderer r?chutten
> +
> +Allow the use of the subsection render not just for the environment data.

But why?

::: toolkit/locales/en-US/chrome/global/aboutTelemetry.properties:12
(Diff revision 2)
>  # - %2$S will be replaced with the value of the toolkit.telemetry.server_owner preference
>  pageSubtitle = This page shows the information about performance, hardware, usage and customizations collected by Telemetry. This information is submitted to %1$S to help improve %2$S.
>  
>  generalDataTitle = General Data
>  
> -environmentDataSubsectionToggle = Click to toggle section
> +subsectionToggle = Click to toggle section

Renaming l10n keys is almost certainly frowned upon, and may require another translation pass. Check with l10n.
Attachment #8873429 - Flags: review?(chutten) → review-
(In reply to Chris H-C :chutten from comment #34)
> > -environmentDataSubsectionToggle = Click to toggle section
> > +subsectionToggle = Click to toggle section
> 
> Renaming l10n keys is almost certainly frowned upon, and may require another
> translation pass. Check with l10n.

Yes, please leave the string IDs unchanged, as their semantics don't change. Then we don't need a hundred people do this string again.

General comment: Reusing strings like this limits the quality of the localized UX. Some languages feel really wonky if you need to make such general statements. Like, there are languages where "yes" and "no" aren't good button labels ;-)

Granted, I think the UX is wonky, and the target audience of this feature might not be too affected by the existing wonkyness, so let's not invest in fixing this by introducing a distinct label for each expander. Just wanted to expand a bit on why this code pattern might not be good in more general-audience facing UI.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 40

a year ago
(In reply to Chris H-C :chutten from comment #33)
> Comment on attachment 8873007 [details]
> Bug 1368667 - Replace KeyValueTable with GenericTable
> 
> https://reviewboard.mozilla.org/r/144508/#review148380
> 
> > This string is changing from "Name" to "Property". Are you reasonably certain that the translations of this string will also be synonymous? (They probably are, but it pays to check)
> > 
> > Also, have you checked with the l10n team to make sure there isn't anything extra you need to do when you remove translation strings?
> 
> I know you chatted with l10n about this, but I don't see it mentioned in the
> bug.

I ended up letting the heading as it was previously using the l10n key "namesHeader".

Comment 41

a year ago
mozreview-review
Comment on attachment 8873007 [details]
Bug 1368667 - Replace KeyValueTable with GenericTable

https://reviewboard.mozilla.org/r/144508/#review149198
Attachment #8873007 - Flags: review?(chutten) → review+

Comment 42

a year ago
mozreview-review
Comment on attachment 8873429 [details]
Bug 1368667 - Generalize Subsection Renderer

https://reviewboard.mozilla.org/r/144834/#review149200
Attachment #8873429 - Flags: review?(chutten) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 48

a year ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s cdd0ee93893c -d cee187e9cd86: rebasing 399841:cdd0ee93893c "Bug 1368667 - Refactor General Data r=chutten"
merging toolkit/content/aboutTelemetry.js
rebasing 399842:6dccc2a74232 "Bug 1368667 - Move Section Specific code to their own objects r=chutten"
merging toolkit/content/aboutTelemetry.js
warning: conflicts while merging toolkit/content/aboutTelemetry.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Priority: -- → P1

Comment 54

a year ago
Pushed by chutten@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0eef7d224f45
Refactor General Data r=chutten
https://hg.mozilla.org/integration/autoland/rev/dfd1b819e204
Move Section Specific code to their own objects r=chutten
https://hg.mozilla.org/integration/autoland/rev/d91ee7347dce
Replace KeyValueTable with GenericTable r=chutten
https://hg.mozilla.org/integration/autoland/rev/ca9c07c8244a
Remove KeyValueTable as it is similar to GenericTable r=chutten
https://hg.mozilla.org/integration/autoland/rev/39b358a1d0ca
Generalize Subsection Renderer r=chutten
You need to log in before you can comment on or make changes to this bug.