110422 - Active Accessibility: Optimize nsIAccessibleProvider implementations in XBL

Status: RESOLVED FIXED

(SeaMonkey :: General, defect, P4)

Description

[Aaron Leventhal]

Jag has suggested some javascript improvements for the same lines of code we
repeated in many XBL bindings:

var accService =
Components.classes["@mozilla.org/accessibilityService;1"].getService(Components.interfaces.nsIAccessibilityService);
return (accService? accService.createXULTextAccessible(this): null);

Quoted from Jag:

There is no need to check whether accService is null. If anything fails, an
exception will be thrown and you'll never reach that line, so:

var accService =
Components.classes["@mozilla.org/accessibilityService;1"].getService(Components.interfaces.nsIAccessibilityService);
return accService.createXULTextAccessible(this);

This is assuming that in normal situations the getService won't fail. If there's
a chance this service isn't included while these xul widgets are, you'll need to
do something like:

const ACCESSIBILITYSERVICE_CONTRACTID = "@mozilla.org/accessibilityService;1";
if (ACCESSIBILITYSERVICE_CONTRACTID in Components.classes) {
 var accService =
Components.classes[(ACCESSIBILITYSERVICE_CONTRACTID].getService(Components.interfaces.nsIAccessibilityService);
 return accService.createXULTextAccessible(this);
}
return null;

===================
Brendan, what do you think?

Comment 1

[Aaron Leventhal]

Brendan, do you have any comments on the javascript recommended here?

Comment 2

[Brendan Eich [:brendan]]

Sure, jag has good JS fu.  I don't need to bless his advice, in general.  Yes,
use const.  Use the in operator when you need to (and not otherwise!).  And let
exceptions get thrown.

/be

Comment 3

[Aaron Leventhal]

-> John

Comment 4

[John Gaunt (redfive)]

Attachment: removal of accservice ? from xbl files

building right now to test these.

Comment 5

[John Gaunt (redfive)]

Hmmm...finally got a build completed and testing this approach. It totally
breaks venkman because it doesn't call through the MSAA api, but just enumerates
all the JS properties. So there is no service and kerplunk, kerpluey.

I'll be going with door #2 then, and will be posting a patch shortly.

btw - I think I see a significant increase in the speed of gettin accessible
objects with this fix right now, unfortunatly  I think door # 2 is going to slow
us back down.

Comment 6

[John Gaunt (redfive)]

Attachment: patch using door #2,

doesn't cause venkman fits ( Yay! ) but I don't think it's really a performance
win.

trading one comparison for an additional variable and a comparison.

Is the old way really that bad?

Comment 7

[Robert Ginda]

Comment on attachment 64425 [details] [diff] [review]
patch using door #2,

r=rginda

Comment 8

[Robert Ginda]

As jag mentioned, the old way had a useless test.  If your service wasn't there,
you never would have reached the ?: test.

I'm not sure why the exception caused a problem for venkman though.  Is the code
that calls into this XBL not dealing with the exception properly?

Comment 9

[Robert Ginda]

Comment on attachment 64425 [details] [diff] [review]
patch using door #2,

revoking r= per discussion with jgaunt

Comment 10

[John Gaunt (redfive)]

Attachment: My bad, back to the original diff

After some closer investigation spurred on by rginda, it turns out I was wrong.
It was causing venkman problems. So we get the perf win. Yay!

Also taking out a WARN_IF_FALSE that pops up and replacing with a regular if
statement.

Comment 11

[jag (Peter Annema)]

Comment on attachment 64437 [details] [diff] [review]
My bad, back to the original diff

sr=jag

Comment 12

[jag (Peter Annema)]

Though I'm surprised you're seeing a significant increase, you're only removing
one ?: statement.

Comment 13

[Robert Ginda]

Comment on attachment 64437 [details] [diff] [review]
My bad, back to the original diff

r=rginda

Comment 14

[John Gaunt (redfive)]

checked in to the trunk