960257 - [AccessFu] Introduce SettingCache in Utils

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Eitan Isaacson [:eeejay]]

This would be similar to PrefCache, but for the Settings API. Useful for screen reader settings that can be adjusted by privileged apps (like the Gaia Settings app).

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: [AccessFu] Introduce SettingCache in Utils.jsm

Comment 2

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8360635 [details] [diff] [review]
[AccessFu] Introduce SettingCache in Utils.jsm

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

Looks good, r=me with some followup regarding the inline comment.

::: accessible/src/jsat/Utils.jsm
@@ +793,5 @@
> +
> +  req.addEventListener('success', () => {
> +    this.value = req.result[aName];
> +    if (this.callback && aRunCallbackNow) {
> +      this.callback(aName, this.value, this);

Nit: slightly uncomfortable about passing this in callback. I think we are only using it to delete callback in bug 960267. Perhaps we can have a constructor arg |aOnce| that is a flag that indicates that the callback needs to be applied only once (or we can aggregate things like aDefault and aOnce into an options arg to the constructor)?

Comment 3

[Eitan Isaacson [:eeejay]]

Attachment: [AccessFu] Introduce SettingCache in Utils.jsm

I did as you suggested. Carry over the r+?

Comment 4

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8363972 [details] [diff] [review]
[AccessFu] Introduce SettingCache in Utils.jsm

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

r=me with just one more change. Looks awesome!

::: accessible/src/jsat/Utils.jsm
@@ +780,5 @@
> +  let callback = aCallback;
> +
> +  let settings = Utils.win.navigator.mozSettings;
> +  if (!settings) {
> +    if (callback && aOptions.callbackNow) {

This block

if (callback) {
  callback(aName, this.value);
  if (aOptions.callbackOnce) {
    callback = null;
  }
}

is also used in addEventListener call and when adding an observer. If we define a function for that, we can just check for aOptions.callbackNow here and in addEventListener.

Comment 5

[Eitan Isaacson [:eeejay]]

Attachment: [AccessFu] Introduce SettingCache in Utils.jsm

Did it!

Comment 6

[Eitan Isaacson [:eeejay]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb3597db9e48

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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