Bug 72570 - AX: cleanup style and naming and code in accessibility search mechanism
Summary: AX: cleanup style and naming and code in accessibility search mechanism
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: chris fleizach
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-16 17:40 PST by chris fleizach
Modified: 2011-11-17 13:16 PST (History)
1 user (show)

See Also:

Attachments
patch (14.49 KB, patch)
2011-11-16 17:58 PST, chris fleizach 		bdakin: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description chris fleizach 2011-11-16 17:40:43 PST 
Original implementation was done by an intern. I think the naming and coding need to be improved and shaped
Comment 1 chris fleizach 2011-11-16 17:58:18 PST 
Created attachment 115497 [details]
patch
Comment 2 Beth Dakin 2011-11-17 11:49:14 PST 
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?
Comment 3 chris fleizach 2011-11-17 11:52:24 PST 
(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
Comment 4 Beth Dakin 2011-11-17 11:55:22 PST 
(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!
Comment 5 chris fleizach 2011-11-17 13:16:15 PST 
http://trac.webkit.org/changeset/100662