Closed Bug 1368667 Opened 7 years ago Closed 7 years ago

Refactor about:telemetry to use generic rendering function

Categories

(Toolkit :: Telemetry, enhancement, P1)

Product:
Toolkit
Component:
Telemetry
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: flyingrub, Assigned: flyingrub, Mentored)

References

Details

Attachments

(5 files)

Bug 1368667 - Refactor General Data
59 bytes, text/x-review-board-request
chutten
: review+
Details
Bug 1368667 - Move Section Specific code to their own objects
59 bytes, text/x-review-board-request
chutten
: review+
Details
Bug 1368667 - Replace KeyValueTable with GenericTable
59 bytes, text/x-review-board-request
chutten
: review+
Details
Bug 1368667 - Remove KeyValueTable as it is similar to GenericTable
59 bytes, text/x-review-board-request
chutten
: review+
Details
Bug 1368667 - Generalize Subsection Renderer
59 bytes, text/x-review-board-request
chutten
: review+
Details
No description provided.
Assignee: nobody → flyinggrub
Attachment #8872609 - Flags: review?(chutten)
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 on attachment 8872609 [details] Bug 1368667 - Refactor General Data https://reviewboard.mozilla.org/r/144148/#review147878
Attachment #8872609 - Flags: review?(chutten) → review+
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)
(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 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 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 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-
Flags: needinfo?(chutten)
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 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+
(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 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 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 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 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 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 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.
(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 on attachment 8873007 [details] Bug 1368667 - Replace KeyValueTable with GenericTable https://reviewboard.mozilla.org/r/144508/#review149198
Attachment #8873007 - Flags: review?(chutten) → review+
Comment on attachment 8873429 [details] Bug 1368667 - Generalize Subsection Renderer https://reviewboard.mozilla.org/r/144834/#review149200
Attachment #8873429 - Flags: review?(chutten) → review+
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)
Priority: -- → P1
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.

Attachment

General

Creator:
Created:
Updated:
Size: