Comment 1 Eitan Isaacson [:eeejay] 2012-05-03 16:58:46 PDT

Created attachment 620900 [details] [diff] [review]
Use accessibility.accessfu to enable or disable accessfu.

This was a bit too easy. But it seems to work.

Comment 2 David Bolter [:davidb] 2012-05-03 18:56:59 PDT

Comment on attachment 620900 [details] [diff] [review]
Use accessibility.accessfu to enable or disable accessfu.

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

r=me

Please consider my thoughts below:

BTW Do you run "gjslint --nojsdoc my_file.js" ? (Just curious)

::: accessible/src/jsat/AccessFu.jsm
@@ +17,5 @@
>  Cu.import('resource://gre/modules/accessibility/VirtualCursorController.jsm');
>  
> +const ACCESSFU_DISABLED = 0;
> +const ACCESSFU_ENABLED = 1;
> +const ACCESSFU_AUTO = 2;

Note, to keep my brain happier I'd consider these prefs rather than modes throughout (as in ACCESSFU_DISABLE, ACCESSFU_ENABLE, ACCESSFU_AUTO throughout).

@@ +41,5 @@
> +    this.prefsBranch = Cc["@mozilla.org/preferences-service;1"]
> +      .getService(Ci.nsIPrefService).getBranch("accessibility.");
> +    this.prefsBranch.addObserver('accessfu', this, false);
> +
> +    let accessfuMode = ACCESSFU_DISABLED;

let accessPref = ACCESSFU_DISABLE; ?

@@ +49,3 @@
>      }
>  
> +    if (this.checkAccessibility(accessfuMode))

Optional: since it returns a boolean I'd prefer an "is" name like "isWanted" or "amINeeded" (note: in 3 places).

@@ +91,5 @@
>      this.chromeWin.removeEventListener('scroll', this);
>      this.chromeWin.removeEventListener('TabOpen', this);
>    },
>  
> +  checkAccessibility: function(aMode) {

s/aMode/aPref ?

Comment 3 Eitan Isaacson [:eeejay] 2012-05-03 19:09:19 PDT

(In reply to David Bolter [:davidb] from comment #2)
> Comment on attachment 620900 [details] [diff] [review]
> @@ +49,3 @@
> >      }
> >  
> > +    if (this.checkAccessibility(accessfuMode))
> 
> Optional: since it returns a boolean I'd prefer an "is" name like "isWanted"
> or "amINeeded" (note: in 3 places).
> 

thanks! I changed it to isEnabled. All the rest of the names are adjusted too.

Comment 4 Eitan Isaacson [:eeejay] 2012-05-03 19:10:37 PDT

(In reply to David Bolter [:davidb] from comment #2)
> BTW Do you run "gjslint --nojsdoc my_file.js" ? (Just curious)
> 

yes, although i think i missed it in the text event patch from the other day.
I run:
gjslint --additional_extensions=jsm --nojsdoc *.jsm

Comment 5 Eitan Isaacson [:eeejay] 2012-05-03 19:18:46 PDT

Created attachment 620930 [details] [diff] [review]
Use accessibility.accessfu to enable or disable accessfu.

Comment 6 Eitan Isaacson [:eeejay] 2012-05-03 19:24:46 PDT

Created attachment 620931 [details] [diff] [review]
Bug 750528 - Use accessibility.accessfu to enable or disable accessfu. r=davidb

Comment 7 Eitan Isaacson [:eeejay] 2012-05-03 19:29:34 PDT

http://hg.mozilla.org/integration/mozilla-inbound/rev/7972b92116b0

Comment 8 alexander :surkov 2012-05-03 22:47:09 PDT

Comment on attachment 620931 [details] [diff] [review]
Bug 750528 - Use accessibility.accessfu to enable or disable accessfu. r=davidb

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

::: accessible/src/jsat/AccessFu.jsm
@@ +49,3 @@
>      }
>  
> +    if (this.amINeeded(accessPref))

at some point David should try to write lyrics instead code ;) alternatively named functions aren't always great for code reading.

@@ +52,1 @@
>        this.enable();

I'd keep enable a part of amINeeded since it seems nobody need to turn it on/off explicitly.

Comment 9 David Bolter [:davidb] 2012-05-04 06:37:30 PDT

Eitan if you happen to follow Alexander and move the enable call inside the boolean function that checks preferences please rename it so that the side effect (yuck) is obvious.

Comment 10 Ed Morley [:emorley] 2012-05-04 11:45:30 PDT

https://hg.mozilla.org/mozilla-central/rev/7972b92116b0

Comment 11 Eitan Isaacson [:eeejay] 2012-05-04 11:46:51 PDT

(In reply to Ed Morley [:edmorley] from comment #10)
> https://hg.mozilla.org/mozilla-central/rev/7972b92116b0

Wrong bug!

Comment 12 Eitan Isaacson [:eeejay] 2012-05-04 11:47:14 PDT

Oops. my bad