[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]

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
https://bugs.webkit.org/show_bug.cgi?id=20868

Attachment 23489: updated patch based on review
https://bugs.webkit.org/attachment.cgi?id=23489&action=edit

------- 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 =
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.