[AccessFu] Introduce utility module

RESOLVED FIXED in mozilla16

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: eeejay, Assigned: eeejay)

Tracking

Trunk
mozilla16
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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).
(Assignee)

Comment 1

5 years ago
Created attachment 634527 [details] [diff] [review]
Introduce Utils module in jsat.

This also cleans up some accumulated lint.
Attachment #634527 - Flags: review?(dbolter)
(Assignee)

Comment 2

5 years ago
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)
(Assignee)

Comment 3

5 years ago
Created attachment 634534 [details] [diff] [review]
Introduce Utils module in jsat.

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+
(Assignee)

Comment 7

5 years ago
(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??).
(Assignee)

Comment 8

5 years ago
Created attachment 635061 [details] [diff] [review]
Introduce Utils module in jsat. r=davidb

Nits addressed.
Attachment #634534 - Attachment is obsolete: true
(Assignee)

Comment 9

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Blocks: 757721
You need to log in before you can comment on or make changes to this bug.