Summary: | AX: Calling NSAccessibilityColumnsAttribute and NSAccessibilityRowsAttribute simply to get column/row count can be very expensive. | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Samuel White <samuel_white> | ||||||||||||||||||||
Component: | Accessibility | Assignee: | Samuel White <samuel_white> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | aboxhall, apinheiro, buildbot, cfleizach, commit-queue, dmazzoni, eflews.bot, gyuyoung.kim, jcraig, jdiggs, mario, rniwa, webkit-bug-importer | ||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | Mac | ||||||||||||||||||||||
OS: | OS X 10.9 | ||||||||||||||||||||||
Attachments: |
|
Description
Samuel White
2013-11-13 11:07:46 PST
To clarify, the dynamic nature of the tables mentioned above means it's not feasible for AT to just cache these values. Created attachment 216840 [details]
Patch.
Created attachment 216842 [details]
Correct patch.
Comment on attachment 216842 [details] Correct patch. View in context: https://bugs.webkit.org/attachment.cgi?id=216842&action=review did you run the full ax test suite? i think there a few tests that dump all the attributes, which would start to fail by adding these new attributes > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2412 > + return @(static_cast<AccessibilityTable*>(m_object)->columnCount()); I think there are toAccessibilityTable methods now. also i would stay away from boxing unless you see it being used elsewhere Comment on attachment 216842 [details] Correct patch. Attachment 216842 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/22889098 New failing tests: accessibility/table-detection.html accessibility/table-with-rules.html Created attachment 216855 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 216842 [details] Correct patch. Attachment 216842 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/22909081 New failing tests: accessibility/table-detection.html accessibility/table-with-rules.html Created attachment 216857 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 216842 [details] Correct patch. Attachment 216842 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/22590037 New failing tests: accessibility/table-detection.html accessibility/table-with-rules.html Created attachment 216860 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 216864 [details]
Updated patch.
(In reply to comment #5) > (From update of attachment 216842 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=216842&action=review > > did you run the full ax test suite? i think there a few tests that dump all the attributes, which would start to fail by adding these new attributes Fixed. > > > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2412 > > + return @(static_cast<AccessibilityTable*>(m_object)->columnCount()); > > I think there are toAccessibilityTable methods now. Ah. Updated. > also i would stay away from boxing unless you see it being used elsewhere Auto boxing can be found in a few places in WebKit and WebCore. Thanks. Comment on attachment 216864 [details] Updated patch. View in context: https://bugs.webkit.org/attachment.cgi?id=216864&action=review > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:111 > +#define NSAccessibilityColumnCountAttribute @"AXColumnCount" these already exist so no need to define again Created attachment 216877 [details]
Updated patch for review.
(In reply to comment #14) > (From update of attachment 216864 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=216864&action=review > > > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:111 > > +#define NSAccessibilityColumnCountAttribute @"AXColumnCount" > > these already exist so no need to define again Indeed. Sadly, I invented these before I checked. Then forgot to pull them out on this end once I found out they already existed. Doh. Thanks. Comment on attachment 216877 [details] Updated patch for review. View in context: https://bugs.webkit.org/attachment.cgi?id=216877&action=review > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2403 > + if ([attributeName isEqualToString:NSAccessibilityColumnCountAttribute]) { single line if statements don't need brackets > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2407 > + if ([attributeName isEqualToString:NSAccessibilityRowCountAttribute]) { ditto Created attachment 216878 [details]
Updated patch for review.
Comment on attachment 216878 [details] Updated patch for review. Attachment 216878 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/22909125 Since this API addition is specific to the Mac platform I think it's safe to move this test to platform/mac/accessibility. Created attachment 216949 [details]
Patch.
Comment on attachment 216949 [details] Patch. Clearing flags on attachment: 216949 Committed r159295: <http://trac.webkit.org/changeset/159295> All reviewed patches have been landed. Closing bug. |