Closed Bug 768997 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Disability Access APIs, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: hub, Assigned: hub)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
Blocks: 772060
No longer blocks: 772060
it makes DEBUG unusable on Mac.
Assignee: nobody → hub
Attachment #641278 - Flags: review?(dbolter)
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?
(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.
(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.
(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 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.
Attachment #641278 - Flags: review?(dbolter) → review+
https://hg.mozilla.org/mozilla-central/rev/aec3f38712a3
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
I prefer something like DEBUG_DISABLED to stress out what you do and file a bug to sort out IsTextInterfaceSupportCorrect things.
You need to log in before you can comment on or make changes to this bug.