Summary: | AX: Add AXUIElementCountForSearchPredicate parameterized attribute. | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Samuel White <samuel_white> | ||||||||||||||||||||||
Component: | Accessibility | Assignee: | Samuel White <samuel_white> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | aboxhall, apinheiro, buildbot, cfleizach, commit-queue, dmazzoni, eflews.bot, gtk-ews, gyuyoung.kim, jcraig, jdiggs, mario, rniwa, webkit-bug-importer, xan.lopez | ||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
Hardware: | Mac (Intel) | ||||||||||||||||||||||||
OS: | OS X 10.9 | ||||||||||||||||||||||||
Attachments: |
|
Description
Samuel White
2013-11-18 22:01:45 PST
Created attachment 217457 [details]
Patch for EWS.
No layout test added yet. Just looking for non Mac platform failures via EWS.
Comment on attachment 217457 [details] Patch for EWS. Attachment 217457 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/30148019 Comment on attachment 217457 [details] Patch for EWS. Attachment 217457 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/29798026 Created attachment 217461 [details]
Updated patch for EWS.
Should fix non Mac platform issues. Patch not for review. Still needs layout test.
Comment on attachment 217461 [details] Updated patch for EWS. Attachment 217461 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/30098033 Comment on attachment 217461 [details] Updated patch for EWS. Attachment 217461 [details] did not pass gtk-ews (gtk): Output: http://webkit-queues.appspot.com/results/30798002 Comment on attachment 217461 [details] Updated patch for EWS. Attachment 217461 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/30528004 Created attachment 217465 [details]
Last patch for EWS (I hope).
Missed some DRT fixes. THIS should do it.
Comment on attachment 217465 [details] Last patch for EWS (I hope). Attachment 217465 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/29938032 New failing tests: platform/mac/accessibility/search-predicate-element-count.html Created attachment 217472 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 217465 [details] Last patch for EWS (I hope). Attachment 217465 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/30058104 New failing tests: platform/mac/accessibility/search-predicate-element-count.html Created attachment 217485 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 217465 [details] Last patch for EWS (I hope). View in context: https://bugs.webkit.org/attachment.cgi?id=217465&action=review > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:550 > + id searchKeyParameter = [parameterizedAttribute objectForKey:@"AXSearchKey"]; you should start these objects out as the type that they're expected, so we don't have to cast (to CFStringRef), and won't run into any other weird namespace collisions > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:580 > + id searchKey = [searchKeyParameter objectAtIndex:i]; ditto for type > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:582 > + criteria.searchKeys.uncheckedAppend(accessibilitySearchKeyForString((CFStringRef)searchKey)); probably CFStringRef not needed Created attachment 217504 [details]
Patch for feedback.
Added layout test to complete patch. Making sure it passes and will update Changelogs.
Comment on attachment 217504 [details] Patch for feedback. Attachment 217504 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/30578040 New failing tests: platform/mac/accessibility/search-predicate-element-count.html Created attachment 217512 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 217514 [details]
Patch for review.
(In reply to comment #14) > (From update of attachment 217465 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=217465&action=review > > > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:550 > > + id searchKeyParameter = [parameterizedAttribute objectForKey:@"AXSearchKey"]; > > you should start these objects out as the type that they're expected, so we don't have to cast (to CFStringRef), and won't run into any other weird namespace collisions The object referenced by each pointer is validated with isKindOfClass: before being messaged so collisions aren't an issue for us here. However, your suggestion helps with readability so I've included it. > > > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:580 > > + id searchKey = [searchKeyParameter objectAtIndex:i]; > > ditto for type > > > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:582 > > + criteria.searchKeys.uncheckedAppend(accessibilitySearchKeyForString((CFStringRef)searchKey)); > > probably CFStringRef not needed I thought these casts made the somewhat tricky CFStringRef/String backing clearer. Since I've followed your above advice, this became redundant. Removed. Thanks. (In reply to comment #19) > (In reply to comment #14) > > (From update of attachment 217465 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=217465&action=review > > > > > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:550 > > > + id searchKeyParameter = [parameterizedAttribute objectForKey:@"AXSearchKey"]; > > > > you should start these objects out as the type that they're expected, so we don't have to cast (to CFStringRef), and won't run into any other weird namespace collisions > > The object referenced by each pointer is validated with isKindOfClass: before being messaged so collisions aren't an issue for us here. However, your suggestion helps with readability so I've included it. > > > > > > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:580 > > > + id searchKey = [searchKeyParameter objectAtIndex:i]; > > > > ditto for type > > > > > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:582 > > > + criteria.searchKeys.uncheckedAppend(accessibilitySearchKeyForString((CFStringRef)searchKey)); > > > > probably CFStringRef not needed > > I thought these casts made the somewhat tricky CFStringRef/String backing clearer. Since I've followed your above advice, this became redundant. Removed. Oops, to clarify, I meant this cast: searchText = (CFStringRef)searchTextParameter; As the relationship between String type and CFStringRef type is non obvious (imo). the others were legacy. > > Thanks. Any additional feedback on this one Chris? Comment on attachment 217514 [details] Patch for review. View in context: https://bugs.webkit.org/attachment.cgi?id=217514&action=review > Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:200 > + NSMutableDictionary* parameterizedAttribute = [NSMutableDictionary dictionary]; this * should be on the other side since it's an ObjC object > Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:213 > + if (searchKeyString) { braces not necessary since it's a single line > Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:1017 > + NSDictionary* parameterizedAttribute = searchPredicateParameterizedAttributeForSearchCriteria(context, startElement, isDirectionNext, UINT_MAX, searchKey, searchText, visibleOnly); * on wrong side > Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:1029 > + NSDictionary* parameterizedAttribute = searchPredicateParameterizedAttributeForSearchCriteria(context, startElement, isDirectionNext, 1, searchKey, searchText, visibleOnly); * on wrong side > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:215 > + NSMutableDictionary* parameterizedAttribute = [NSMutableDictionary dictionary]; * on wrong side > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:228 > + if (searchKeyString) { braces not necessary > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:1046 > + NSDictionary* parameterizedAttribute = searchPredicateParameterizedAttributeForSearchCriteria(context, startElement, isDirectionNext, UINT_MAX, searchKey, searchText, visibleOnly); * on wrong side > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:1058 > + NSDictionary* parameterizedAttribute = searchPredicateParameterizedAttributeForSearchCriteria(context, startElement, isDirectionNext, 1, searchKey, searchText, visibleOnly); * on wrong side Created attachment 218189 [details]
Updated patch.
(In reply to comment #22) > (From update of attachment 217514 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=217514&action=review > > > Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:200 > > + NSMutableDictionary* parameterizedAttribute = [NSMutableDictionary dictionary]; > > this * should be on the other side since it's an ObjC object > > > Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:213 > > + if (searchKeyString) { > > braces not necessary since it's a single line Fixed. > > > Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:1017 > > + NSDictionary* parameterizedAttribute = searchPredicateParameterizedAttributeForSearchCriteria(context, startElement, isDirectionNext, UINT_MAX, searchKey, searchText, visibleOnly); > > * on wrong side > > > Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:1029 > > + NSDictionary* parameterizedAttribute = searchPredicateParameterizedAttributeForSearchCriteria(context, startElement, isDirectionNext, 1, searchKey, searchText, visibleOnly); > > * on wrong side > > > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:215 > > + NSMutableDictionary* parameterizedAttribute = [NSMutableDictionary dictionary]; > > * on wrong side > > > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:228 > > + if (searchKeyString) { > > braces not necessary Fixed. > > > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:1046 > > + NSDictionary* parameterizedAttribute = searchPredicateParameterizedAttributeForSearchCriteria(context, startElement, isDirectionNext, UINT_MAX, searchKey, searchText, visibleOnly); > > * on wrong side > > > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:1058 > > + NSDictionary* parameterizedAttribute = searchPredicateParameterizedAttributeForSearchCriteria(context, startElement, isDirectionNext, 1, searchKey, searchText, visibleOnly); > > * on wrong side Why are we considering "NSString* str" wrong and "NSString *str" correct? If I look at the files you mention, the vast majority of our obj-c code is of the style "NSString* str". Is this something you hope to change moving forward? If so, I can adopt this style per your request. Thanks. (In reply to comment #24) > (In reply to comment #22) > > (From update of attachment 217514 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=217514&action=review > > > > > Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:200 > > > + NSMutableDictionary* parameterizedAttribute = [NSMutableDictionary dictionary]; > > > > this * should be on the other side since it's an ObjC object > > > > > Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:213 > > > + if (searchKeyString) { > > > > braces not necessary since it's a single line > > Fixed. > > > > > > Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:1017 > > > + NSDictionary* parameterizedAttribute = searchPredicateParameterizedAttributeForSearchCriteria(context, startElement, isDirectionNext, UINT_MAX, searchKey, searchText, visibleOnly); > > > > * on wrong side > > > > > Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:1029 > > > + NSDictionary* parameterizedAttribute = searchPredicateParameterizedAttributeForSearchCriteria(context, startElement, isDirectionNext, 1, searchKey, searchText, visibleOnly); > > > > * on wrong side > > > > > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:215 > > > + NSMutableDictionary* parameterizedAttribute = [NSMutableDictionary dictionary]; > > > > * on wrong side > > > > > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:228 > > > + if (searchKeyString) { > > > > braces not necessary > > Fixed. > > > > > > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:1046 > > > + NSDictionary* parameterizedAttribute = searchPredicateParameterizedAttributeForSearchCriteria(context, startElement, isDirectionNext, UINT_MAX, searchKey, searchText, visibleOnly); > > > > * on wrong side > > > > > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:1058 > > > + NSDictionary* parameterizedAttribute = searchPredicateParameterizedAttributeForSearchCriteria(context, startElement, isDirectionNext, 1, searchKey, searchText, visibleOnly); > > > > * on wrong side > > Why are we considering "NSString* str" wrong and "NSString *str" correct? If I look at the files you mention, the vast majority of our obj-c code is of the style "NSString* str". Is this something you hope to change moving forward? If so, I can adopt this style per your request. Thanks. For objectiveC objects it should be like "NSString *a" -- a lot of code doesn't conform to this standard, but i guess for code we touch we should slowly start to move them into correct form --------------- Under Pointer & Referneces http://www.webkit.org/coding/coding-style.html Pointer types in non-C++ code — Pointer types should be written with a space between the type and the * (so the * is adjacent to the following identifier if any). [pointers-non-cpp] --------------------- Created attachment 218198 [details]
Final patch.
Moved * in every file being compiled as obj-c++ to meet spec. Chris mentioned.
Should be ready to go. Thanks.
Comment on attachment 218198 [details]
Final patch.
Will commit manually if all tests pass.
Committed in http://trac.webkit.org/changeset/159980 Closing. platform/mac/accessibility/search-predicate-element-count.html is failing on all OS X versions. I'm going to roll out soon, unless someone speaks up, because this affects EWS reliability. (In reply to comment #29) > platform/mac/accessibility/search-predicate-element-count.html is failing on all OS X versions. I'm going to roll out soon, unless someone speaks up, because this affects EWS reliability. The test only fails when run in a group. I suggest we mark it as flaky until I can get to the bottom o fit. OK, marked as flaky, and filed bug 125195 to track this. (In reply to comment #31) > OK, marked as flaky, and filed bug 125195 to track this. Great, thank you. |