Bug 116112 - AX: Use caching when requesting children object on iOS
Summary: AX: Use caching when requesting children object on iOS
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: 2013-05-14 11:34 PDT by chris fleizach
Modified: 2013-05-15 13:51 PDT (History)
7 users (show)

See Also:


Attachments
patch (5.25 KB, patch)
2013-05-14 11:35 PDT, chris fleizach
ddkilzer: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description chris fleizach 2013-05-14 11:34:02 PDT
To speed up performance on iOS we should use the attribute caching that was put in
Comment 1 chris fleizach 2013-05-14 11:35:58 PDT
Created attachment 201736 [details]
patch
Comment 2 David Kilzer (:ddkilzer) 2013-05-14 16:19:16 PDT
Comment on attachment 201736 [details]
patch 

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

r=me, but please consider using the stack-allocated C++ object to start/stop the cache.

> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:307
> +    [self startCachingAttributes];
>      RefPtr<AccessibilityObject> axObject = m_object->accessibilityHitTest(IntPoint(point));
> +    [self stopCachingAttributes];

Can we use the stack-allocated C++ object pattern to turn the caching on and off (rather than the Objective-C start/stop calls)?

Basically you just declare the object on the stack (usually in its own block to isolate the code that uses the cache).  The constructor would call the -startCachingAttributes method and the destructor would call the -stopCachingAttributes method.

This also makes it easier to change the implementation later without changing all the call sites where the cache is used.  And if the cache is slightly different on iOS and Mac, that could be abstracted into this locking object so that (maybe) more code could be shared.
Comment 3 Darin Adler 2013-05-14 17:29:23 PDT
Comment on attachment 201736 [details]
patch 

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

> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:1095
> +    [self startCachingAttributes];
>      
>      // As long as there's a parent wrapper, that's the correct chain to climb.
>      AccessibilityObject* parent = m_object->parentObjectUnignored(); 
>      if (parent)
>          return parent->wrapper();
> +    
> +    [self stopCachingAttributes];

If parent is 0 looks like we’ll call start but not stop. Isn’t that a problem?
Comment 4 Darin Adler 2013-05-14 17:29:42 PDT
Comment on attachment 201736 [details]
patch 

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

>> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:1095
>> +    [self startCachingAttributes];
>>      
>>      // As long as there's a parent wrapper, that's the correct chain to climb.
>>      AccessibilityObject* parent = m_object->parentObjectUnignored(); 
>>      if (parent)
>>          return parent->wrapper();
>> +    
>> +    [self stopCachingAttributes];
> 
> If parent is 0 looks like we’ll call start but not stop. Isn’t that a problem?

If parent is non-zero, I mean.
Comment 5 chris fleizach 2013-05-14 17:37:15 PDT
(In reply to comment #4)
> (From update of attachment 201736 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=201736&action=review
> 
> >> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:1095
> >> +    [self startCachingAttributes];
> >>      
> >>      // As long as there's a parent wrapper, that's the correct chain to climb.
> >>      AccessibilityObject* parent = m_object->parentObjectUnignored(); 
> >>      if (parent)
> >>          return parent->wrapper();
> >> +    
> >> +    [self stopCachingAttributes];
> > 
> > If parent is 0 looks like we’ll call start but not stop. Isn’t that a problem?
> 
> If parent is non-zero, I mean.

Yep good catch. Switching to David's idea of a C++ stack-allocated object will fix this issue. Thanks
Comment 6 chris fleizach 2013-05-15 13:51:08 PDT
http://trac.webkit.org/changeset/150145