Bug 145991 – AX: iOS accessibility tests are not running because we need WKTR support

• Component: Accessibility
• Status: RESOLVED FIXED

[chris fleizach]

No ax tests are running in the simulator because they were disabled and support not added

<rdar://problem/21390968>

[chris fleizach]

Created attachment 254931 [details]
patch

[WebKit Commit Bot]

Attachment 254931 [details] did not pass style-queue:


ERROR: Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:38:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:1003:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:1007:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:1011:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:1015:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:1019:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 7 in 70 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Created attachment 254936 [details]
patch

[Daniel Bates]

Comment on attachment 254936 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254936&action=review

Looks sane to me.

> Tools/ChangeLog:11
> +          rename to more generic names.

Minor: Please remove the indentation on this line such that the 'r' in rename left aligns with the 'R' in rename of the previous line.

> Tools/DumpRenderTree/ios/AccessibilityUIElementIOS.mm:715
> +    // Mac programmers should not be adding more than one notification listener per element.

Did you mean to write "Mac programmers" instead of "iOS programmers" or "Mac and iOS programmers"? I mean this comment is in an iOS-specific file. I take it you wrote the comment using "Mac programmers" because iOS makes use of the Mac accessibility code.

> Tools/DumpRenderTree/ios/AccessibilityUIElementIOS.mm:729
> +    // Mac programmers should not be trying to remove a listener that's already removed.

Ditto.

> Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:142
> +    return 0;

0 => nullptr

> Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:151
> +    return 0;

Ditto.

> Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:195
> +    JSValueRef arrayResult = JSObjectMakeArray(context, 0, 0, 0);

We should pass nullptr for the last two arguments instead of 0.

> Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:196
> +    JSObjectRef arrayObj = JSValueToObject(context, arrayResult, 0);

We should pass nullptr instead of 0 to indicate a null pointer as the last argument in JSValueToObject().

> Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:199
> +        JSObjectSetPropertyAtIndex(context, arrayObj, i, JSObjectMake(context, elements[i]->wrapperClass(), elements[i].get()), 0);

Ditto.

> Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:508
> +    bool result = (traits & [m_element _axSelectedTrait]) == [m_element _axSelectedTrait];

I suggest we inline the value of this variable into the return statement below as this variable is used exactly once in this function and I do not find that it's name helps improve the readability of this code.

> Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:584
> +        return 0;

0 => nullptr

> Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:774
> +        return 0;

0 => nullptr

> Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:802
> +    // Mac programmers should not be adding more than one notification listener per element.

See my remark about "Mac programmers" above.

[chris fleizach]

(In reply to comment #4)

Thanks. Changes made

> Comment on attachment 254936 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=254936&action=review
> 
> Looks sane to me.
> 
> > Tools/ChangeLog:11
> > +          rename to more generic names.
> 
> Minor: Please remove the indentation on this line such that the 'r' in
> rename left aligns with the 'R' in rename of the previous line.
> 
> > Tools/DumpRenderTree/ios/AccessibilityUIElementIOS.mm:715
> > +    // Mac programmers should not be adding more than one notification listener per element.
> 
> Did you mean to write "Mac programmers" instead of "iOS programmers" or "Mac
> and iOS programmers"? I mean this comment is in an iOS-specific file. I take
> it you wrote the comment using "Mac programmers" because iOS makes use of
> the Mac accessibility code.
> 
> > Tools/DumpRenderTree/ios/AccessibilityUIElementIOS.mm:729
> > +    // Mac programmers should not be trying to remove a listener that's already removed.
> 
> Ditto.
> 
> > Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:142
> > +    return 0;
> 
> 0 => nullptr
> 
> > Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:151
> > +    return 0;
> 
> Ditto.
> 
> > Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:195
> > +    JSValueRef arrayResult = JSObjectMakeArray(context, 0, 0, 0);
> 
> We should pass nullptr for the last two arguments instead of 0.
> 
> > Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:196
> > +    JSObjectRef arrayObj = JSValueToObject(context, arrayResult, 0);
> 
> We should pass nullptr instead of 0 to indicate a null pointer as the last
> argument in JSValueToObject().
> 
> > Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:199
> > +        JSObjectSetPropertyAtIndex(context, arrayObj, i, JSObjectMake(context, elements[i]->wrapperClass(), elements[i].get()), 0);
> 
> Ditto.
> 
> > Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:508
> > +    bool result = (traits & [m_element _axSelectedTrait]) == [m_element _axSelectedTrait];
> 
> I suggest we inline the value of this variable into the return statement
> below as this variable is used exactly once in this function and I do not
> find that it's name helps improve the readability of this code.
> 
> > Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:584
> > +        return 0;
> 
> 0 => nullptr
> 
> > Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:774
> > +        return 0;
> 
> 0 => nullptr
> 
> > Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:802
> > +    // Mac programmers should not be adding more than one notification listener per element.
> 
> See my remark about "Mac programmers" above.

[chris fleizach]

http://trac.webkit.org/changeset/185609