Bug 112276 - Allow multiple searchKeys to be passed to AXUIElementCopyParameterizedAttributeValue
Summary: Allow multiple searchKeys to be passed to AXUIElementCopyParameterizedAttribu...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.8
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-13 12:32 PDT by Greg Hughes
Modified: 2013-03-29 12:13 PDT (History)
14 users (show)

See Also:


Attachments
patch (13.49 KB, patch)
2013-03-13 12:33 PDT, Greg Hughes
cfleizach: review-
Details | Formatted Diff | Diff
patch v3 (9.06 KB, patch)
2013-03-13 17:47 PDT, Greg Hughes
cfleizach: review-
Details | Formatted Diff | Diff
patch v4 (6.74 KB, patch)
2013-03-14 12:20 PDT, Greg Hughes
cfleizach: review-
Details | Formatted Diff | Diff
Patch v5, now with layout tests (25.93 KB, patch)
2013-03-18 10:16 PDT, Greg Hughes
cfleizach: review-
Details | Formatted Diff | Diff
Patch v6 (26.92 KB, application/octet-stream)
2013-03-18 11:42 PDT, Greg Hughes
no flags Details
patch v6 (second try) (26.92 KB, patch)
2013-03-18 11:43 PDT, Greg Hughes
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch v7 (26.97 KB, application/octet-stream)
2013-03-27 12:28 PDT, Greg Hughes
no flags Details
Patch v7 (attempt 2) (26.97 KB, patch)
2013-03-27 12:29 PDT, Greg Hughes
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-04 for mac-future (794.73 KB, application/zip)
2013-03-28 10:28 PDT, Build Bot
no flags Details
Patch v8 (25.92 KB, patch)
2013-03-28 14:24 PDT, Greg Hughes
gtk-ews: commit-queue-
Details | Formatted Diff | Diff
Patch v9 (25.91 KB, patch)
2013-03-28 14:45 PDT, Greg Hughes
cfleizach: review-
cfleizach: commit-queue-
Details | Formatted Diff | Diff
patch v10 (30.04 KB, patch)
2013-03-29 10:44 PDT, Greg Hughes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Greg Hughes 2013-03-13 12:32:26 PDT
Accessibility clients need to be able to request multiple different element types when using AXUIElementCopyParameterizedAttributeValue. Currently to get a list of links and buttons clients need to make multiple requests to AXUIElementCopyParameterizedAttributeValue and then compare the positions of returned elements to sort them accordingly. 

This has a negative impact on performance for webKit and clients.

Clients should be allowed to pass an NSArray of searchKeys to AXUIElementCopyParameterizedAttributeValue
Comment 1 Greg Hughes 2013-03-13 12:33:43 PDT
Created attachment 192964 [details]
patch
Comment 2 Greg Hughes 2013-03-13 12:38:36 PDT
<rdar://13412692>
Comment 3 WebKit Review Bot 2013-03-13 12:57:23 PDT
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 4 chris fleizach 2013-03-13 12:57:30 PDT
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 5 Greg Hughes 2013-03-13 13:17:23 PDT
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
Comment 6 Greg Hughes 2013-03-13 17:47:37 PDT
Created attachment 193031 [details]
patch v3
Comment 7 chris fleizach 2013-03-13 19:14:46 PDT
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
Comment 8 Greg Hughes 2013-03-14 12:20:58 PDT
Created attachment 193169 [details]
patch v4
Comment 9 chris fleizach 2013-03-14 12:24:27 PDT
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
Comment 10 Greg Hughes 2013-03-18 10:16:45 PDT
Created attachment 193594 [details]
Patch v5, now with layout tests
Comment 11 Greg Hughes 2013-03-18 10:17:53 PDT
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."
Comment 12 WebKit Review Bot 2013-03-18 10:21:32 PDT
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 13 chris fleizach 2013-03-18 10:32:17 PDT
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
Comment 14 Greg Hughes 2013-03-18 11:38:23 PDT
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);
Comment 15 Greg Hughes 2013-03-18 11:42:55 PDT
Created attachment 193618 [details]
Patch v6
Comment 16 Greg Hughes 2013-03-18 11:43:29 PDT
Created attachment 193619 [details]
patch v6 (second try)
Comment 17 WebKit Review Bot 2013-03-18 14:14:14 PDT
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 18 Build Bot 2013-03-18 15:00:46 PDT
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 19 EFL EWS Bot 2013-03-18 15:15:57 PDT
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 20 Build Bot 2013-03-18 19:11:18 PDT
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 21 Build Bot 2013-03-18 19:31:28 PDT
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
Comment 22 Greg Hughes 2013-03-27 12:28:46 PDT
Created attachment 195377 [details]
Patch v7
Comment 23 Greg Hughes 2013-03-27 12:29:24 PDT
Created attachment 195378 [details]
Patch v7 (attempt 2)
Comment 24 EFL EWS Bot 2013-03-27 12:44:21 PDT
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 25 Build Bot 2013-03-27 17:17:05 PDT
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 26 Build Bot 2013-03-28 10:27:57 PDT
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
Comment 27 Build Bot 2013-03-28 10:28:00 PDT
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
Comment 28 Greg Hughes 2013-03-28 14:24:43 PDT
Created attachment 195642 [details]
Patch v8
Comment 29 kov's GTK+ EWS bot 2013-03-28 14:42:27 PDT
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
Comment 30 Greg Hughes 2013-03-28 14:45:55 PDT
Created attachment 195649 [details]
Patch v9
Comment 31 chris fleizach 2013-03-29 09:32:38 PDT
Comment on attachment 195649 [details]
Patch v9

there are no change logs in this patch
Comment 32 Greg Hughes 2013-03-29 10:44:22 PDT
Created attachment 195762 [details]
patch v10
Comment 33 WebKit Review Bot 2013-03-29 12:12:53 PDT
Comment on attachment 195762 [details]
patch v10

Clearing flags on attachment: 195762

Committed r147236: <http://trac.webkit.org/changeset/147236>
Comment 34 WebKit Review Bot 2013-03-29 12:13:00 PDT
All reviewed patches have been landed.  Closing bug.