Bug 97301

Created attachment 165068 [details] Patch

Comment on attachment 165068 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165068&action=review it seems like the overall approach could find the elementById and get a Node back, then return the ax object for that one, so that we don't have to iterate all the children to find the object > Tools/DumpRenderTree/chromium/TestRunner/AccessibilityControllerChromium.cpp:106 > + for (unsigned i = 0; i < obj.childCount(); i++) { obj.childCount() should be in a variable so you don't have to call a method each time through the loop > Tools/DumpRenderTree/chromium/TestRunner/AccessibilityControllerChromium.cpp:187 > + result->setNull(); can you set result->setNull() at the top of the method and then just return right away on the error conditions? that will make it a tad cleaner and remove the need for brackets > Tools/DumpRenderTree/mac/AccessibilityControllerMac.mm:43 > +#define END_AX_OBJC_EXCEPTIONS } @catch(NSException *e) { if (![[e name] isEqualToString:NSAccessibilityException]) @throw; } we should find a way to only define this once since it's also defined in AXUIElement > Tools/DumpRenderTree/mac/AccessibilityControllerMac.mm:52 > +{ I believe this method already exists in AccessibilityUIElement... we should probably have it in just one location > Tools/DumpRenderTree/mac/AccessibilityControllerMac.mm:54 > + return NULL; we should return nil for ObjC objects > Tools/DumpRenderTree/mac/AccessibilityControllerMac.mm:103 > + NSArray* children = [obj accessibilityAttributeValue:NSAccessibilityChildrenAttribute]; since this is an ObjC object the * should be next to the "children" http://www.webkit.org/coding/coding-style.html#pointers-non-cpp same goes for NSString* idAttribute > Tools/DumpRenderTree/mac/AccessibilityControllerMac.mm:104 > + for (NSUInteger i = 0; i < [children count]; ++i) { [children count] should be in a variable so we don't have to call the method each time through the loop > Tools/DumpRenderTree/mac/AccessibilityControllerMac.mm:120 > + else no need for else here > Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.h:51 > + PassRefPtr<AccessibilityUIElement> accessibleElementById(JSStringRef attribute); attribute should probably be "id" > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityControllerMac.mm:61 > + return [(NSString *)cfString autorelease]; same comments as DRT > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityControllerMac.mm:108 > + return 0; same comments as DRT

Created attachment 165138 [details] Rebase only, still needs work

Comment on attachment 165138 [details] Rebase only, still needs work Attachment 165138 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13957690

Created attachment 165148 [details] Feedback addressed

Attachment 165148 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/acce..." exit_code: 1 Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityCommonMac.h:43: Extra space before ( in function call [whitespace/parens] [4] Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityCommonMac.h:48: Extra space before ( in function call [whitespace/parens] [4] Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityCommonMac.h:51: This { should be at the end of the previous line [whitespace/braces] [4] Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityCommonMac.h:56: Extra space before [ [whitespace/braces] [5] Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityCommonMac.h:60: This { should be at the end of the previous line [whitespace/braces] [4] Tools/DumpRenderTree/mac/AccessibilityCommonMac.h:43: Extra space before ( in function call [whitespace/parens] [4] Tools/DumpRenderTree/mac/AccessibilityCommonMac.h:48: Extra space before ( in function call [whitespace/parens] [4] Tools/DumpRenderTree/mac/AccessibilityCommonMac.h:51: This { should be at the end of the previous line [whitespace/braces] [4] Tools/DumpRenderTree/mac/AccessibilityCommonMac.h:56: Extra space before [ [whitespace/braces] [5] Tools/DumpRenderTree/mac/AccessibilityCommonMac.h:60: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 10 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 165068 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165068&action=review > it seems like the overall approach could find the elementById and get a Node back, > then return the ax object for that one, so that we don't have to iterate all the > children to find the object I thought about that, but I think this approach is better for tests: * It tests the actual interface for walking the accessibility tree, rather than using a fake shortcut created just for testing * It ensures anything found is actually reachable from the accessibility root * Calling accessibleElementById with a nonexistent id is a handy way to touch every element in the accessibility tree once, which would be sufficient to repro a lot of crash bugs. (A few layout tests used to repro crash bugs walk every element in the tree using JS now, this would be far easier.) * Finally, I don't think that speed is as important for code that's only used for testing; the accessibility layout tests are all pretty fast now and exploring the whole tree doesn't affect their runtime >> Tools/DumpRenderTree/chromium/TestRunner/AccessibilityControllerChromium.cpp:106 >> + for (unsigned i = 0; i < obj.childCount(); i++) { > > obj.childCount() should be in a variable so you don't have to call a method each time through the loop Done >> Tools/DumpRenderTree/chromium/TestRunner/AccessibilityControllerChromium.cpp:187 >> + result->setNull(); > > can you set result->setNull() at the top of the method and then just return right away on the error conditions? > that will make it a tad cleaner and remove the need for brackets Sure, that's cleaner. >> Tools/DumpRenderTree/mac/AccessibilityControllerMac.mm:43 >> +#define END_AX_OBJC_EXCEPTIONS } @catch(NSException *e) { if (![[e name] isEqualToString:NSAccessibilityException]) @throw; } > > we should find a way to only define this once since it's also defined in AXUIElement Moved to a common header file. >> Tools/DumpRenderTree/mac/AccessibilityControllerMac.mm:54 >> + return NULL; > > we should return nil for ObjC objects Done >> Tools/DumpRenderTree/mac/AccessibilityControllerMac.mm:103 >> + NSArray* children = [obj accessibilityAttributeValue:NSAccessibilityChildrenAttribute]; > > since this is an ObjC object the * should be next to the "children" > http://www.webkit.org/coding/coding-style.html#pointers-non-cpp > > same goes for NSString* idAttribute Done >> Tools/DumpRenderTree/mac/AccessibilityControllerMac.mm:104 >> + for (NSUInteger i = 0; i < [children count]; ++i) { > > [children count] should be in a variable so we don't have to call the method each time through the loop Done >> Tools/DumpRenderTree/mac/AccessibilityControllerMac.mm:120 >> + else > > no need for else here Done >> Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.h:51 >> + PassRefPtr<AccessibilityUIElement> accessibleElementById(JSStringRef attribute); > > attribute should probably be "id" Named it idAttribute to not conflict with the obj-C type "id". >> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityControllerMac.mm:61 >> + return [(NSString *)cfString autorelease]; > > same comments as DRT Done

The style checker doesn't seem to realize that AccessibilityCommonMac.h is an objective-C file. Should I name it .hh? Why don't other obj-C header files ending in .h have the same problem? Or is there something else I'm missing?

(In reply to comment #8) > The style checker doesn't seem to realize that AccessibilityCommonMac.h is an objective-C file. > > Should I name it .hh? > > Why don't other obj-C header files ending in .h have the same problem? Or is there something else I'm missing? Maybe a bug in the style checker? ObjC headers are always .h in all code I've ever seen

Comment on attachment 165148 [details] Feedback addressed Attachment 165148 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13951791 New failing tests: accessibility/aria-hidden-with-elements.html

Comment on attachment 165148 [details] Feedback addressed View in context: https://bugs.webkit.org/attachment.cgi?id=165148&action=review >> Tools/DumpRenderTree/mac/AccessibilityCommonMac.h:43 >> +@interface NSString (JSStringRefAdditions) > > Extra space before ( in function call [whitespace/parens] [4] This does seem bogus. Just ignore it — StyleQueue isn't the final say. > Tools/DumpRenderTree/mac/AccessibilityCommonMac.h:64 > +@implementation NSString (JSStringRefAdditions) > + > ++ (NSString *)stringWithJSStringRef:(JSStringRef)jsStringRef > +{ > + if (!jsStringRef) > + return nil; > + > + CFStringRef cfString = JSStringCopyCFString(kCFAllocatorDefault, jsStringRef); > + return [(NSString *)cfString autorelease]; > +} > + > +- (JSStringRef)createJSStringRef > +{ > + return JSStringCreateWithCFString((CFStringRef)self); > +} > + > +@end The @implementation should not be in a .h file. If it was in a .m file it would be fine. I'm surprised this builds. > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityCommonMac.h:64 > +@implementation NSString (JSStringRefAdditions) > + > ++ (NSString *)stringWithJSStringRef:(JSStringRef)jsStringRef > +{ > + if (!jsStringRef) > + return nil; > + > + CFStringRef cfString = JSStringCopyCFString(kCFAllocatorDefault, jsStringRef); > + return [(NSString *)cfString autorelease]; > +} > + > +- (JSStringRef)createJSStringRef > +{ > + return JSStringCreateWithCFString((CFStringRef)self); > +} > + > +@end Ditto.

Created attachment 165205 [details] Patch

Comment on attachment 165148 [details] Feedback addressed View in context: https://bugs.webkit.org/attachment.cgi?id=165148&action=review >>> Tools/DumpRenderTree/mac/AccessibilityCommonMac.h:43 >>> +@interface NSString (JSStringRefAdditions) >> >> Extra space before ( in function call [whitespace/parens] [4] > > This does seem bogus. Just ignore it — StyleQueue isn't the final say. Thanks for taking a look! I'll ignore it, and now that the impl is in a .mm file, there are no other warnings. >> Tools/DumpRenderTree/mac/AccessibilityCommonMac.h:64 >> +@end > > The @implementation should not be in a .h file. If it was in a .m file it would be fine. I'm surprised this builds. Fixed.

Attachment 165205 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/acce..." exit_code: 1 Tools/DumpRenderTree/mac/AccessibilityCommonMac.h:42: Extra space before ( in function call [whitespace/parens] [4] Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityCommonMac.h:42: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 2 in 24 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 165205 [details] Patch looks good

Comment on attachment 165205 [details] Patch Attachment 165205 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13950806 New failing tests: WebFilterOperationsTest.saveAndRestore accessibility/aria-hidden-with-elements.html

Created attachment 165248 [details] Patch for landing

Comment on attachment 165248 [details] Patch for landing Rejecting attachment 165248 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: rce/patched-yasm --revision 154708 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' 51>At revision 154708. ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Total errors found: 0 in 0 files Full output: http://queues.webkit.org/results/13981413

Created attachment 165263 [details] Patch for landing

Created attachment 165265 [details] Patch for landing

Comment on attachment 165265 [details] Patch for landing Clearing flags on attachment: 165265 Committed r129308: <http://trac.webkit.org/changeset/129308>

All reviewed patches have been landed. Closing bug.