Bug 270866 – AX: WebAccessibilityObjectWrapperMac and AXIsolatedTree do many unnecessary isMainThread() calls

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

[Tyler Wilcock]

...

[Radar WebKit Bug Importer]

<rdar://problem/124468129>

[Tyler Wilcock]

Created attachment 470325 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #2)
> Created attachment 470325 [details]
> Patch

diff --git a/Source/WebCore/accessibility/AXTextMarker.cpp b/Source/WebCore/accessibility/AXTextMarker.cpp
index abfdacccb630..eb5e1128e179 100644
--- a/Source/WebCore/accessibility/AXTextMarker.cpp
+++ b/Source/WebCore/accessibility/AXTextMarker.cpp
@@ -218,8 +218,8 @@ RefPtr<AXCoreObject> AXTextMarker::object() const
         return tree ? tree->objectForID(objectID()) : nullptr;
     }
 #endif
-    auto tree = std::get<WeakPtr<AXObjectCache>>(axTreeForID(treeID()));
-    return tree ? tree->objectForID(objectID()) : nullptr;
+    auto cache = std::get<WeakPtr<AXObjectCache>>(axTreeForID(treeID()));
+    return cache ? cache->objectForID(objectID()) : nullptr;

AG: The use of tree here instead of cache was deliberate to remark the fact that AXObjectCache is also a tree as far as this code is concerned. In the future we should have a common interface for the tree portion of AXObjectcache and AXIsolatedTree.

 }

 String AXTextMarker::debugDescription() const

[Tyler Wilcock]

Created attachment 470342 [details]
Patch

[Tyler Wilcock]

> -    auto tree = std::get<WeakPtr<AXObjectCache>>(axTreeForID(treeID()));
> -    return tree ? tree->objectForID(objectID()) : nullptr;
> +    auto cache = std::get<WeakPtr<AXObjectCache>>(axTreeForID(treeID()));
> +    return cache ? cache->objectForID(objectID()) : nullptr;
> 
> AG: The use of tree here instead of cache was deliberate to remark the fact
> that AXObjectCache is also a tree as far as this code is concerned. In the
> future we should have a common interface for the tree portion of
> AXObjectcache and AXIsolatedTree.
TW: Got it, makes sense, reverted to the old naming in the latest patch!

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #4)
> Created attachment 470342 [details]
> Patch

diff --git a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm b/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
index 0a77761affda..ac0f55f0cda0 100644
--- a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
+++ b/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm
@@ -72,6 +72,7 @@
 #import "RenderTextControl.h"
 #import "RenderView.h"
 #import "RenderWidget.h"
+#import "RuntimeApplicationChecks.h"
 #import "ScrollView.h"
 #import "TextIterator.h"
 #import "VisibleUnits.h"
@@ -759,12 +760,8 @@ ALLOW_DEPRECATED_IMPLEMENTATIONS_END
     return actions;
 }

-- (NSArray*)additionalAccessibilityAttributeNames
+- (NSArray*)_additionalAccessibilityAttributeNames:(const RefPtr<AXCoreObject>&)backingObject
 {
-    RefPtr<AXCoreObject> backingObject = self.axBackingObject;
-    if (!backingObject)
-        return nil;
-
     NSMutableArray *additional = [NSMutableArray array];
     if (backingObject->supportsARIAOwns())
         [additional addObject:NSAccessibilityOwnsAttribute];
@@ -1245,7 +1242,7 @@ ALLOW_DEPRECATED_IMPLEMENTATIONS_END
     else if (backingObject->isVideo())
         objectAttributes = videoAttrs.get().get();

-    NSArray *additionalAttributes = [self additionalAccessibilityAttributeNames];
+    NSArray *additionalAttributes = [self _additionalAccessibilityAttributeNames:backingObject];
     if ([additionalAttributes count])
         objectAttributes = [objectAttributes arrayByAddingObjectsFromArray:additionalAttributes];

@@ -1314,7 +1311,8 @@ - (AXTextMarkerRangeRef)selectedTextMarkerRange

 - (id)associatedPluginParent
 {
-    if (!self.axBackingObject || !self.axBackingObject->hasApplePDFAnnotationAttribute())
+    RefPtr<AXCoreObject> backingObject = self.axBackingObject;
+    if (!backingObject || !backingObject->hasApplePDFAnnotationAttribute())
         return nil;

AG: can we pass the backingObject to this one too?

[Tyler Wilcock]

Created attachment 470345 [details]
Patch

[Tyler Wilcock]

>  - (id)associatedPluginParent
>  {
> -    if (!self.axBackingObject ||
> !self.axBackingObject->hasApplePDFAnnotationAttribute())
> +    RefPtr<AXCoreObject> backingObject = self.axBackingObject;
> +    if (!backingObject || !backingObject->hasApplePDFAnnotationAttribute())
>          return nil;
> 
> AG: can we pass the backingObject to this one too?
TW: Sure can, done in latest patch!

[EWS]

Committed 276073@main (299896402ebb): <https://commits.webkit.org/276073@main>

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