Closed Bug 766238 Opened 9 years ago Closed 9 years ago

[AccessFu] Introduce utility module

Categories

(Core :: Disability Access APIs, defect)

ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: eeejay, Assigned: eeejay)

References

Details

Attachments

(1 file, 2 obsolete files)

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).
Attached patch Introduce Utils module in jsat. (obsolete) — Splinter Review
This also cleans up some accumulated lint.
Attachment #634527 - Flags: review?(dbolter)
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.
Attachment #634527 - Attachment is obsolete: true
Attachment #634527 - Flags: review?(dbolter)
Attached patch Introduce Utils module in jsat. (obsolete) — Splinter Review
Ok, this time for real.
Attachment #634534 - Flags: review?(dbolter)
Gavin do we have something like Eitan's Logger available already?
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 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)?
Attachment #634534 - Flags: review?(dbolter) → review+
(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??).
Nits addressed.
Attachment #634534 - Attachment is obsolete: true
http://hg.mozilla.org/integration/mozilla-inbound/rev/f08d99730f33
Assignee: nobody → eitan
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/f08d99730f33
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Blocks: 757721
You need to log in before you can comment on or make changes to this bug.