Bug 42131 – WebKitTestRunner needs to support accessibility-related DRT APIs

• Component: WebKit2
• Status: RESOLVED FIXED

[Maciej Stachowiak]

WebKitTestRunner needs to support accessibility-related DRT APIs

[Maciej Stachowiak]

<rdar://problem/8183711>

[chris fleizach]

Created attachment 113192 [details]
patch

[chris fleizach]

Created attachment 113195 [details]
patch

[WebKit Review Bot]

Attachment 113195 [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/AccessibilityUIElement.cpp:27:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
Total errors found: 1 in 46 files


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

[Gustavo Noronha (kov)]

Comment on attachment 113195 [details]
patch

Attachment 113195 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10148012

[Darin Adler]

Comment on attachment 113195 [details]
patch

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

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapper.mm:-499
> -    ASSERT(!value.isEmpty());
> -    

You can’t just remove this assert. The call to get below won’t work right with certain values (either null strings or empty strings, I can’t recall which) because of how hash tables work. If you need to allow these strings, then you need to add an explicit early return, not fall through to the hash table code.

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePagePrivate.h:71
> +WK_EXPORT void* WKAccessibilityRootObject(WKBundlePageRef);
> +WK_EXPORT void* WKAccessibilityFocusedObject(WKBundlePageRef);    

Does this really need to be void*?

[chris fleizach]

(In reply to comment #6)
> (From update of attachment 113195 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113195&action=review
> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapper.mm:-499
> > -    ASSERT(!value.isEmpty());
> > -    
> 


> You can’t just remove this assert. The call to get below won’t work right with certain values (either null strings or empty strings, I can’t recall which) because of how hash tables work. If you need to allow these strings, then you need to add an explicit early return, not fall through to the hash table code.
> 

Will add an early return. This method needs to handle any kind of string that it gets and if it doesn't match, return the "AnyType" of element search criteria

> > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePagePrivate.h:71
> > +WK_EXPORT void* WKAccessibilityRootObject(WKBundlePageRef);
> > +WK_EXPORT void* WKAccessibilityFocusedObject(WKBundlePageRef);    
> 
> Does this really need to be void*?

They return the "id" type naturally. Should I make a new opaque type to wrap that?

[chris fleizach]

Created attachment 113348 [details]
patch

[chris fleizach]

Created attachment 113414 [details]
patch

[chris fleizach]

Created attachment 113497 [details]
patch

[chris fleizach]

Created attachment 113509 [details]
patch

[chris fleizach]

Created attachment 113518 [details]
patch

[chris fleizach]

Created attachment 113524 [details]
patch

[Mario Sanchez Prada]

Comment on attachment 113524 [details]
patch

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

Informal review, hope you find it helpful. See my comments below

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:146
> +    WebCore::AXObjectCache::enableAccessibility();

It probably doesn't matter much, but shouldn't we use the typical "if (!AXObjectCache::accessibilityEnabled())" check before calling to enableAccessibility(), for consistency with other places in the code? (e.g. see accessibilityRoot() in WebKit/mac/WebView/WebFrame.mm, or accessibilityAttributeValue() in WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObject.mm)

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:164
> +    WebCore::AXObjectCache::enableAccessibility();

Same here.

> Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp:53
> +    return m_element;            

I think it could be better to do something like return m_element ? true : false; here so we always return something of bool type, since m_element is a PlatformUIElement

> Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp:56
> +// Unsupported methods on various platforms. As their implemented on other platforms this list should be modified.

"their" -> "they're" ?

> Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.h:47
> +    // Controller Methods - platform-independent implementations

Missing '.' at the end of the comment. There are some more of these in the patch (e.g. AccessibilityUIElement.h), so won't comment on the rest (don't want to flood with this trivial things).

[chris fleizach]

(In reply to comment #14)
> (From update of attachment 113524 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113524&action=review
> 
> Informal review, hope you find it helpful. See my comments below
> 
> > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:146
> > +    WebCore::AXObjectCache::enableAccessibility();
> 
> It probably doesn't matter much, but shouldn't we use the typical "if (!AXObjectCache::accessibilityEnabled())" check before calling to enableAccessibility(), for consistency with other places in the code? (e.g. see accessibilityRoot() in WebKit/mac/WebView/WebFrame.mm, or accessibilityAttributeValue() in WebKit2/WebProcess/WebPage/mac/WKAccessibilityWebPageObject.mm)
> 

Yea those are there unnecessarily. They did not come from this patch, so I left them alone

> > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.cpp:164
> > +    WebCore::AXObjectCache::enableAccessibility();
> 
> Same here.

This one is likely needed because the entry point to accessibility from DRT through WK2 will be through this method. If ax is not on, then the ax object cache will not be created and nothing will work


> 
> > Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp:53
> > +    return m_element;            
> 
> I think it could be better to do something like return m_element ? true : false; here so we always return something of bool type, since m_element is a PlatformUIElement
> 

PlatformUIElement is not a ref pointer, so i think this is preferred style.

> > Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp:56
> > +// Unsupported methods on various platforms. As their implemented on other platforms this list should be modified.
> 
> "their" -> "they're" ?
> 

thanks

> > Tools/WebKitTestRunner/InjectedBundle/AccessibilityController.h:47
> > +    // Controller Methods - platform-independent implementations
> 
> Missing '.' at the end of the comment. There are some more of these in the patch (e.g. AccessibilityUIElement.h), so won't comment on the rest (don't want to flood with this trivial things).

Thanks

[chris fleizach]

> > 
> > > Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp:53
> > > +    return m_element;            
> > 
> > I think it could be better to do something like return m_element ? true : false; here so we always return something of bool type, since m_element is a PlatformUIElement
> > 
> 
> PlatformUIElement is not a ref pointer, so i think this is preferred style.
> 

I don't see anything in style guide about this, so I don't know what's preferred. However I see this in RenderObject.h

virtual bool canHaveChildren() const { return virtualChildren(); }

where virtualChildren is a pointer

[Mario Sanchez Prada]

(In reply to comment #16)
> [...]
> I don't see anything in style guide about this, so I don't know what's preferred. However I see this in RenderObject.h
> 
> virtual bool canHaveChildren() const { return virtualChildren(); }
> 
> where virtualChildren is a pointer

Ok, perhaps it's me that I'm used to that from other projects (so many coding styles that sometimes it's hard to remember which one is the right one).

Glad to see my comments were somehow useful, anyway :)

[chris fleizach]

Part1
http://trac.webkit.org/changeset/100487

[chris fleizach]

and final part
http://trac.webkit.org/changeset/100488

[chris fleizach]

looks like this is causing some trouble on GTK...

looking into it


  GEN    DerivedSources/InjectedBundle/JSAccessibilityTextMarkerRange.cpp
<command-line>:0:1: error: macro names must be identifiers
<command-line>:0:1: error: macro names must be identifiers
<command-line>:0:1: error: macro names must be identifiers

[chris fleizach]

(In reply to comment #20)
> looks like this is causing some trouble on GTK...
> 
> looking into it
> 
> 
>   GEN    DerivedSources/InjectedBundle/JSAccessibilityTextMarkerRange.cpp
> <command-line>:0:1: error: macro names must be identifiers
> <command-line>:0:1: error: macro names must be identifiers
> <command-line>:0:1: error: macro names must be identifiers

Or maybe not. looks like GTK has been failing for a while

[chris fleizach]

It looks like the release build of WebKitTestRunner is failing on something to do with this patch...

[chris fleizach]

(In reply to comment #22)
> It looks like the release build of WebKitTestRunner is failing on something to do with this patch...

However, I can build release just fine on my machine. Unfortunately the new run-webkit-tests does not output what happens when "check build" fails

[Adam Roben (:aroben)]

This may have caused bug 73326.