Bug 243920 – AX: Remove ~30 ASSERT_NOT_REACHED AXIsolatedObject functions by moving them out of AXCoreObject interface

• Component: Accessibility
• Status: RESOLVED FIXED

[Tyler Wilcock]

Remove ~30 ASSERT_NOT_REACHED AXIsolatedObject functions by moving them out of AXCoreObject interface

[Radar WebKit Bug Importer]

<rdar://problem/98622522>

[Tyler Wilcock]

Created attachment 461599 [details]
Patch

[Tyler Wilcock]

Created attachment 461628 [details]
Patch

[Tyler Wilcock]

Created attachment 461629 [details]
Patch

[chris fleizach]

Comment on attachment 461629 [details]
Patch

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

> Source/WebCore/accessibility/atspi/AccessibilityObjectAtspi.cpp:885
>  
> +    auto* liveObject = dynamicDowncast<AccessibilityObject>(m_coreObject);

do we need to check that this is not nil in all these places below? we're already checking !m_coreObject above. can we combine those checks so we don' have to modify more code below

> Source/WebCore/inspector/InspectorAuditAccessibilityObject.cpp:-178
> -        computedProperties.hidden = axObject->isHidden();

should the Audit class only be dealing in AccessibilityObjects instead of AXCoreObject?

[Tyler Wilcock]

Created attachment 461660 [details]
Patch

[Tyler Wilcock]

(In reply to chris fleizach from comment #5)
> Comment on attachment 461629 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=461629&action=review
> 
> > Source/WebCore/accessibility/atspi/AccessibilityObjectAtspi.cpp:885
> >  
> > +    auto* liveObject = dynamicDowncast<AccessibilityObject>(m_coreObject);
> 
> do we need to check that this is not nil in all these places below? we're
> already checking !m_coreObject above. can we combine those checks so we don'
> have to modify more code below
Taking your suggestion would involve changing every use of m_coreObject to liveObject in all of these functions, which feels like a big change (it wouldn't make sense to early-null-check-and-return liveObject and then use m_coreObject throughout the rest of the function).

What's in the patch now is the most minimally invasive way of changing the wrapper to handle this change, I think. In an ideal world, having this wrapper hold either AccessibilityObject* or AXIsolatedObject* (rather than AXCoreObject*) would prevent this problem.

> > Source/WebCore/inspector/InspectorAuditAccessibilityObject.cpp:-178
> > -        computedProperties.hidden = axObject->isHidden();
> 
> should the Audit class only be dealing in AccessibilityObjects instead of
> AXCoreObject?
Yes, good call.

[EWS]

Committed 253506@main (864652524aa0): <https://commits.webkit.org/253506@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 461660 [details].