[AccessFu] Avoid recreating a constructor for the internal State object in Utils.jsm.

RESOLVED FIXED in mozilla30

Status

()

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

People

(Reporter: yzen, Assigned: yzen)

Tracking

Trunk
mozilla30
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
We are currently creating a State constructor every time the getState utility is used. This needs to be taken out.

Note: also I noticed some issues with toString and fat arrow function on the device, however could not reproduce it on the gaia and firefox desktop:

I/Gecko   ( 3721): [AccessFu] ERROR Error handing accessible event:
I/Gecko   ( 3721):  "toString" is read-only
I/Gecko   ( 3721):   State@resource://gre/modules/accessibility/Utils.jsm:200
I/Gecko   ( 3721):   getState@resource://gre/modules/accessibility/Utils.jsm:219
I/Gecko   ( 3721):   genForObject@resource://gre/modules/accessibility/OutputGenerator.jsm:118
I/Gecko   ( 3721):   addOutput@resource://gre/modules/accessibility/OutputGenerator.jsm:61
I/Gecko   ( 3721):   genForContext@resource://gre/modules/accessibility/OutputGenerator.jsm:83
I/Gecko   ( 3721):   SpeechPresenter_pivotChanged@resource://gre/modules/accessibility/Presentation.jsm:466
I/Gecko   ( 3721):   Presentation_pivotChanged@resource://gre/modules/accessibility/Presentation.jsm:574
I/Gecko   ( 3721):   handleAccEvent@resource://gre/modules/accessibility/EventManager.jsm:164
I/Gecko   ( 3721):   observe@resource://gre/modules/accessibility/EventManager.jsm:592
I/Gecko   ( 3721):   moveCursorInner@chrome://global/content/accessibility/content-script.js:54
I/Gecko   ( 3721):   moveCursor@chrome://global/content/accessibility/content-script.js:76
(Assignee)

Updated

5 years ago
OS: Mac OS X → All
(Assignee)

Comment 1

5 years ago
Created attachment 8369671 [details] [diff] [review]
967238.patch
Attachment #8369671 - Flags: review?(eitan)
Comment on attachment 8369671 [details] [diff] [review]
967238.patch

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

::: accessible/src/jsat/Utils.jsm
@@ +315,5 @@
> + * State object used internally to process accessible's states.
> + * @param {Number} aBase     Base state.
> + * @param {Number} aExtended Extended state.
> + */
> +this.State = function(aBase, aExtended) {

Make this a non-anonymous function.

Since we are not exporting the 'State' symbol, I don't think it needs to be "this.", but not sure.
Attachment #8369671 - Flags: review?(eitan) → review+
(Assignee)

Comment 3

5 years ago
Afaik, this.[name] is the same as function[name](){} or name = function(){}
(In reply to Eitan Isaacson [:eeejay] from comment #2)
> Comment on attachment 8369671 [details] [diff] [review]
> 967238.patch
> 
> Review of attachment 8369671 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/jsat/Utils.jsm
> @@ +315,5 @@
> > + * State object used internally to process accessible's states.
> > + * @param {Number} aBase     Base state.
> > + * @param {Number} aExtended Extended state.
> > + */
> > +this.State = function(aBase, aExtended) {
> 
> Make this a non-anonymous function.
> 
> Since we are not exporting the 'State' symbol, I don't think it needs to be
> "this.", but not sure.
(Assignee)

Comment 4

5 years ago
^ That's in context of a jsm. I wanted to be consistent with the rest of the rest of the file, but I can still change it if you'd like.
https://hg.mozilla.org/mozilla-central/rev/ea2e099d80e2
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.