| Summary: | AX: Use caching when requesting children object on iOS | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | chris fleizach <cfleizach> | ||||
| Component: | Accessibility | Assignee: | chris fleizach <cfleizach> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | aboxhall, apinheiro, commit-queue, ddkilzer, dmazzoni, jdiggs, mario | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
chris fleizach
2013-05-14 11:34:02 PDT
Created attachment 201736 [details]
patch
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 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 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. (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 |