[webkit-reviews] review denied: [Bug 20868] webkit should use AX array centric API for performance : [Attachment 23489] updated patch based on review

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Sep 18 15:29:26 PDT 2008

Darin Adler <darin at apple.com> has denied chris fleizach <cfleizach at apple.com>'s
request for review:
Bug 20868: webkit should use AX array centric API for performance

Attachment 23489: updated patch based on review

------- Additional Comments from Darin Adler <darin at apple.com>
+// 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 =

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

+    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
+	 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

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

More information about the webkit-reviews mailing list