865023 - [AccessFu] Streamline preference getting and observing

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Eitan Isaacson [:eeejay]]

There is a common pattern of getting a preference (which may or may not exist), caching it, and doing things when it changes. This should all be a common think in Utils.jsm

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Introduce a prefs cache and listener utility.

Comment 2

[Eitan Isaacson [:eeejay]]

Comment on attachment 752878 [details] [diff] [review]
Introduce a prefs cache and listener utility.

Never mind! I have something else in the works.

Comment 3

[Eitan Isaacson [:eeejay]]

Attachment: Bug 865023 - Introduce a prefs cache and listener utility.

Ok, this is more sane..

Comment 4

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

Comment on attachment 752976 [details] [diff] [review]
Bug 865023 - Introduce a prefs cache and listener utility.

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

r=me with the following changes:

::: accessible/src/jsat/AccessFu.jsm
@@ +41,5 @@
>      }
>  
> +    this._activatePref = new PrefCache(
> +      'accessibility.accessfu.activate',
> +      function() {

You can try:
() => {
  this._enableOrDisable();
}
(No need to bind then, since parent scope's this will be used).

Or even better:
this._activatePref = new PrefCache(
  'accessibility.accessfu.activate',
  this._enableOrDisable, true);

@@ +97,5 @@
> +    // Populate quicknav modes
> +    this._quicknavModesPref =
> +      new PrefCache(
> +        'accessibility.accessfu.quicknav_modes',
> +        function (aName, aValue) {

Same thing as above: with ()=>{} notation, you won't need to bind.

::: accessible/src/jsat/Utils.jsm
@@ +438,5 @@
> +  let branch = Services.prefs;
> +  this.value = this._getValue(branch);
> +
> +  if (this.callback && aRunCallbackNow) {
> +    this.callback(this.name, this.value);

This callback is wrapped in try/catch in observer. Perhaps, this should be the same here?

@@ +445,5 @@
> +  branch.addObserver(aName, this, true);
> +};
> +
> +PrefCache.prototype = {
> +  _getValue: function _getValue(aBranch) {

If the type is not expected to change, maybe we should determine this._getValue in constuctor?

::: accessible/src/jsat/UtteranceGenerator.jsm
@@ +16,5 @@
>  const UTTERANCE_DESC_FIRST = 0;
>  
> +Cu.import('resource://gre/modules/accessibility/Utils.jsm');
> +
> +var gUtteranceOrder = new PrefCache('accessibility.accessfu.utterance');

NIT: let gUtteranceOrder

@@ +74,5 @@
>        utterance.push.apply(utterance,
>          UtteranceGenerator.genForObject(aAccessible));
>      };
>  
> +    let utteranceOrder = gUtteranceOrder.value || 0;

NIT: better use UTTERANCE_DESC_FIRST rather than 0. This way it will be clear what 0 means.

@@ +314,5 @@
>    },
>  
>    _addName: function _addName(utterance, aAccessible, aFlags) {
>      let name = (aFlags & INCLUDE_NAME) ? (aAccessible.name || '') : '';
> +    let utteranceOrder = gUtteranceOrder.value || 0;

NIT: better use UTTERANCE_DESC_FIRST rather than 0.

Comment 5

[Eitan Isaacson [:eeejay]]

(In reply to Yura Zenevich [:yzen] from comment #4)
> Comment on attachment 752976 [details] [diff] [review]
> Bug 865023 - Introduce a prefs cache and listener utility.
> 
> Review of attachment 752976 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +97,5 @@
> > +    // Populate quicknav modes
> > +    this._quicknavModesPref =
> > +      new PrefCache(
> > +        'accessibility.accessfu.quicknav_modes',
> > +        function (aName, aValue) {
> 
> Same thing as above: with ()=>{} notation, you won't need to bind.
> 

Scary! My syntax highlighter does not even like that yet. But it indeed works.

> ::: accessible/src/jsat/Utils.jsm
> @@ +438,5 @@
> > +  let branch = Services.prefs;
> > +  this.value = this._getValue(branch);
> > +
> > +  if (this.callback && aRunCallbackNow) {
> > +    this.callback(this.name, this.value);
> 
> This callback is wrapped in try/catch in observer. Perhaps, this should be
> the same here?
> 

The observer is called asynchronously, and needs it for us to output an error. I'm adding it here too, because in some places it is called from the top level, like in the utterance module.

> @@ +445,5 @@
> > +  branch.addObserver(aName, this, true);
> > +};
> > +
> > +PrefCache.prototype = {
> > +  _getValue: function _getValue(aBranch) {
> 
> If the type is not expected to change, maybe we should determine
> this._getValue in constuctor?
> 

Not always, a pref could not exist, and be set later to specific type, we would need to then retrieve the type.

Comment 6

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

(In reply to Eitan Isaacson [:eeejay] from comment #5)
> > @@ +445,5 @@
> > > +  branch.addObserver(aName, this, true);
> > > +};
> > > +
> > > +PrefCache.prototype = {
> > > +  _getValue: function _getValue(aBranch) {
> > 
> > If the type is not expected to change, maybe we should determine
> > this._getValue in constuctor?
> > 

Ah, good point!
> 
> Not always, a pref could not exist, and be set later to specific type, we
> would need to then retrieve the type.

Comment 7

[Eitan Isaacson [:eeejay]]

Attachment: Bug 865023 - Introduce a prefs cache and listener utility. r=yzen

Fixed nits. Landing..

Comment 8

[Eitan Isaacson [:eeejay]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/51d94f573e26

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/51d94f573e26