768997 - nsAccUtils::IsTextInterfaceSupportCorrect() is too expensive and called too often in DEBUG

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Hubert Figuiere [:hub]]

nsAccUtils::IsTextInterfaceSupportCorrect() is too expensive and called too often in DEBUG

Bug 750191 made it build unconditionally in DEBUG which is why we get it all the time.

It is called when querying the role, on all platform but I noticed it on Mac.

Comment 1

[Hubert Figuiere [:hub]]

it makes DEBUG unusable on Mac.

Comment 2

[Hubert Figuiere [:hub]]

Attachment: nsAccUtils::IsTextInterfaceSupportCorrect() is no longer called in DEBUG on Mac. r=

Comment 3

[Trevor Saunders (:tbsaunde)]

Comment on attachment 641278 [details] [diff] [review]
nsAccUtils::IsTextInterfaceSupportCorrect() is no longer called in DEBUG on Mac. r=

>-#ifdef DEBUG
>+#ifdef DEBUG_a11y

unless someone wants to own getting rid of all the one off DEBUG_FOO around, and make sure DEBUG_A11Y keeps building I'd rather not add uses of DEBUG_A11Y.

what's wrong with making IsTextInterfaceSupportCorrect() faster? it doesn't look terribly hard to make atleast a bit faster, but I'm not clear on how often its called on mac.

Also do you actually ever build with DEBUG_A11Y? or would it make sense to just remove this all together?

Comment 4

[Hubert Figuiere [:hub]]

(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> Comment on attachment 641278 [details] [diff] [review]
> nsAccUtils::IsTextInterfaceSupportCorrect() is no longer called in DEBUG on
> Mac. r=
> 
> >-#ifdef DEBUG
> >+#ifdef DEBUG_a11y
> 
> unless someone wants to own getting rid of all the one off DEBUG_FOO around,
> and make sure DEBUG_A11Y keeps building I'd rather not add uses of
> DEBUG_A11Y.
> 
> what's wrong with making IsTextInterfaceSupportCorrect() faster? it doesn't
> look terribly hard to make atleast a bit faster,

I tend to just think that debug only code is debug only code, then I shouldn't bother.

> but I'm not clear on how
> often its called on mac.

It is called each time we call -[mozAccessible role], which is more often than not. Now maybe I should just call it somewhere else.

> Also do you actually ever build with DEBUG_A11Y? or would it make sense to
> just remove this all together?

I had never, but bug 750191 did cause this to come into the picture.

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to Hub Figuiere [:hub] from comment #4)
> (In reply to Trevor Saunders (:tbsaunde) from comment #3)
> > Comment on attachment 641278 [details] [diff] [review]
> > nsAccUtils::IsTextInterfaceSupportCorrect() is no longer called in DEBUG on
> > Mac. r=
> > 
> > >-#ifdef DEBUG
> > >+#ifdef DEBUG_a11y
> > 
> > unless someone wants to own getting rid of all the one off DEBUG_FOO around,
> > and make sure DEBUG_A11Y keeps building I'd rather not add uses of
> > DEBUG_A11Y.
> > 
> > what's wrong with making IsTextInterfaceSupportCorrect() faster? it doesn't
> > look terribly hard to make atleast a bit faster,
> 
> I tend to just think that debug only code is debug only code, then I
> shouldn't bother.

well, its mostly deCOMtamination stuff which needs to happen eventually anyway...

> > Also do you actually ever build with DEBUG_A11Y? or would it make sense to
> > just remove this all together?
> 
> I had never, but bug 750191 did cause this to come into the picture.

ok, it wouldn't suprise me if it doesn't build then.

Comment 6

[Hubert Figuiere [:hub]]

(In reply to Trevor Saunders (:tbsaunde) from comment #5)

> > > what's wrong with making IsTextInterfaceSupportCorrect() faster? it doesn't
> > > look terribly hard to make atleast a bit faster,
> > 
> > I tend to just think that debug only code is debug only code, then I
> > shouldn't bother.
> 
> well, its mostly deCOMtamination stuff which needs to happen eventually
> anyway...

So maybe I can just get away with removing it? I'm fine with that.

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 641278 [details] [diff] [review]
nsAccUtils::IsTextInterfaceSupportCorrect() is no longer called in DEBUG on Mac. r=

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

If this helps your mac debugging, fine with me.

::: accessible/src/mac/mozAccessible.mm
@@ +411,5 @@
>  {
>    if (!mGeckoAccessible)
>      return nil;
>  
> +#ifdef DEBUG_a11y

It should be DEBUG_A11Y, all caps.

Comment 8

[Hubert Figuiere [:hub]]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[alexander :surkov (:asurkov)]

I prefer something like DEBUG_DISABLED to stress out what you do and file a bug to sort out IsTextInterfaceSupportCorrect things.