Summary: | webkit should use AX array centric API for performance | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | chris fleizach <cfleizach> | ||||||||
Component: | Accessibility | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | ||||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Mac | ||||||||||
OS: | OS X 10.5 | ||||||||||
Attachments: |
|
Description
chris fleizach
2008-09-15 17:42:48 PDT
Created attachment 23478 [details]
patch to implement AX API
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.
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.
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
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.
Created attachment 23572 [details]
AX Array API updated patch
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
|