764119 - [AccessFu] Add preference for explore by touch

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Eitan Isaacson [:eeejay]]

This adds the preference, and a minor refactor. I tried to make it as readable and simple as possible.

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Explore by touch preference

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

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]]

(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] (NeedInfo me for attention)]

(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]]

(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] (NeedInfo me for attention)]

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]]

(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]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/ad0775733110

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

(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]]

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