Last Comment Bug 766238 - [AccessFu] Introduce utility module
: [AccessFu] Introduce utility module
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: 757721
  Show dependency treegraph
 
Reported: 2012-06-19 11:58 PDT by Eitan Isaacson [:eeejay]
Modified: 2012-06-21 05:40 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Introduce Utils module in jsat. (18.27 KB, patch)
2012-06-19 12:00 PDT, Eitan Isaacson [:eeejay]
no flags Details | Diff | Splinter Review
Introduce Utils module in jsat. (18.78 KB, patch)
2012-06-19 12:13 PDT, Eitan Isaacson [:eeejay]
dbolter: review+
Details | Diff | Splinter Review
Introduce Utils module in jsat. r=davidb (18.80 KB, patch)
2012-06-20 14:06 PDT, Eitan Isaacson [:eeejay]
no flags Details | Diff | Splinter Review

Description Eitan Isaacson [:eeejay] 2012-06-19 11:58:31 PDT
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] 2012-06-19 12:00:24 PDT
Created attachment 634527 [details] [diff] [review]
Introduce Utils module in jsat.

This also cleans up some accumulated lint.
Comment 2 Eitan Isaacson [:eeejay] 2012-06-19 12:03:09 PDT
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] 2012-06-19 12:13:42 PDT
Created attachment 634534 [details] [diff] [review]
Introduce Utils module in jsat.

Ok, this time for real.
Comment 4 David Bolter [:davidb] 2012-06-19 12:58:41 PDT
Gavin do we have something like Eitan's Logger available already?
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-19 13:02:22 PDT
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] 2012-06-19 13:39:10 PDT
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] 2012-06-20 14:02:52 PDT
(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] 2012-06-20 14:06:20 PDT
Created attachment 635061 [details] [diff] [review]
Introduce Utils module in jsat. r=davidb

Nits addressed.
Comment 9 Eitan Isaacson [:eeejay] 2012-06-20 14:10:30 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/f08d99730f33
Comment 10 Ed Morley [:emorley] 2012-06-21 04:04:31 PDT
https://hg.mozilla.org/mozilla-central/rev/f08d99730f33

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