848852 - Document handler for FHR documents

Status: RESOLVED WONTFIX

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

Description

[Gregory Szorc [:gps]]

The plan is to refactor payload saving and uploading out of policy.jsm and healthreporter.jsm and into a standalone "document handler" entity. There will presumably be a separate implementation for desktop and android. This bug will focus on the desktop version.

My plan is to land a standalone/backwards-compatible DesktopDocumentHandler type first then refactor existing code to use it later.

Comment 1

[Gregory Szorc [:gps]]

Attachment: Desktop document handler, v1

Here is what I've got so far. I still need to implement retry logic and more tests. I may also jiggle some of the property names now that we aren't within the policy's namespace. Looking for a high-level "does this API for a document handler look sane" feedback pass - not a code review!

Comment 2

[Gregory Szorc [:gps]]

Attachment: Part 1: Attach BagheeraRequest to Bagheera client promises, v1

Eventually this will be used to ensure in-flight requests are aborted explicitly by the document handler rather than by Necko shutting down. (I'm not even sure what Necko does to in-flight requests during shutdown.)

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 723678 [details] [diff] [review]
Part 1: Attach BagheeraRequest to Bagheera client promises, v1

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

This strikes me as slightly fragile -- promises are meant to be chained, but that chaining would hide the request.

But I confess to not having a better solution, unless we want to go down the road of adding a 'cancel' operation to Promise.

Comment 4

[Gregory Szorc [:gps]]

Fun fact: Promise.defer() takes a "proto" argument which defines the prototype used on the created promise instance.

I also considered just exposing .abort on the promise but figured "why not expose the entire thing, just in case." This whole thing is definitely an advanced API and shouldn't be used under normal circumstances.

Comment 5

[Gregory Szorc [:gps]]

The impetus behind this was FHR on Android back when we thought the existing XPCOM service would do more. Things have changed and this is no longer needed.