Bug 124293

Summary: AX: Calling NSAccessibilityColumnsAttribute and NSAccessibilityRowsAttribute simply to get column/row count can be very expensive.
Product: WebKit Reporter: Samuel White <samuel_white>
Component: AccessibilityAssignee: 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 Flags
Patch.
none
Correct patch.
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2
none
Updated patch.
none
Updated patch for review.
none
Updated patch for review.
cfleizach: review+, eflews.bot: commit-queue-
Patch. none

Description Samuel White 2013-11-13 11:07:46 PST
VoiceOver needs to query for column and row count often when navigating a table. When the table is large, this can be VERY expensive to do over and over. Given the dynamic nature of many tables (like Facebook, Twitter) we need a lightweight way to get the count out easily.

Such as:
NSAccessibilityColumnCountAttribute
NSAccessibilityRowCountAttribute
Comment 1 Radar WebKit Bug Importer 2013-11-13 11:08:05 PST
<rdar://problem/15460808>
Comment 2 Samuel White 2013-11-13 11:09:13 PST
To clarify, the dynamic nature of the tables mentioned above means it's not feasible for AT to just cache these values.
Comment 3 Samuel White 2013-11-13 12:30:12 PST
Created attachment 216840 [details]
Patch.
Comment 4 Samuel White 2013-11-13 12:31:11 PST
Created attachment 216842 [details]
Correct patch.
Comment 5 chris fleizach 2013-11-13 13:22:05 PST
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 6 Build Bot 2013-11-13 14:13:16 PST
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
Comment 7 Build Bot 2013-11-13 14:13:17 PST
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 8 Build Bot 2013-11-13 14:17:38 PST
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
Comment 9 Build Bot 2013-11-13 14:17:41 PST
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 10 Build Bot 2013-11-13 14:24:55 PST
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
Comment 11 Build Bot 2013-11-13 14:24:57 PST
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
Comment 12 Samuel White 2013-11-13 14:34:40 PST
Created attachment 216864 [details]
Updated patch.
Comment 13 Samuel White 2013-11-13 14:35:57 PST
(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 14 chris fleizach 2013-11-13 14:47:19 PST
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
Comment 15 Samuel White 2013-11-13 15:53:21 PST
Created attachment 216877 [details]
Updated patch for review.
Comment 16 Samuel White 2013-11-13 15:54:34 PST
(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 17 chris fleizach 2013-11-13 16:04:16 PST
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
Comment 18 Samuel White 2013-11-13 16:43:01 PST
Created attachment 216878 [details]
Updated patch for review.
Comment 19 EFL EWS Bot 2013-11-13 18:45:23 PST
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
Comment 20 Samuel White 2013-11-13 22:10:31 PST
Since this API addition is specific to the Mac platform I think it's safe to move this test to platform/mac/accessibility.
Comment 21 Samuel White 2013-11-14 09:28:29 PST
Created attachment 216949 [details]
Patch.
Comment 22 WebKit Commit Bot 2013-11-14 10:54:59 PST
Comment on attachment 216949 [details]
Patch.

Clearing flags on attachment: 216949

Committed r159295: <http://trac.webkit.org/changeset/159295>
Comment 23 WebKit Commit Bot 2013-11-14 10:55:03 PST
All reviewed patches have been landed.  Closing bug.