Bug 243920

Summary: AX: Remove ~30 ASSERT_NOT_REACHED AXIsolatedObject functions by moving them out of AXCoreObject interface
Product: WebKit Reporter: Tyler Wilcock <tyler_w>
Component: AccessibilityAssignee: Tyler Wilcock <tyler_w>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, andresg_22, apinheiro, cfleizach, dmazzoni, ews-watchlist, hi, jcraig, jdiggs, joepeck, pangle, samuel_white, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Tyler Wilcock
Reported 2022-08-13 14:08:33 PDT
Remove ~30 ASSERT_NOT_REACHED AXIsolatedObject functions by moving them out of AXCoreObject interface
Attachments
Patch (27.05 KB, patch)
2022-08-13 14:14 PDT, Tyler Wilcock 		no flags
DetailsFormatted DiffDiff
Patch (38.59 KB, patch)
2022-08-14 19:01 PDT, Tyler Wilcock 		no flags
DetailsFormatted DiffDiff
Patch (39.78 KB, patch)
2022-08-14 19:13 PDT, Tyler Wilcock 		no flags
DetailsFormatted DiffDiff
Patch (43.53 KB, patch)
2022-08-15 20:08 PDT, Tyler Wilcock 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2022-08-13 14:08:43 PDT
<rdar://problem/98622522>
Tyler Wilcock
Comment 2 2022-08-13 14:14:44 PDT
Created attachment 461599 [details] Patch
Tyler Wilcock
Comment 3 2022-08-14 19:01:14 PDT
Created attachment 461628 [details] Patch
Tyler Wilcock
Comment 4 2022-08-14 19:13:08 PDT
Created attachment 461629 [details] Patch
chris fleizach
Comment 5 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?
Tyler Wilcock
Comment 6 2022-08-15 20:08:06 PDT
Created attachment 461660 [details] Patch
Tyler Wilcock
Comment 7 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.
EWS
Comment 8 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].
Note You need to log in before you can comment on or make changes to this bug.