Bug 20868 – webkit should use AX array centric API for performance

• Component: Accessibility
• Status: RESOLVED FIXED

[chris fleizach]

webkit should use the following on the mac

- (NSUInteger)accessibilityIndexOfChild:(id)child AVAILABLE_MAC_OS_X_VERSION_10_2_AND_LATER;
- (NSUInteger)accessibilityArrayAttributeCount:(NSString *)attribute AVAILABLE_MAC_OS_X_VERSION_10_2_AND_LATER;
- (NSArray *)accessibilityArrayAttributeValues:(NSString *)attribute index:(NSUInteger)index maxCount:(NSUInteger)maxCount AVAILABLE_MAC_OS_X_VERSION_10_2_AND_LATER;

[chris fleizach]

Created attachment 23478 [details]
patch to implement AX API

[Darin Adler]

Comment on attachment 23478 [details]
patch to implement AX API

 7         Implemenent a few AX API methods that will be called by AppKit, which will 

Typo in "Implement".

 103 @interface NSObject (WebKitAccessibilityArrayCategory)

This seems strange. Why is this a category on an arbitrary NSObject? If this really is part of a contract with AppKit I'd like to see it in a header. Will we always want to declare it this way, or will we have a more elegant way to do it long term.

 1934     unsigned k = 0, count = children.size();

We don't do more than one of these at the same time.

 1949         NSArray* widgetChildren = [self renderWidgetChildren];
 1950         if (widgetChildren)
 1951             return [widgetChildren count];
 1952         return 0;

There's no need for a nil check here since Objective-C already gives 0 if you message to nil.

 1966             if (childCount > ([children count]-index))
 1967             {
 1968                 childCount = ([children count]-index);
 1969             }

We don't do parentheses around if statement bodies like this; and there are missing spaces around the "-" operator and the parentheses are unneeded around the "-" expression. I think this would read better if you used the min() function and it would be more efficient too since it wouldn't call the function twice.

What guarantees that index is not > [children count]?

 1974         unsigned added = 0, k = index, count = children.size(), available = MIN(count - index, maxCount);

It'd be better to use the min() function from C++ rather than the MIN macro.

I think I'll say review- because of the comments above, but this seems close.

[Eric Seidel (no email)]

Comment on attachment 23478 [details]
patch to implement AX API

There are some style violations:

This is Obj-C code, so the * goes on the right:
+            NSArray* children = [self renderWidgetChildren];
+        NSArray* widgetChildren = [self renderWidgetChildren];

No { }:
+            if (childCount > ([children count]-index))
+            {
+                childCount = ([children count]-index);
+            }

I believe it's part of our style guide that each variable gets its own line:
+        unsigned added = 0, k = index, count = children.size(), available = MIN(count - index, maxCount);  but I guess I'd have to check.

You don't need to check nil here:
+        NSArray* widgetChildren = [self renderWidgetChildren];
+        if (widgetChildren)
+            return [widgetChildren count];
+        return 0;

return [widgetChildren count] will do the same.

Otherwise looks fine.  I assume you can fix the style on landing.

[chris fleizach]

Created attachment 23489 [details]
updated patch based on review

eric said +, darin said -, so requesting review again

darin asked if we need the category on NSObject. Since those methods are only exposed in SnowLeopard, the category needs to be there in leopard and tiger. i made that explicit with a #define

[Darin Adler]

Comment on attachment 23489 [details]
updated patch based on review

+// Accessibility API appearing in SnowLeopard

It's best not to have a check-in to the WebKit project explicitly announce minor changes that will be happening in Snow Leopard. As someone working at Apple contributing to WebKit you should try to minimize this. You should leave that sort of comment out in the future.

The methods in the category are only needed so we can make method calls. There's no need for a method to be in the category if WebKit is just going to implement it. So I'm not sure all three methods are needed.

If these methods won't even be called on Tiger or Leopard, then maybe the code should only be compiled when it's neither Tiger nor Leopard?

+- (NSUInteger)accessibilityIndexOfChild:(id)child

Why doesn't this method call super at the end instead of returning NSNotFound? Why doesn't this method handle the renderWidgetChildren case?

+    AccessibilityObject::AccessibilityChildrenVector children = m_object->children();

I don't think we want to copy the vector here; that's not good for performance. The local variable should be a const AccessibilityObject::AccessibilityChildrenVector&. This occurs in multiple places.

+    unsigned k = 0;
+    unsigned count = children.size();
+    for (; k < count; ++k) {

It'd be a more normal idiom to define k inside the for statement.

+            NSUInteger arrayLength = std::min(childCount - index, maxCount);

This is OK, but you can also say just "min" as long as there's a "using namespace std" at the top of the file, and that's slightly preferred over this.

+        unsigned added = 0;
+        
+        NSMutableArray *subarray = [NSMutableArray arrayWithCapacity:available];
+        for (; added < available; ++index, ++added)
+            [subarray addObject:children[index]->wrapper()];

As above, it's a more normal idiom to put define added inside the for statement.

I'm going to say review- because of the combination of the vector copies and the questions about accessibilityIndexOfChild.

[chris fleizach]

Created attachment 23572 [details]
AX Array API updated patch

[Darin Adler]

Comment on attachment 23572 [details]
AX Array API updated patch

 1947     return [super accessibilityIndexOfChild:child];

I think I was wrong to suggest you call super here. I think this should just return not found for consistency with the other functions.

r=me

[chris fleizach]

http://trac.webkit.org/changeset/36669