Summary: | WebKitTestRunner needs to support accessibility-related DRT APIs | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Maciej Stachowiak <mjs> | ||||||||||||||||||
Component: | WebKit2 | Assignee: | 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
Maciej Stachowiak
2010-07-12 19:47:54 PDT
Created attachment 113192 [details]
patch
Created attachment 113195 [details]
patch
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 on attachment 113195 [details] patch Attachment 113195 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10148012 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*? (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? Created attachment 113348 [details]
patch
Created attachment 113414 [details]
patch
Created attachment 113497 [details]
patch
Created attachment 113509 [details]
patch
Created attachment 113518 [details]
patch
Created attachment 113524 [details]
patch
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). (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
> >
> > > 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
(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 :) and final part http://trac.webkit.org/changeset/100488 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 (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 It looks like the release build of WebKitTestRunner is failing on something to do with this patch... (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 This may have caused bug 73326. |