945044 - about:telemetry should restore state on reload and have histograms filter

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Avi Halachmi (:avih)]

Attachment: about:telemetry - keep sections states on reload (collapsed/expanded)

Keeping sections state on reload would allow to focus on an expanded section, reload the page, and immediately observed the updated values without the need to re-expand the section and then visually relocate the required data.

Approach:
- Add a hidden checkbox to each section which reflects the section's state (updated whenever the section is collapsed/expanded).

- Rely on Firefox's form auto-fill (?) on reload (default behavior) to keep the states of the checkboxes on reload/restore.

- OnLoad: expand the sections with the checkbox checked.

To reset the states: CTRL-F5 (reload without using the cache).

Comment 1

[Avi Halachmi (:avih)]

Attachment: about:telemetry - keep sections states on reload (collapsed/expanded)

Slightly more readable.

Comment 2

[Avi Halachmi (:avih)]

Attachment: part 1 - store/restore expanded/collapsed state

Comment 3

[Avi Halachmi (:avih)]

Attachment: part 2 - cleanup (funtional to declarative top level UI)

This part also adds a small improvement: show empty sections as gray.

Comment 4

[Avi Halachmi (:avih)]

Attachment: part 3 - add histograms filter

The folded diff of all 3 parts is here: https://pastebin.mozilla.org/3711148

Comment 5

[Vladan Djeric (:vladan)]

Comment on attachment 8341777 [details] [diff] [review]
part 1 - store/restore expanded/collapsed state

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

::: toolkit/content/aboutTelemetry.js
@@ +845,5 @@
>    Telemetry.asyncFetchTelemetryData(displayPingData);
> +
> +  // Restore sections states
> +  let stateboxes = document.getElementsByClassName("statebox");
> +  for (let i = 0; i < stateboxes.length; i++) {

for (statebox of stateboxes) {
  ...
}

Comment 6

[Vladan Djeric (:vladan)]

Comment on attachment 8341781 [details] [diff] [review]
part 2 - cleanup (funtional to declarative top level UI)

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

::: toolkit/content/aboutTelemetry.js
@@ +826,5 @@
>  
>    // Restore sections states
>    let stateboxes = document.getElementsByClassName("statebox");
> +  for (let box of stateboxes) {
> +    if (box.checked){ // Was open. Will still display as empty if not has-data

Typo: add a space between ) and {

Comment 7

[Vladan Djeric (:vladan)]

Comment on attachment 8341782 [details] [diff] [review]
part 3 - add histograms filter

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

::: toolkit/content/aboutTelemetry.js
@@ +785,5 @@
>  }
>  
> +/**
> + * Helper function for filtering histogram elements by their id
> + * Result is adding class "filter-blocked" for histogram class nodes who's id don't match the filter.

"It adds the "filter-blocked" class to histogram nodes whose IDs don't match the filter." :)

@@ +790,5 @@
> + *
> + * @param aContainerNode Container node containing the histogram class nodes to filter
> + * @param aFilterText either text or /RegEx/. If text, case-insensitive and AND words
> + */
> +function filterHistograms(aContainerNode, aFilterText) {

Make filterHistograms and all these inner helper functions members of the Histograms singleton

@@ +799,5 @@
> +    if (!filter.length) {
> +      return true; // empty filter
> +    }
> +    for (let item of filter) {
> +      if (item.length && subject.toLowerCase().indexOf(item) < 0) {

histogram IDs are all upper case.. you could just make the filter words upper-case too :)

@@ +815,5 @@
> +  let isPassFunc; // filter function, set once, then applied to all elements
> +  filter = filter.trim();
> +  if (filter[0] != "/") { // Plain text: case insensitive, AND if multi-string
> +    isPassFunc = isPassText;
> +    filter = filter.toLowerCase().split(" ");

Document the filter syntax in the filter caption on the page

@@ +835,5 @@
> +    hist.classList[isPassFunc(hist.id, filter) ? "remove" : "add"]("filter-blocked");
> +  }
> +}
> +
> +function histogramFilterChanged() {

Add a function comment + make this function part of the Histogram singleton

@@ +839,5 @@
> +function histogramFilterChanged() {
> +  if (this.idleTimeout) {
> +    clearTimeout(this.idleTimeout);
> +  }
> +  this.idleTimeout = setTimeout(function () {

You don't need to bind "this" if you use arrow notation for the anonymous function. But you could also just look up the filter element by ID

@@ +867,5 @@
>      for (let [name, hgram] of Iterator(histograms)) {
>        Histogram.render(hgramDiv, name, hgram);
>      }
> +
> +    let filterBox = hgramDiv.parentElement.getElementsByClassName("filter")[0];

Let's just give the filter box element an ID in the xhtml file

@@ +869,5 @@
>      }
> +
> +    let filterBox = hgramDiv.parentElement.getElementsByClassName("filter")[0];
> +    filterBox.addEventListener("input", histogramFilterChanged, false);
> +    filterHistograms(hgramDiv, filterBox.value);

why are we filtering on an empty value during onLoad?

::: toolkit/content/aboutTelemetry.xhtml
@@ +64,5 @@
>        <input type="checkbox" class="statebox"/>
>        <h1 class="section-name">&aboutTelemetry.histogramsSection;</h1>
>        <span class="toggle-caption">&aboutTelemetry.toggle;</span>
>        <span class="empty-caption">&aboutTelemetry.emptySection;</span>
> +      <span class="filter-ui">&aboutTelemetry.filterText; <input type="text" class="filter"/></span>

Nit: put the input element on a separate indented line in this file

::: toolkit/locales/en-US/chrome/global/aboutTelemetry.dtd
@@ +64,5 @@
>    Show raw data from hangs
>  ">
> +
> +<!ENTITY aboutTelemetry.filterText "
> +  Filter (words or /RegEx..)

Filter (words or /RegEx/). A pattern starting with just a slash would not work

Comment 8

[Vladan Djeric (:vladan)]

(In reply to Vladan Djeric (:vladan) from comment #7)
> @@ +869,5 @@
> >      }
> > +
> > +    let filterBox = hgramDiv.parentElement.getElementsByClassName("filter")[0];
> > +    filterBox.addEventListener("input", histogramFilterChanged, false);
> > +    filterHistograms(hgramDiv, filterBox.value);
> 
> why are we filtering on an empty value during onLoad?

Nevermind, I figured out it's to restore state after a reload. Maybe just add a check for filterBox.value being non-empty to make it clear

Comment 9

[Avi Halachmi (:avih)]

(In reply to Vladan Djeric (:vladan) from comment #7)
> Comment on attachment 8341782 [details] [diff] [review]
> histogram IDs are all upper case.. you could just make the filter words
> upper-case too :)

- This is an assumption which won't necessarily stick and I prefer to keep it safe.
- These case-insensitive and regex filters are generic and could be used elsewhere as well, but not if we assume the subject is always upper case.
- The filter itself is not a bottleneck which we need to squeeze for max performance with assumptions - the layout flush which comes later is way slower.


> why are we filtering on an empty value during onLoad?

Firefox keeps the fields values on reload - same as the stateboxes, So on (re)load, the filter is preserved and applied.


> Document the filter syntax in the filter caption on the page

You mean other than this? :

> > +  Filter (words or /RegEx..)
> 
> Filter (words or /RegEx/). A pattern starting with just a slash would not
> work

I had it this way initially, but it could also be /flag/i for instance, which is why I added the ellipsis and didn't suggest it must end with a slash (and also, whoever uses regexp would know it needs at least another slash). But I'm open to suggestions for improvements.

Comment 10

[Avi Halachmi (:avih)]

(In reply to Avi Halachmi (:avih) from comment #9)
> - These case-insensitive and regex filters are generic and could be used
> elsewhere as well ...

That was a lie, but I'll make it consistently generic and while at it, also faster.

Comment 12

[Avi Halachmi (:avih)]

Attachment: Addressed comments and folded

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

Comment 13

[Carsten Book [:Tomcat]]

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