Last Comment Bug 764119 - [AccessFu] Add preference for explore by touch
: [AccessFu] Add preference for explore by touch
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: mozilla16
Assigned To: Eitan Isaacson [:eeejay]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-12 13:06 PDT by Eitan Isaacson [:eeejay]
Modified: 2012-06-14 02:47 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Explore by touch preference (7.69 KB, patch)
2012-06-12 13:08 PDT, Eitan Isaacson [:eeejay]
dbolter: review+
Details | Diff | Splinter Review

Description Eitan Isaacson [:eeejay] 2012-06-12 13:06:20 PDT
This adds the preference, and a minor refactor. I tried to make it as readable and simple as possible.
Comment 1 Eitan Isaacson [:eeejay] 2012-06-12 13:08:29 PDT
Created attachment 632374 [details] [diff] [review]
Explore by touch preference
Comment 2 David Bolter [:davidb] 2012-06-12 13:24:05 PDT
Comment on attachment 632374 [details] [diff] [review]
Explore by touch preference

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

::: accessible/src/jsat/AccessFu.jsm
@@ +101,5 @@
> +  _processPreferences: function _processPreferences(aEnabled, aTouchEnabled) {
> +    let accessPref = ACCESSFU_DISABLE;
> +    try {
> +      accessPref = (aEnabled == undefined) ?
> +        this.prefsBranch.getIntPref('activate') : aEnabled;

You could write:
accessPref = aEnabled || this.prefsBranch.getIntPref('activate');

(same for the ebt case)

@@ -113,5 @@
>          return;
>        }
> -
> -      if (this._observingSystemSettings) {
> -        Services.obs.removeObserver(this, 'Accessibility:Settings');

Where do you remove this observer?
Comment 3 Eitan Isaacson [:eeejay] 2012-06-12 13:30:30 PDT
(In reply to David Bolter [:davidb] from comment #2)
> Comment on attachment 632374 [details] [diff] [review]
> Explore by touch preference
> 
> Review of attachment 632374 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/jsat/AccessFu.jsm
> @@ +101,5 @@
> > +  _processPreferences: function _processPreferences(aEnabled, aTouchEnabled) {
> > +    let accessPref = ACCESSFU_DISABLE;
> > +    try {
> > +      accessPref = (aEnabled == undefined) ?
> > +        this.prefsBranch.getIntPref('activate') : aEnabled;
> 
> You could write:
> accessPref = aEnabled || this.prefsBranch.getIntPref('activate');
> 
> (same for the ebt case)
> 

I don't think I can because I want it to check out as true when aEnabled is 0. So I need to explicitly check if the argument was passed.

> @@ -113,5 @@
> >          return;
> >        }
> > -
> > -      if (this._observingSystemSettings) {
> > -        Services.obs.removeObserver(this, 'Accessibility:Settings');
> 
> Where do you remove this observer?

I never do. Gave up on that. I don't think it is that bad. attach() could only be run once, so there is no concern of adding the observer more than once.
Comment 4 David Bolter [:davidb] 2012-06-12 13:35:36 PDT
(In reply to Eitan Isaacson [:eeejay] from comment #3)
> (In reply to David Bolter [:davidb] from comment #2)
> > Comment on attachment 632374 [details] [diff] [review]
> > Explore by touch preference
> > 
> > Review of attachment 632374 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: accessible/src/jsat/AccessFu.jsm
> > @@ +101,5 @@
> > > +  _processPreferences: function _processPreferences(aEnabled, aTouchEnabled) {
> > > +    let accessPref = ACCESSFU_DISABLE;
> > > +    try {
> > > +      accessPref = (aEnabled == undefined) ?
> > > +        this.prefsBranch.getIntPref('activate') : aEnabled;
> > 
> > You could write:
> > accessPref = aEnabled || this.prefsBranch.getIntPref('activate');
> > 
> > (same for the ebt case)
> > 
> 
> I don't think I can because I want it to check out as true when aEnabled is
> 0. So I need to explicitly check if the argument was passed.

Oh! I see.

> > @@ -113,5 @@
> > >          return;
> > >        }
> > > -
> > > -      if (this._observingSystemSettings) {
> > > -        Services.obs.removeObserver(this, 'Accessibility:Settings');
> > 
> > Where do you remove this observer?
> 
> I never do. Gave up on that. I don't think it is that bad. attach() could
> only be run once, so there is no concern of adding the observer more than
> once.

I guess that's ok?
Comment 5 Eitan Isaacson [:eeejay] 2012-06-12 13:45:35 PDT
(In reply to David Bolter [:davidb] from comment #4)
> (In reply to Eitan Isaacson [:eeejay] from comment #3)
> > (In reply to David Bolter [:davidb] from comment #2)
> > > Comment on attachment 632374 [details] [diff] [review]
> > > Explore by touch preference
> > > 
> > > Review of attachment 632374 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: accessible/src/jsat/AccessFu.jsm
> > > @@ -113,5 @@
> > > >          return;
> > > >        }
> > > > -
> > > > -      if (this._observingSystemSettings) {
> > > > -        Services.obs.removeObserver(this, 'Accessibility:Settings');
> > > 
> > > Where do you remove this observer?
> > 
> > I never do. Gave up on that. I don't think it is that bad. attach() could
> > only be run once, so there is no concern of adding the observer more than
> > once.
> 
> I guess that's ok?

The patch in bug 764057 allows Accessibility:Settings to be sent in an unsolicited manner and we need to catch it.

Also, I think I was trying to be fancy earlier and it was silly. This is hardly readable already, don't need to do that dance. No real gain.
Comment 6 David Bolter [:davidb] 2012-06-12 17:25:49 PDT
Comment on attachment 632374 [details] [diff] [review]
Explore by touch preference

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

r=me since I think this only improves the current situation. I do worry about not cleaning up observers and I think we probably need a cleanup function to be called during shutdown but we'll need to check with front-end folks on the blessed way to do that (and if it is indeed necessary).
Comment 7 Eitan Isaacson [:eeejay] 2012-06-13 12:24:37 PDT
(In reply to David Bolter [:davidb] from comment #6)
> Comment on attachment 632374 [details] [diff] [review]
> Explore by touch preference
> 
> Review of attachment 632374 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me since I think this only improves the current situation. I do worry
> about not cleaning up observers and I think we probably need a cleanup
> function to be called during shutdown but we'll need to check with front-end
> folks on the blessed way to do that (and if it is indeed necessary).

We don't yet have an AccessFu.detach, but if we did, that is where we would remove the observer. Connecting an observer for one event and then detaching it seems a bit like an antipattern to me.
Comment 8 Eitan Isaacson [:eeejay] 2012-06-13 12:24:59 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/ad0775733110
Comment 9 David Bolter [:davidb] 2012-06-13 12:40:55 PDT
(In reply to Eitan Isaacson [:eeejay] from comment #7)
> (In reply to David Bolter [:davidb] from comment #6)
> > Comment on attachment 632374 [details] [diff] [review]
> > Explore by touch preference
> > 
> > Review of attachment 632374 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r=me since I think this only improves the current situation. I do worry
> > about not cleaning up observers and I think we probably need a cleanup
> > function to be called during shutdown but we'll need to check with front-end
> > folks on the blessed way to do that (and if it is indeed necessary).
> 
> We don't yet have an AccessFu.detach, but if we did, that is where we would
> remove the observer.

Yes. Exactly. Extensions for example have something like this where they are expected to do cleanup (including removing observers). I'm not 100% sure how cleanup should happen in the accessfu case but if we have a detach then of course there. JS objects don't have destructor calls so we can't do the obvious.
Comment 10 Ed Morley [:emorley] 2012-06-14 02:47:03 PDT
https://hg.mozilla.org/mozilla-central/rev/ad0775733110

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