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

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Yura Zenevich [:yzen] (please use "needinfo")]

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

Comment 1

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 967238.patch

Comment 2

[Eitan Isaacson [:eeejay]]

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.

Comment 3

[Yura Zenevich [:yzen] (please use "needinfo")]

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.

Comment 4

[Yura Zenevich [:yzen] (please use "needinfo")]

^ 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.

Comment 5

[Yura Zenevich [:yzen] (please use "needinfo")]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea2e099d80e2

Comment 6

[Carsten Book [:Tomcat]]

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