Bug 42131

Summary: WebKitTestRunner needs to support accessibility-related DRT APIs
Product: WebKit Reporter: Maciej Stachowiak <mjs>
Component: WebKit2Assignee: chris fleizach <cfleizach>
Status: RESOLVED FIXED    
Severity: Normal CC: aroben, bdakin, gustavo, jcraig, mario, mrobinson, webkit.review.bot, xan.lopez
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
patch
none
patch
darin: review-, gustavo: commit-queue-
patch
none
patch
none
patch
none
patch
none
patch
none
patch bdakin: review+

Description Maciej Stachowiak 2010-07-12 19:47:54 PDT
WebKitTestRunner needs to support accessibility-related DRT APIs
Comment 1 Maciej Stachowiak 2010-07-12 19:49:27 PDT
<rdar://problem/8183711>
Comment 2 chris fleizach 2011-11-01 10:46:50 PDT
Created attachment 113192 [details]
patch
Comment 3 chris fleizach 2011-11-01 10:53:10 PDT
Created attachment 113195 [details]
patch
Comment 4 WebKit Review Bot 2011-11-01 10:57:20 PDT
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.
Comment 5 Gustavo Noronha (kov) 2011-11-01 11:18:17 PDT
Comment on attachment 113195 [details]
patch

Attachment 113195 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10148012
Comment 6 Darin Adler 2011-11-01 15:55:08 PDT
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*?
Comment 7 chris fleizach 2011-11-02 09:35:35 PDT
(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?
Comment 8 chris fleizach 2011-11-02 12:13:12 PDT
Created attachment 113348 [details]
patch
Comment 9 chris fleizach 2011-11-02 18:34:10 PDT
Created attachment 113414 [details]
patch
Comment 10 chris fleizach 2011-11-03 08:40:00 PDT
Created attachment 113497 [details]
patch
Comment 11 chris fleizach 2011-11-03 09:50:27 PDT
Created attachment 113509 [details]
patch
Comment 12 chris fleizach 2011-11-03 10:25:30 PDT
Created attachment 113518 [details]
patch
Comment 13 chris fleizach 2011-11-03 11:00:49 PDT
Created attachment 113524 [details]
patch
Comment 14 Mario Sanchez Prada 2011-11-15 02:55:15 PST
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).
Comment 15 chris fleizach 2011-11-16 09:09:26 PST
(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
Comment 16 chris fleizach 2011-11-16 09:14:27 PST
> > 
> > > 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
Comment 17 Mario Sanchez Prada 2011-11-16 09:49:52 PST
(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 :)
Comment 18 chris fleizach 2011-11-16 12:15:33 PST
Part1
http://trac.webkit.org/changeset/100487
Comment 19 chris fleizach 2011-11-16 12:18:00 PST
and final part
http://trac.webkit.org/changeset/100488
Comment 20 chris fleizach 2011-11-16 12:30:41 PST
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
Comment 21 chris fleizach 2011-11-16 12:33:07 PST
(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
Comment 22 chris fleizach 2011-11-16 15:59:08 PST
It looks like the release build of WebKitTestRunner is failing on something to do with this patch...
Comment 23 chris fleizach 2011-11-16 16:08:48 PST
(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
Comment 24 Adam Roben (:aroben) 2011-11-29 08:25:40 PST
This may have caused bug 73326.