Bug 243920 - AX: Remove ~30 ASSERT_NOT_REACHED AXIsolatedObject functions by moving them out of AXCoreObject interface
Summary: AX: Remove ~30 ASSERT_NOT_REACHED AXIsolatedObject functions by moving them o...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tyler Wilcock
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-08-13 14:08 PDT by Tyler Wilcock
Modified: 2022-08-16 22:36 PDT (History)
13 users (show)

See Also:


Attachments
Patch (27.05 KB, patch)
2022-08-13 14:14 PDT, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (38.59 KB, patch)
2022-08-14 19:01 PDT, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (39.78 KB, patch)
2022-08-14 19:13 PDT, Tyler Wilcock
no flags Details | Formatted Diff | Diff
Patch (43.53 KB, patch)
2022-08-15 20:08 PDT, Tyler Wilcock
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tyler Wilcock 2022-08-13 14:08:33 PDT
Remove ~30 ASSERT_NOT_REACHED AXIsolatedObject functions by moving them out of AXCoreObject interface
Comment 1 Radar WebKit Bug Importer 2022-08-13 14:08:43 PDT
<rdar://problem/98622522>
Comment 2 Tyler Wilcock 2022-08-13 14:14:44 PDT
Created attachment 461599 [details]
Patch
Comment 3 Tyler Wilcock 2022-08-14 19:01:14 PDT
Created attachment 461628 [details]
Patch
Comment 4 Tyler Wilcock 2022-08-14 19:13:08 PDT
Created attachment 461629 [details]
Patch
Comment 5 chris fleizach 2022-08-14 22:57:37 PDT
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?
Comment 6 Tyler Wilcock 2022-08-15 20:08:06 PDT
Created attachment 461660 [details]
Patch
Comment 7 Tyler Wilcock 2022-08-15 20:13:59 PDT
(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.
Comment 8 EWS 2022-08-16 22:36:28 PDT
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].