766238 - [AccessFu] Introduce utility module

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Eitan Isaacson [:eeejay]]

The time has come.. for a module where we could keep common functions.

The goals are:
1. A place for common functions so we don't duplicate things across js modules.
2. A place where most if not all platform sniffing and abstractions should live.
3. Something instead of dump() for logging. I keep on inserting dumps to debug bits and removing them later (sometimes not, and this introduces unperformant crud).

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Introduce Utils module in jsat.

This also cleans up some accumulated lint.

Comment 2

[Eitan Isaacson [:eeejay]]

Comment on attachment 634527 [details] [diff] [review]
Introduce Utils module in jsat.

Some bits here are untested. Sorry, going to resubmit in a few minutes.

Comment 3

[Eitan Isaacson [:eeejay]]

Attachment: Introduce Utils module in jsat.

Ok, this time for real.

Comment 4

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

Gavin do we have something like Eitan's Logger available already?

Comment 5

[:Gavin Sharp [email: [e-mail]]]

There's services/common/log4moz.js, but I think it's more complicated and is only really used by services code for the moment (I think it originated as labs code).

The Logger code in that patch seems small enough that you really shouldn't worry about duplicated functionality, particularly since it will be isolated to a11y code.

Comment 6

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

Comment on attachment 634534 [details] [diff] [review]
Introduce Utils module in jsat.

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

r=me with nits comments addressed.

::: accessible/src/jsat/Presenters.jsm
@@ +152,5 @@
>        aContext.accessible.scrollTo(
>          Ci.nsIAccessibleScrollType.SCROLL_TYPE_ANYWHERE);
>        this._highlight(aContext.accessible);
>      } catch (e) {
> +      Logger.error('Error getting bounds: ' + e);

Nit: you don't need 'Error' here right?

::: accessible/src/jsat/Utils.jsm
@@ +28,5 @@
> +      let matches = shellVersion.match(/\((\d+)\)$/);
> +      if (matches)
> +        this._AndroidSdkVersion = parseInt(matches[1]);
> +      else
> +        this._AndroidSdkVersion = 15; // Most useful in desktop debugging.

I'll take your word for it.

@@ +39,5 @@
> +    this._AndroidSdkVersion = value;
> +  },
> +
> +  getBrowserApp: function getBrowserApp(aWindow) {
> +    switch (this.OS) {

Tempting to do /return (this.OS == 'Android')? foo : fee/ here and in getViewport since there are only two cases... but your call. This is readable the way you have it.

@@ +95,5 @@
> +      this, [this.ERROR].concat(Array.prototype.slice.call(arguments)));
> +  },
> +
> +  accessibleToString: function accessibleToString(aAccessible) {
> +    let str = '';

Maybe default should be 'failed' (the exception case) but your call.

@@ +105,5 @@
> +
> +    return str;
> +  },
> +
> +  eventToString: function eventToString(aEvent) {

Why no try/catch in this one?

@@ +112,5 @@
> +      let event = aEvent.QueryInterface(Ci.nsIAccessibleStateChangeEvent);
> +      let stateStrings = (event.isExtraState()) ?
> +        gAccRetrieval.getStringStates(0, event.state) :
> +        gAccRetrieval.getStringStates(event.state, 0);
> +      str += ' (' + stateStrings.item(0) + ')';

Why is it item(0)?

Comment 7

[Eitan Isaacson [:eeejay]]

(In reply to David Bolter [:davidb] from comment #6)
> Comment on attachment 634534 [details] [diff] [review]
> Introduce Utils module in jsat.
> 
> Review of attachment 634534 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with nits comments addressed.
> 
> ::: accessible/src/jsat/Presenters.jsm
> @@ +152,5 @@
> >        aContext.accessible.scrollTo(
> >          Ci.nsIAccessibleScrollType.SCROLL_TYPE_ANYWHERE);
> >        this._highlight(aContext.accessible);
> >      } catch (e) {
> > +      Logger.error('Error getting bounds: ' + e);
> 
> Nit: you don't need 'Error' here right?
> 

Yeah, i'll rephrase that.

> @@ +39,5 @@
> > +    this._AndroidSdkVersion = value;
> > +  },
> > +
> > +  getBrowserApp: function getBrowserApp(aWindow) {
> > +    switch (this.OS) {
> 
> Tempting to do /return (this.OS == 'Android')? foo : fee/ here and in
> getViewport since there are only two cases... but your call. This is
> readable the way you have it.
> 

I would like to keep it expandable for B2G (and maybe Thunderbird???? j/k).

> @@ +95,5 @@
> > +      this, [this.ERROR].concat(Array.prototype.slice.call(arguments)));
> > +  },
> > +
> > +  accessibleToString: function accessibleToString(aAccessible) {
> > +    let str = '';
> 
> Maybe default should be 'failed' (the exception case) but your call.
> 

The exception case is when the accessible is defunct so I will change it to '[ defunct ]'

> @@ +105,5 @@
> > +
> > +    return str;
> > +  },
> > +
> > +  eventToString: function eventToString(aEvent) {
> 
> Why no try/catch in this one?
> 

It is there for defunct accessibles.

> @@ +112,5 @@
> > +      let event = aEvent.QueryInterface(Ci.nsIAccessibleStateChangeEvent);
> > +      let stateStrings = (event.isExtraState()) ?
> > +        gAccRetrieval.getStringStates(0, event.state) :
> > +        gAccRetrieval.getStringStates(event.state, 0);
> > +      str += ' (' + stateStrings.item(0) + ')';
> 
> Why is it item(0)?

Because event.state only contains one state value (right??).

Comment 8

[Eitan Isaacson [:eeejay]]

Attachment: Introduce Utils module in jsat. r=davidb

Nits addressed.

Comment 9

[Eitan Isaacson [:eeejay]]

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

Comment 10

[Ed Morley [:emorley]]

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