Closed Bug 811159 Opened 7 years ago Closed 7 years ago

Save last uploaded payload


(Firefox Health Report Graveyard :: Client: Desktop, defect)

Not set


(firefox18 fixed, firefox19 fixed, firefox20 fixed)

Firefox 20
Tracking Status
firefox18 --- fixed
firefox19 --- fixed
firefox20 --- fixed


(Reporter: gps, Assigned: gps)




(1 file, 1 obsolete file)

Attached patch Save payload after upload, v1 (obsolete) — Splinter Review
One of the requirements for about:healthreport is to allow display of the previously-uploaded payload any time. This necessitates the requirement of saving the payload locally.

The attached patch does just that. I'm not asking for full review yet because I just got this working and it could almost certainly use some clean-up. Why submit a patch at all? I don't know either. Long day and I want something to show for it, I suppose.
Attachment #680892 - Flags: feedback?(rnewman)
Comment on attachment 680892 [details] [diff] [review]
Save payload after upload, v1

Review of attachment 680892 [details] [diff] [review]:

I'd prefer to see a test for a failing write.

::: services/healthreport/healthreporter.jsm
@@ +387,5 @@
>                                      payload,
>                                      this.lastSubmitID);
> +    return promise.then(this._onBagheeraResult.bind(this, request, false))
> +                  .then(this._saveLastPayload.bind(this, payload));

I'd argue that we want to save the payload first. We don't want to upload something that we then fail to write out; we won't be able to display it to the user. (And perhaps we want something else to do the upload.)

@@ +448,5 @@
> +
> +    return OS.File.writeAtomic(path, buffer, {tmpPath: pathTmp});
> +  },
> +
> +  getLastPayload: function getLoadPayload() {

JavaDoc, omit function name.

::: services/healthreport/tests/xpcshell/head.js
@@ +2,5 @@
>   * */
>  "use strict";
> +do_get_profile();

I suggest putting a comment here, and a link to Bug 810543.
Attachment #680892 - Flags: feedback?(rnewman) → feedback+
Saving before upload now.

I left the function name in because I think the style should be consistent. And, we haven't yet investigated whether exceptionStr preserves it, right? I figure we can bulk update our style in the future.
Assignee: nobody → gps
Attachment #680892 - Attachment is obsolete: true
Attachment #681839 - Flags: review?(rnewman)
Comment on attachment 681839 [details] [diff] [review]
Save payload before upload, v2

Review of attachment 681839 [details] [diff] [review]:

Good enough for me.
Attachment #681839 - Flags: review?(rnewman) → review+
Comment on attachment 681839 [details] [diff] [review]
Save payload before upload, v2

Bulk-setting approval flags for FHR landing for FxOS ADU ping (Bug 788894).
Attachment #681839 - Flags: approval-mozilla-beta?
Attachment #681839 - Flags: approval-mozilla-aurora?
Target Milestone: --- → mozilla20
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Comment on attachment 681839 [details] [diff] [review]
Save payload before upload, v2

FHR for B2G ADU ping, won't be built/enabled for Mobile/Desktop.
Attachment #681839 - Flags: approval-mozilla-beta?
Attachment #681839 - Flags: approval-mozilla-beta+
Attachment #681839 - Flags: approval-mozilla-aurora?
Attachment #681839 - Flags: approval-mozilla-aurora+
Component: Metrics and Firefox Health Report → Client: Desktop
Flags: in-testsuite+
Product: Mozilla Services → Firefox Health Report
Target Milestone: mozilla20 → Firefox 20
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.