Closed Bug 1251952 Opened 4 years ago Closed 4 years ago

about:telemetry should handle empty ping archives gracefully

Categories

(Toolkit :: Telemetry, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: gfritzsche, Assigned: penhlenh, Mentored)

References

Details

(Whiteboard: [measurement:client] [lang=js])

Attachments

(1 file, 2 obsolete files)

If the ping archive is empty (e.g. on new profiles or when ping archiving is disabled), aboutTelemetry.js should handle this gracefully.

We should disable the "archived ping data" option, so that we never show the UI to load them and check that no more errors are thrown when loading about:telemetry with an empty archive directory.
Assignee: nobody → penhlenh
Attached patch bug-1251952-fix.patch (obsolete) — Splinter Review
Please review.
Attachment #8724821 - Flags: review?(gfritzsche)
Comment on attachment 8724821 [details] [diff] [review]
bug-1251952-fix.patch

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

This seems to work well!
I have some requests for improvement below though.

::: toolkit/content/aboutTelemetry.js
@@ +324,5 @@
>      let structuredChanged = viewStructured !== this.viewStructuredPingData;
>      this.viewCurrentPingData = viewCurrent;
>      this.viewStructuredPingData = viewStructured;
>  
> +    // Checking source archived if it's null, we disabled the source archive.

I suggest different comment, e.g.:
// If we have no archived pings, disable the ping archive selection.
// This can happen on new profiles or if the ping archive is disabled.

@@ +328,5 @@
> +    // Checking source archived if it's null, we disabled the source archive.
> +    let archivedPingList = yield TelemetryArchive.promiseArchivedPingList();
> +    let sourceArchived = document.getElementById("ping-source-archive");
> +    if (archivedPingList.length == 0) {
> +      sourceArchived.disabled = true;

I think we don't need if-else branching here, we can do something like:
sourceArchived.disabled = (archivedPingList.length == 0);

@@ +340,5 @@
>          document.getElementById("archived-ping-picker").classList.add("hidden");
>          this._updateCurrentPingData();
>        } else {
>          document.getElementById("current-ping-picker").classList.add("hidden");
>          yield this._updateArchivedPingList();

Now we can pass |archivedPingList| to this function, so it doesn't have to call |TelemetryArchive.promiseArchivedPingList()| again.
Attachment #8724821 - Flags: review?(gfritzsche) → feedback+
Attached patch bug-1251952-fix-v2.patch (obsolete) — Splinter Review
Please check.
Attachment #8724821 - Attachment is obsolete: true
Attachment #8725555 - Flags: review+
Attachment #8725555 - Flags: review+ → review?(gfritzsche)
Comment on attachment 8725555 [details] [diff] [review]
bug-1251952-fix-v2.patch

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

This looks good.
There is one minor thing we should fix, then we can land this.

::: toolkit/content/aboutTelemetry.js
@@ +367,5 @@
>                             .then((ping) => displayPingData(ping, true));
>    },
>  
> +  _updateArchivedPingList: Task.async(function*(archivedPingList) {
> +    let pingList = archivedPingList;

This line is redundant - let's just call the argument |pingList|.
Attachment #8725555 - Flags: review?(gfritzsche) → review+
I updated the argument as you comment, please review.
Attachment #8725555 - Attachment is obsolete: true
Attachment #8725990 - Flags: review?(gfritzsche)
Comment on attachment 8725990 [details] [diff] [review]
bug-1251952-fix-v3.patch

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

Thanks, this looks good!
Attachment #8725990 - Flags: review?(gfritzsche) → review+
This only affects about:telemetry, which doesn't have test coverage - no try run needed.
Status: NEW → ASSIGNED
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/acdeef9aa12a
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.