Bug 72570 – AX: cleanup style and naming and code in accessibility search mechanism

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[chris fleizach]

Original implementation was done by an intern. I think the naming and coding need to be improved and shaped

[chris fleizach]

Created attachment 115497 [details]
patch

[Beth Dakin]

Comment on attachment 115497 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115497&action=review

Definite improvement!

> Source/WebCore/accessibility/AccessibilityObject.cpp:-333
> -    ASSERT(AXObjectCache::accessibilityEnabled());

Did you mean to remove this assertion? If so, why?

[chris fleizach]

(In reply to comment #2)
> (From update of attachment 115497 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115497&action=review
> 
> Definite improvement!
> 
> > Source/WebCore/accessibility/AccessibilityObject.cpp:-333
> > -    ASSERT(AXObjectCache::accessibilityEnabled());
> 
> Did you mean to remove this assertion? If so, why?

I don't really know why it was added in the first place. I definitely should have told the intern to remove it. 

Asserting that is true is quite meaningless. 

No one will use this method unless it's been triggered through an existing accessibility mechanism (which means that accessibility is already enabled)... 

This method is really no different than any other method, so it doesn't harm anything to have it there, but it makes it confusing as to why it's there

[Beth Dakin]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 115497 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=115497&action=review
> > 
> > Definite improvement!
> > 
> > > Source/WebCore/accessibility/AccessibilityObject.cpp:-333
> > > -    ASSERT(AXObjectCache::accessibilityEnabled());
> > 
> > Did you mean to remove this assertion? If so, why?
> 
> I don't really know why it was added in the first place. I definitely should have told the intern to remove it. 
> 
> Asserting that is true is quite meaningless. 
> 
> No one will use this method unless it's been triggered through an existing accessibility mechanism (which means that accessibility is already enabled)... 
> 
> This method is really no different than any other method, so it doesn't harm anything to have it there, but it makes it confusing as to why it's there

Makes sense!

[chris fleizach]

http://trac.webkit.org/changeset/100662