838877 - Change policy handling to permit direct pref modification

Status: RESOLVED FIXED

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

Description

[Richard Newman [:rnewman]]

We recently changed this so that enabling or disabling went through a function call, so that we can schedule a remote deletion.

This isn't well suited for Android: we don't want to ship the JS Bagheera client, the Gecko side of FHR won't even know the document ID, and our life is easier if we can mirror the pref directly.

We'd like to switch to using pref observers, which also buys us correct behavior if a user pokes around in about:config.

The function call can stay for convenience access.

Comment 2

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v1

Watch the upload enablement pref, deleting accordingly.

I think this is the only pref we change from the UI that requires observation. Might find more as we go.

Comment 3

[Richard Newman [:rnewman]]

Note that we don't have an end-of-lifecycle component for the policy object, so it doesn't unhook its observer.

Comment 4

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v2

The previous version broke a single test, which was expecting pref changes to not cause a deletion -- an outstanding deletion will cause updates to be delayed.

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 713156 [details] [diff] [review]
Proposed patch. v2

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

Looks good to me.

::: services/datareporting/policy.jsm
@@ +710,5 @@
> +    let result = null;
> +    if (!flag) {
> +      result = this.deleteRemoteData(reason);
> +    }
> +      

If you are going to change style only, please don't introduce leading whitespace :P

Comment 6

[Richard Newman [:rnewman]]

https://hg.mozilla.org/services/services-central/rev/430b8f9868f8

There might be more once Chenxia's done with UX.

Comment 7

[Richard Newman [:rnewman]]

https://hg.mozilla.org/mozilla-central/rev/430b8f9868f8