Last Comment Bug 750528 - [AccessFu] Add preference for enabling/disabling AccessFu
: [AccessFu] Add preference for enabling/disabling AccessFu
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: mozilla15
Assigned To: Eitan Isaacson [:eeejay]
:
:
Mentors:
Depends on:
Blocks: 749719
  Show dependency treegraph
 
Reported: 2012-04-30 15:14 PDT by Eitan Isaacson [:eeejay]
Modified: 2012-05-04 11:47 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Use accessibility.accessfu to enable or disable accessfu. (4.38 KB, patch)
2012-05-03 16:58 PDT, Eitan Isaacson [:eeejay]
dbolter: review+
Details | Diff | Splinter Review
Use accessibility.accessfu to enable or disable accessfu. (4.44 KB, patch)
2012-05-03 19:18 PDT, Eitan Isaacson [:eeejay]
no flags Details | Diff | Splinter Review
Bug 750528 - Use accessibility.accessfu to enable or disable accessfu. r=davidb (4.46 KB, patch)
2012-05-03 19:24 PDT, Eitan Isaacson [:eeejay]
no flags Details | Diff | Splinter Review

Description Eitan Isaacson [:eeejay] 2012-04-30 15:14:59 PDT
I am thinking accessibility.accessfu (int)
0: Disabled
1: Enabled
2: Auto

Auto is the default and it will sniff platform a11y and enabled or disable AccessFu.

On platforms that either don't have platform sniffing enabled in AccessFu (Mac, Windows, Linux), or platforms that don't have "platform a11y" outside of Firefox (B2G), auto is synonymous with disabled.
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

Note You need to log in before you can comment on or make changes to this bug.