[webkit-reviews] review denied: [Bug 20868] webkit should use AX array centric API for performance : [Attachment 23478] patch to implement AX API

[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 23478: patch to implement AX API
https://bugs.webkit.org/attachment.cgi?id=23478&action=edit

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