1251952 - about:telemetry should handle empty ping archives gracefully

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[Georg Fritzsche [:gfritzsche]]

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.

Comment 1

[Penh Lenh]

Attachment: bug-1251952-fix.patch

Please review.

Comment 2

[Georg Fritzsche [: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.

Comment 3

[Penh Lenh]

Attachment: bug-1251952-fix-v2.patch

Please check.

Comment 4

[Georg Fritzsche [: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|.

Comment 5

[Penh Lenh]

Attachment: bug-1251952-fix-v3.patch

I updated the argument as you comment, please review.

Comment 6

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8725990 [details] [diff] [review]
bug-1251952-fix-v3.patch

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

Thanks, this looks good!

Comment 7

[Georg Fritzsche [:gfritzsche]]

This only affects about:telemetry, which doesn't have test coverage - no try run needed.

Comment 8

[Pulsebot]

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

Comment 9

[Carsten Book [:Tomcat]]

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