116112 2013-05-14 11:34:02 -0700 AX: Use caching when requesting children object on iOS 2013-05-15 13:51:08 -0700 1 1 1 Unclassified WebKit Accessibility 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 cfleizach cfleizach aboxhall apinheiro commit-queue ddkilzer dmazzoni jdiggs mario oldest_to_newest 888954 0 cfleizach 2013-05-14 11:34:02 -0700 To speed up performance on iOS we should use the attribute caching that was put in 888955 1 201736 cfleizach 2013-05-14 11:35:58 -0700 Created attachment 201736 patch 889058 2 201736 ddkilzer 2013-05-14 16:19:16 -0700 Comment on attachment 201736 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. 889075 3 201736 darin 2013-05-14 17:29:23 -0700 Comment on attachment 201736 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? 889076 4 201736 darin 2013-05-14 17:29:42 -0700 Comment on attachment 201736 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. 889078 5 cfleizach 2013-05-14 17:37:15 -0700 (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 889498 6 cfleizach 2013-05-15 13:51:08 -0700 http://trac.webkit.org/changeset/150145 201736 2013-05-14 11:35:58 -0700 2013-05-14 17:29:42 -0700 patch patch text/plain 5378 cfleizach SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE1MDA4MSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDI2IEBACisyMDEzLTA1LTE0ICBDaHJpcyBG bGVpemFjaCAgPGNmbGVpemFjaEBhcHBsZS5jb20+CisKKyAgICAgICAgQVg6IFVzZSBjYWNoaW5n IHdoZW4gcmVxdWVzdGluZyBjaGlsZHJlbiBvYmplY3Qgb24gaU9TCisgICAgICAgIGh0dHBzOi8v YnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMTYxMTIKKworICAgICAgICBSZXZpZXdl ZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBCdWlsZGluZyB1cCB0aGUgY2hpbGRyZW4g bGlzdCBpbiB0aGUgQVggaGllcmFyY2h5IGNhbiBiZSB0aW1lIGNvbnN1bWluZy4gT24gaU9TLCB0 aGlzCisgICAgICAgIGlzIG5vdyBtdWNoIG1vcmUgbm90aWNlYWJsZSAoSSBiZWxpZXZlIGR1ZSB0 byB0aGUgd2F5IHRhYmxlcyBjYWxjdWxhdGUgdGhlaXIgQVggaWdub3JlZCBmbGFnKS4KKworICAg ICAgICBXZSBjYW4gc3BlZWQgZXZlcnl0aGluZyB1cCBpZiB3ZSBqdXN0IGNhY2hlIHRoZSBpc0ln bm9yZWQoKSBhdHRyaWJ1dGUgd2hpbGUgYnVpbGRpbmcgdXAgY2hpbGRyZW4uCisKKyAgICAgICAg KiBhY2Nlc3NpYmlsaXR5L2lvcy9XZWJBY2Nlc3NpYmlsaXR5T2JqZWN0V3JhcHBlcklPUy5tbToK KyAgICAgICAgKC1bV2ViQWNjZXNzaWJpbGl0eU9iamVjdFdyYXBwZXIgYWNjZXNzaWJpbGl0eUhp dFRlc3Q6XSk6CisgICAgICAgICgtW1dlYkFjY2Vzc2liaWxpdHlPYmplY3RXcmFwcGVyIGFjY2Vz c2liaWxpdHlFbGVtZW50Q291bnRdKToKKyAgICAgICAgKC1bV2ViQWNjZXNzaWJpbGl0eU9iamVj dFdyYXBwZXIgYWNjZXNzaWJpbGl0eUVsZW1lbnRBdEluZGV4Ol0pOgorICAgICAgICAoLVtXZWJB Y2Nlc3NpYmlsaXR5T2JqZWN0V3JhcHBlciBpbmRleE9mQWNjZXNzaWJpbGl0eUVsZW1lbnQ6XSk6 CisgICAgICAgICgtW1dlYkFjY2Vzc2liaWxpdHlPYmplY3RXcmFwcGVyIGFjY2Vzc2liaWxpdHlD b250YWluZXJdKToKKyAgICAgICAgKiBhY2Nlc3NpYmlsaXR5L21hYy9XZWJBY2Nlc3NpYmlsaXR5 T2JqZWN0V3JhcHBlckJhc2UuaDoKKyAgICAgICAgKiBhY2Nlc3NpYmlsaXR5L21hYy9XZWJBY2Nl c3NpYmlsaXR5T2JqZWN0V3JhcHBlckJhc2UubW06CisgICAgICAgICgtW1dlYkFjY2Vzc2liaWxp dHlPYmplY3RXcmFwcGVyQmFzZSBzdGFydENhY2hpbmdBdHRyaWJ1dGVzXSk6CisgICAgICAgICgt W1dlYkFjY2Vzc2liaWxpdHlPYmplY3RXcmFwcGVyQmFzZSBzdG9wQ2FjaGluZ0F0dHJpYnV0ZXNd KToKKwogMjAxMy0wNS0xNCAgQW50b2luZSBRdWludCAgPGdyYW91dHNAYXBwbGUuY29tPgogCiAg ICAgICAgIFNuYXBzaG90dGVkIHBsdWctaW5zIHNoYWRvdyB0cmVlIHNob3VsZCByZXNldCBzdHls ZSBpbmhlcml0YW5jZQpJbmRleDogU291cmNlL1dlYkNvcmUvYWNjZXNzaWJpbGl0eS9tYWMvV2Vi QWNjZXNzaWJpbGl0eU9iamVjdFdyYXBwZXJCYXNlLm1tCj09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9X ZWJDb3JlL2FjY2Vzc2liaWxpdHkvbWFjL1dlYkFjY2Vzc2liaWxpdHlPYmplY3RXcmFwcGVyQmFz ZS5tbQkocmV2aXNpb24gMTUwMDgxKQorKysgU291cmNlL1dlYkNvcmUvYWNjZXNzaWJpbGl0eS9t YWMvV2ViQWNjZXNzaWJpbGl0eU9iamVjdFdyYXBwZXJCYXNlLm1tCSh3b3JraW5nIGNvcHkpCkBA IC0zNDIsNiArMzQyLDIyIEBACiAgICAgfQogfQogCistICh2b2lkKXN0YXJ0Q2FjaGluZ0F0dHJp YnV0ZXMKK3sKKyAgICBpZiAoIW1fb2JqZWN0KQorICAgICAgICByZXR1cm47CisgICAgCisgICAg bV9vYmplY3QtPmF4T2JqZWN0Q2FjaGUoKS0+c3RhcnRDYWNoaW5nQ29tcHV0ZWRPYmplY3RBdHRy aWJ1dGVzVW50aWxUcmVlTXV0YXRlcygpOworfQorCistICh2b2lkKXN0b3BDYWNoaW5nQXR0cmli dXRlcworeworICAgIGlmICghbV9vYmplY3QpCisgICAgICAgIHJldHVybjsKKyAgICAKKyAgICBt X29iamVjdC0+YXhPYmplY3RDYWNoZSgpLT5zdG9wQ2FjaGluZ0NvbXB1dGVkT2JqZWN0QXR0cmli dXRlcygpOworfQorCiAvLyBUaGlzIGlzIHNldCBieSBEUlQgd2hlbiBpdCB3YW50cyB0byBsaXN0 ZW4gZm9yIG5vdGlmaWNhdGlvbnMuCiBzdGF0aWMgQk9PTCBhY2Nlc3NpYmlsaXR5U2hvdWxkUmVw b3N0Tm90aWZpY2F0aW9uczsKICsgKHZvaWQpYWNjZXNzaWJpbGl0eVNldFNob3VsZFJlcG9zdE5v dGlmaWNhdGlvbnM6KEJPT0wpcmVwb3N0CkluZGV4OiBTb3VyY2UvV2ViQ29yZS9hY2Nlc3NpYmls aXR5L21hYy9XZWJBY2Nlc3NpYmlsaXR5T2JqZWN0V3JhcHBlckJhc2UuaAo9PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0t LSBTb3VyY2UvV2ViQ29yZS9hY2Nlc3NpYmlsaXR5L21hYy9XZWJBY2Nlc3NpYmlsaXR5T2JqZWN0 V3JhcHBlckJhc2UuaAkocmV2aXNpb24gMTUwMDgxKQorKysgU291cmNlL1dlYkNvcmUvYWNjZXNz aWJpbGl0eS9tYWMvV2ViQWNjZXNzaWJpbGl0eU9iamVjdFdyYXBwZXJCYXNlLmgJKHdvcmtpbmcg Y29weSkKQEAgLTYxLDYgKzYxLDkgQEAKIC0gKENHUGF0aFJlZiljb252ZXJ0UGF0aFRvU2NyZWVu U3BhY2U6KFdlYkNvcmU6OlBhdGggJilwYXRoOwogLSAoQ0dQb2ludCljb252ZXJ0UG9pbnRUb1Nj cmVlblNwYWNlOihXZWJDb3JlOjpGbG9hdFBvaW50ICYpcG9pbnQ7CiAKKy0gKHZvaWQpc3RhcnRD YWNoaW5nQXR0cmlidXRlczsKKy0gKHZvaWQpc3RvcENhY2hpbmdBdHRyaWJ1dGVzOworCiBAZW5k CiAKICNlbmRpZiAvLyBXZWJBY2Nlc3NpYmlsaXR5T2JqZWN0V3JhcHBlckJhc2VfaApJbmRleDog U291cmNlL1dlYkNvcmUvYWNjZXNzaWJpbGl0eS9pb3MvV2ViQWNjZXNzaWJpbGl0eU9iamVjdFdy YXBwZXJJT1MubW0KPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvYWNjZXNzaWJpbGl0eS9p b3MvV2ViQWNjZXNzaWJpbGl0eU9iamVjdFdyYXBwZXJJT1MubW0JKHJldmlzaW9uIDE1MDA4MSkK KysrIFNvdXJjZS9XZWJDb3JlL2FjY2Vzc2liaWxpdHkvaW9zL1dlYkFjY2Vzc2liaWxpdHlPYmpl Y3RXcmFwcGVySU9TLm1tCSh3b3JraW5nIGNvcHkpCkBAIC0zMDIsNyArMzAyLDEwIEBACiAgICAg ICAgIHJldHVybiBuaWw7CiAgICAgCiAgICAgLy8gVHJ5IGEgZnV6enkgaGl0IHRlc3QgZmlyc3Qg dG8gZmluZCBhbiBhY2Nlc3NpYmxlIGVsZW1lbnQuCisgICAgW3NlbGYgc3RhcnRDYWNoaW5nQXR0 cmlidXRlc107CiAgICAgUmVmUHRyPEFjY2Vzc2liaWxpdHlPYmplY3Q+IGF4T2JqZWN0ID0gbV9v YmplY3QtPmFjY2Vzc2liaWxpdHlIaXRUZXN0KEludFBvaW50KHBvaW50KSk7CisgICAgW3NlbGYg c3RvcENhY2hpbmdBdHRyaWJ1dGVzXTsKKwogICAgIGlmICghYXhPYmplY3QpCiAgICAgICAgIHJl dHVybiBuaWw7CiAgICAgCkBAIC0zMjcsNyArMzMwLDExIEBACiAgICAgaWYgKFtzZWxmIGlzQXR0 YWNobWVudF0pCiAgICAgICAgIHJldHVybiBbW3NlbGYgYXR0YWNobWVudFZpZXddIGFjY2Vzc2li aWxpdHlFbGVtZW50Q291bnRdOwogICAgIAotICAgIHJldHVybiBtX29iamVjdC0+Y2hpbGRyZW4o KS5zaXplKCk7CisgICAgW3NlbGYgc3RhcnRDYWNoaW5nQXR0cmlidXRlc107CisgICAgc2l6ZV90 IGNoaWxkcmVuU2l6ZSA9IG1fb2JqZWN0LT5jaGlsZHJlbigpLnNpemUoKTsKKyAgICBbc2VsZiBz dG9wQ2FjaGluZ0F0dHJpYnV0ZXNdOworICAgIAorICAgIHJldHVybiBjaGlsZHJlblNpemU7CiB9 CiAKIC0gKGlkKWFjY2Vzc2liaWxpdHlFbGVtZW50QXRJbmRleDooTlNJbnRlZ2VyKWluZGV4CkBA IC0zMzgsNyArMzQ1LDEwIEBACiAgICAgaWYgKFtzZWxmIGlzQXR0YWNobWVudF0pCiAgICAgICAg IHJldHVybiBbW3NlbGYgYXR0YWNobWVudFZpZXddIGFjY2Vzc2liaWxpdHlFbGVtZW50QXRJbmRl eDppbmRleF07CiAgICAgCisgICAgW3NlbGYgc3RhcnRDYWNoaW5nQXR0cmlidXRlc107CiAgICAg QWNjZXNzaWJpbGl0eU9iamVjdDo6QWNjZXNzaWJpbGl0eUNoaWxkcmVuVmVjdG9yIGNoaWxkcmVu ID0gbV9vYmplY3QtPmNoaWxkcmVuKCk7CisgICAgW3NlbGYgc3RvcENhY2hpbmdBdHRyaWJ1dGVz XTsKKwogICAgIGlmIChzdGF0aWNfY2FzdDx1bnNpZ25lZD4oaW5kZXgpID49IGNoaWxkcmVuLnNp emUoKSkKICAgICAgICAgcmV0dXJuIG5pbDsKICAgICAKQEAgLTM1Nyw3ICszNjcsMTAgQEAKICAg ICBpZiAoW3NlbGYgaXNBdHRhY2htZW50XSkKICAgICAgICAgcmV0dXJuIFtbc2VsZiBhdHRhY2ht ZW50Vmlld10gaW5kZXhPZkFjY2Vzc2liaWxpdHlFbGVtZW50OmVsZW1lbnRdOwogICAgIAorICAg IFtzZWxmIHN0YXJ0Q2FjaGluZ0F0dHJpYnV0ZXNdOwogICAgIEFjY2Vzc2liaWxpdHlPYmplY3Q6 OkFjY2Vzc2liaWxpdHlDaGlsZHJlblZlY3RvciBjaGlsZHJlbiA9IG1fb2JqZWN0LT5jaGlsZHJl bigpOworICAgIFtzZWxmIHN0b3BDYWNoaW5nQXR0cmlidXRlc107CisKICAgICB1bnNpZ25lZCBj b3VudCA9IGNoaWxkcmVuLnNpemUoKTsKICAgICBmb3IgKHVuc2lnbmVkIGsgPSAwOyBrIDwgY291 bnQ7ICsraykgewogICAgICAgICBBY2Nlc3NpYmlsaXR5T2JqZWN0V3JhcHBlciogd3JhcHBlciA9 IGNoaWxkcmVuW2tdLT53cmFwcGVyKCk7CkBAIC0xMDcxLDExICsxMDg0LDE1IEBACiB7CiAgICAg aWYgKCFbc2VsZiBfcHJlcGFyZUFjY2Vzc2liaWxpdHlDYWxsXSkKICAgICAgICAgcmV0dXJuIG5p bDsKKworICAgIFtzZWxmIHN0YXJ0Q2FjaGluZ0F0dHJpYnV0ZXNdOwogICAgIAogICAgIC8vIEFz IGxvbmcgYXMgdGhlcmUncyBhIHBhcmVudCB3cmFwcGVyLCB0aGF0J3MgdGhlIGNvcnJlY3QgY2hh aW4gdG8gY2xpbWIuCiAgICAgQWNjZXNzaWJpbGl0eU9iamVjdCogcGFyZW50ID0gbV9vYmplY3Qt PnBhcmVudE9iamVjdFVuaWdub3JlZCgpOyAKICAgICBpZiAocGFyZW50KQogICAgICAgICByZXR1 cm4gcGFyZW50LT53cmFwcGVyKCk7CisgICAgCisgICAgW3NlbGYgc3RvcENhY2hpbmdBdHRyaWJ1 dGVzXTsKIAogICAgIC8vIFRoZSBvbmx5IG9iamVjdCB3aXRob3V0IGEgcGFyZW50IHdyYXBwZXIg c2hvdWxkIGJlIGEgc2Nyb2xsIHZpZXcuCiAgICAgQVNTRVJUKG1fb2JqZWN0LT5pc0FjY2Vzc2li aWxpdHlTY3JvbGxWaWV3KCkpOwo=