Summary: | Allow multiple searchKeys to be passed to AXUIElementCopyParameterizedAttributeValue | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Greg Hughes <ghughes> | ||||||||||||||||||||||||||
Component: | Accessibility | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
Severity: | Normal | CC: | aboxhall, apinheiro, buildbot, cfleizach, dmazzoni, gtk-ews, jdiggs, mifenton, philn, rniwa, rwlbuis, tonikitoo, webkit.review.bot, xan.lopez | ||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||
Hardware: | Mac | ||||||||||||||||||||||||||||
OS: | OS X 10.8 | ||||||||||||||||||||||||||||
Attachments: |
|
Description
Greg Hughes
2013-03-13 12:32:26 PDT
Created attachment 192964 [details]
patch
Attachment 192964 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/accessibility/AccessibilityObject.cpp', u'Source/WebCore/accessibility/AccessibilityObject.h', u'Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm']" exit_code: 1
Source/WebCore/accessibility/AccessibilityObject.cpp:126: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4]
Source/WebCore/accessibility/AccessibilityObject.cpp:125: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
Total errors found: 2 in 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 192964 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=192964&action=review > Source/WebCore/accessibility/AccessibilityObject.cpp:121 > + unsigned length = criteria->searchKeys->size(); my guess is that size() is a size_t object > Source/WebCore/accessibility/AccessibilityObject.cpp:124 > + switch (criteria->searchKeys->at(i)) { all these changes here are unnecessary and they'll fail the style guidelines > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3187 > + if ([searchKeyEntry isKindOfClass:[NSString self]]) [NSString class] > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3189 > + else if ([searchKeyEntry isKindOfClass:[NSArray self]]) { [NSArray class] > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3190 > + unsigned length = [(NSArray *)searchKeyEntry count]; i believe count is a NSUInteger > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3192 > + for (unsigned i = 0; i < length; ++i) { size length is NSUInteger, so should i > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3194 > + if ([searchKey isKindOfClass:[NSString self]]) [NSString class] Comment on attachment 192964 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=192964&action=review >> Source/WebCore/accessibility/AccessibilityObject.cpp:121 >> + unsigned length = criteria->searchKeys->size(); > > my guess is that size() is a size_t object fixed in next patch >> Source/WebCore/accessibility/AccessibilityObject.cpp:124 >> + switch (criteria->searchKeys->at(i)) { > > all these changes here are unnecessary and they'll fail the style guidelines fixed in next patch (also moved switch statement to is own function to make the loop easier to read) >> Source/WebCore/accessibility/AccessibilityObject.cpp:125 >> + // The AnyTypeSearchKey matches any non-null AccessibilityObject. > > When wrapping a line, only indent 4 spaces. [whitespace/indent] [3] fixed in next patch >> Source/WebCore/accessibility/AccessibilityObject.cpp:126 >> + case AnyTypeSearchKey: > > A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] fixed in next patch >> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3187 >> + if ([searchKeyEntry isKindOfClass:[NSString self]]) > > [NSString class] fixed in next patch >> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3189 >> + else if ([searchKeyEntry isKindOfClass:[NSArray self]]) { > > [NSArray class] fixed in next patch >> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3190 >> + unsigned length = [(NSArray *)searchKeyEntry count]; > > i believe count is a NSUInteger fixed in next patch (downcast to a size_t because reserveInitialCapacity takes a size_t) >> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3192 >> + for (unsigned i = 0; i < length; ++i) { > > size length is NSUInteger, so should i fixed in next patch (downcast to a size_t because reserveInitialCapacity takes a size_t) >> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3194 >> + if ([searchKey isKindOfClass:[NSString self]]) > > [NSString class] fixed in next patch Created attachment 193031 [details]
patch v3
Comment on attachment 193031 [details] patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=193031&action=review you also need a layout test. see LayoutTests/platform/mac/accessibility/search-predicate something or other for an example of using this functionality > Source/WebCore/ChangeLog:5 > + https://bugs.webkit.org/show_bug.cgi?id=112276 no new line between title and link > Source/WebCore/ChangeLog:6 > + add description of what this patch does > Source/WebCore/ChangeLog:7 > + Reviewed by Chris Fleizach don't put my name here, leave it as blank > Source/WebCore/accessibility/AccessibilityObject.cpp:115 > +bool AccessibilityObject::isAccessibilityObjectSearchKeyMatch(AccessibilityObject* axObject, AccessibilitySearchKey searchKey, AccessibilityObject *startObject) * is in wrong place for startObject > Source/WebCore/accessibility/AccessibilityObject.cpp:256 > + if (isAccessibilityObjectSearchKeyMatch(axObject, criteria->searchKeys->at(i), criteria->startObject)) you should pass in the index of the search key criteria instead of breaking up the criteria like this. that will be less churn overall. so i imagine the method would take, axObject, criteria, searchKeyIndex using the at() method is not necessary, you should be able to do searchKeys[i] > Source/WebCore/accessibility/AccessibilityObject.h:315 > + Vector<AccessibilitySearchKey> *searchKeys; i don't think you need to make a pointer out of this. it can just be a Vector<> then you can make a constructor that takes in the other objects and just use the Vector<> that you get out of the criteria > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3190 > + size_t length = (size_t)[(NSArray *)searchKeyEntry count]; i think you'll do less casting if you leave this as a NSUINteger and then just do one static_cast<size_t> in reserveInitialCapacity > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3192 > + for (size_t i = 0; i < length; ++i) { i should be a NUINteger because it's used to access obejctAtIndex: which expects that type Created attachment 193169 [details]
patch v4
Comment on attachment 193169 [details] patch v4 View in context: https://bugs.webkit.org/attachment.cgi?id=193169&action=review overall looks good, but change log isn't correct and you need a layout test > Source/WebCore/ChangeLog:3 > + Allow multiple searchKeys to be passed to AXUIElementCopyParameterizedAttributeValue please use prepare-Changelog --bug=X to make the appropriate change log then check other change log entries Created attachment 193594 [details]
Patch v5, now with layout tests
The style fails for having a variable name in the header, but that entire header has variable names, so instead of changing the entire header I left it as is. ./Tools/Scripts/run-webkit-tests --debug accessibility "All 295 tests ran as expected." Attachment 193594 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/mac/accessibility/search-predicate-expected.txt', u'LayoutTests/platform/mac/accessibility/search-predicate.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/accessibility/AccessibilityObject.cpp', u'Source/WebCore/accessibility/AccessibilityObject.h', u'Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm', u'Tools/ChangeLog', u'Tools/DumpRenderTree/AccessibilityUIElement.cpp', u'Tools/DumpRenderTree/AccessibilityUIElement.h', u'Tools/DumpRenderTree/ios/AccessibilityUIElementIOS.mm', u'Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm', u'Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp', u'Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h', u'Tools/WebKitTestRunner/InjectedBundle/Bindings/AccessibilityUIElement.idl', u'Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm']" exit_code: 1
Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:205: The parameter name "context" adds no information, so it should be removed. [readability/parameter_name] [5]
Tools/DumpRenderTree/AccessibilityUIElement.h:202: The parameter name "context" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 2 in 16 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 193594 [details] Patch v5, now with layout tests View in context: https://bugs.webkit.org/attachment.cgi?id=193594&action=review > Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:945 > + if ( searchKeyParameter == nil ) this should just me if (searchKeyParameter) > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:939 > + JSObjectRef array = const_cast<JSObjectRef>(searchKey); i don't think you want a const_cast. that's for removing const-ness. i think you want a static case. however if there's a toJSObjectRef() method that would be better > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:956 > + if ( searchKeyParameter == nil ) same issue here > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:958 > + [(NSMutableArray *)searchKeyParameter addObject:[NSString stringWithJSStringRef:singleSearchKey]]; i think you should define the searchKeyParameter as the type your using it as in each code block, so you don't have to cast like this > LayoutTests/platform/mac/accessibility/search-predicate.html:266 > + // Heading OR link search Comments should be a full sentence > LayoutTests/platform/mac/accessibility/search-predicate.html:268 > + resultElement = containerElement.uiElementForSearchPredicate(startElement, true, ["AXHeadingLevel2SearchKey","AXLinkSearchKey"], ""); space after the comma between the items in the array > LayoutTests/platform/mac/accessibility/search-predicate.html:272 > + // continue from result element ditto about comment > LayoutTests/platform/mac/accessibility/search-predicate.html:273 > + resultElement = containerElement.uiElementForSearchPredicate(resultElement, true, ["AXHeadingLevel2SearchKey","AXLinkSearchKey"], ""); ditto about comma spacing > LayoutTests/platform/mac/accessibility/search-predicate.html:277 > + // going back should bring us back to the heading ditto about comment > LayoutTests/platform/mac/accessibility/search-predicate.html:278 > + resultElement = containerElement.uiElementForSearchPredicate(resultElement, false, ["AXHeadingLevel2SearchKey","AXLinkSearchKey"], ""); ditto about comma spacing const_cast has to be used and it is what is used in other places in WebKit to do the same thing, i.e. EventSendingController.cpp:86 JSObjectRef array = const_cast<JSObjectRef>(arrayValue); Created attachment 193618 [details]
Patch v6
Created attachment 193619 [details]
patch v6 (second try)
Attachment 193619 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/mac/accessibility/search-predicate-expected.txt', u'LayoutTests/platform/mac/accessibility/search-predicate.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/accessibility/AccessibilityObject.cpp', u'Source/WebCore/accessibility/AccessibilityObject.h', u'Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm', u'Tools/ChangeLog', u'Tools/DumpRenderTree/AccessibilityUIElement.cpp', u'Tools/DumpRenderTree/AccessibilityUIElement.h', u'Tools/DumpRenderTree/ios/AccessibilityUIElementIOS.mm', u'Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm', u'Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp', u'Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h', u'Tools/WebKitTestRunner/InjectedBundle/Bindings/AccessibilityUIElement.idl', u'Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm']" exit_code: 1
Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:205: The parameter name "context" adds no information, so it should be removed. [readability/parameter_name] [5]
Tools/DumpRenderTree/AccessibilityUIElement.h:202: The parameter name "context" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 2 in 16 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 193619 [details] patch v6 (second try) Attachment 193619 [details] did not pass win-ews (win): Output: http://webkit-commit-queue.appspot.com/results/17202351 Comment on attachment 193619 [details] patch v6 (second try) Attachment 193619 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17205571 Comment on attachment 193619 [details] patch v6 (second try) Attachment 193619 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17234324 New failing tests: platform/mac/accessibility/search-predicate.html Comment on attachment 193619 [details] patch v6 (second try) Attachment 193619 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17220304 New failing tests: platform/mac/accessibility/search-predicate.html Created attachment 195377 [details]
Patch v7
Created attachment 195378 [details]
Patch v7 (attempt 2)
Comment on attachment 195378 [details] Patch v7 (attempt 2) Attachment 195378 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17227718 Comment on attachment 195378 [details] Patch v7 (attempt 2) Attachment 195378 [details] did not pass win-ews (win): Output: http://webkit-commit-queue.appspot.com/results/17298356 Comment on attachment 195378 [details] Patch v7 (attempt 2) Attachment 195378 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17331079 New failing tests: media/video-zoom.html platform/mac/accessibility/search-predicate.html Created attachment 195592 [details]
Archive of layout-test-results from webkit-ews-04 for mac-future
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04 Port: mac-future Platform: Mac OS X 10.8.2
Created attachment 195642 [details]
Patch v8
Comment on attachment 195642 [details] Patch v8 Attachment 195642 [details] did not pass gtk-ews (gtk): Output: http://webkit-commit-queue.appspot.com/results/17355092 Created attachment 195649 [details]
Patch v9
Comment on attachment 195649 [details]
Patch v9
there are no change logs in this patch
Created attachment 195762 [details]
patch v10
Comment on attachment 195762 [details] patch v10 Clearing flags on attachment: 195762 Committed r147236: <http://trac.webkit.org/changeset/147236> All reviewed patches have been landed. Closing bug. |