Summary: | AX: AXObjectCache should try to use an unignored accessibilityObject when posting a selection notification when on the border between two accessibilityObjects | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Doug Russell <d_russell> | ||||||||||||
Component: | Accessibility | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | commit-queue, darin, rniwa, webkit-bug-importer | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Doug Russell
2015-06-19 18:54:10 PDT
Created attachment 255275 [details]
patch
Comment on attachment 255275 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=255275&action=review Same comments apply to all the copied and pasted versions in DumpRenderTree; I commented on the WebKitTestRunner code. > Source/WebCore/accessibility/AXObjectCache.cpp:1020 > + Node* node = position.deprecatedNode(); Why is it right to use deprecatedNode here? Ryosuke, is that right? > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityNotificationHandler.mm:112 > + static Class cls = nil; > + static dispatch_once_t onceToken; > + dispatch_once(&onceToken, ^{ > + cls = NSClassFromString(@"WebAccessibilityObjectWrapper"); > + }); > + ASSERT(cls); > + return cls; In WebKit code itself we often turn off the thread safety for globals. But if this is only used from one thread or if the thread safety for globals is turned on in WebKitTestRunner (which I think it could be), then this should just be: static Class wrapperClass = objc_getClass("WebAccessibilityObjectWrapper"); return wrapperClass; I also suggest using objc_getClass, since NSClassFromString just turns around and calls that. > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityNotificationHandler.mm:129 > +static JSValueRef MakeValueRefForValue(JSContextRef context, id value) WebKit project does not capitalize function names. > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityNotificationHandler.mm:132 > + return JSValueMakeString(context, JSRetainPtr<JSStringRef>(Adopt, [value createJSStringRef]).get()); We really need an adopt function to help write this kind of thing. The constructor syntax is super awkward. > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityNotificationHandler.mm:139 > + return toJS(context, WTF::getPtr(WTR::AccessibilityUIElement::create(static_cast<PlatformUIElement>(value)))); No need to use WTF::getPtr here. Should just use the .get() function. The getPtr functions exists for generic programming that needs to work without knowing the type. > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityNotificationHandler.mm:147 > +static JSValueRef MakeArrayRefForArray(JSContextRef context, NSArray *array) WebKit project does not capitalize function names. > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityNotificationHandler.mm:150 > + JSValueRef arguments[count]; We normally don’t use the variable-sized array extension, but it’s probably oK for code only used in the test tool. > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityNotificationHandler.mm:158 > +static JSValueRef MakeObjectRefForDictionary(JSContextRef context, NSDictionary *dictionary) WebKit project does not capitalize function names. > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityNotificationHandler.mm:164 > + > + JSRetainPtr<JSStringRef> propertyName = JSRetainPtr<JSStringRef>(Adopt, [key createJSStringRef]); This should just be: JSRetainPtr<JSStringRef> propertyName { Adopt, [key createJSStringRef] }; But it would be even better if we had an adopt function so we could write: auto propertyName = adopt([key createJSStringRef]); Can adopt just be a free function in JSRetainPtr.h like template<typename T> JSRetainPtr<T> adopt(T value) { return JSRetainPtr<T>(Adopt, value); } or should it be a static method on JSRetainPtr (or something else)? Created attachment 255444 [details]
patch
(In reply to comment #4) > Can adopt just be a free function in JSRetainPtr.h like > > template<typename T> JSRetainPtr<T> adopt(T value) > { > return JSRetainPtr<T>(Adopt, value); > } > > or should it be a static method on JSRetainPtr (or something else)? It can and should be something like that, but that template is too generic and would apply to too many different classes. Instead we should just do overloading, I think: inline JSRetainPtr<JSStringRef> adopt(JSStringRef string) { return JSRetainPtr<JSStringRef>(Adopt, string); } We can overload for other types as needed. Not sure what types JSRetainPtr supports. Later we will want to make the JSRetainPtr constructor for Adopt be private and move to this function exclusively. Created attachment 255452 [details]
patch
Comment on attachment 255452 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=255452&action=review > Source/WebCore/accessibility/AXObjectCache.cpp:1010 > + AccessibilityObject* object = getOrCreate(node); > + postTextStateChangeNotification(object, intent, selection); Would read better without the local variable. > Source/WebCore/accessibility/AXObjectCache.cpp:1028 > + if (object && object->accessibilityIsIgnored()) { Does getOrCreate actually ever return null? The name makes it sound like it wouldn’t. > Source/WebCore/accessibility/AXObjectCache.cpp:1030 > + if (AccessibilityObject *nextSibling = object->nextSiblingUnignored(1)) Incorrect formatting for WebKit coding style; * is in the wrong place. Also, I suggest using auto or auto* for this. > Source/WebCore/accessibility/AXObjectCache.cpp:1033 > + if (AccessibilityObject *previousSibling = object->previousSiblingUnignored(1)) Ditto. > Source/WebCore/accessibility/AccessibilityObject.cpp:464 > + if (limit < 0) > + limit = std::numeric_limits<int>::max(); This seems unnecessarily complex. If the default should be std::numeric_limits<int>::max(), then lets specify that default in the header, instead of making negative numbers magic. Also, we never use this feature, always passing 1 for limit, so this is dead untested code. > Source/WebCore/accessibility/AccessibilityObject.cpp:465 > + for (previous = this->previousSibling(); previous && previous->accessibilityIsIgnored(); previous = previous->previousSibling()) { Normally we’d write this without this-> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:447 > + if ((userInfo = dictionaryRemovingNonSupportedTypes(userInfo))) > + info = [NSDictionary dictionaryWithObjectsAndKeys:notificationName, @"notificationName", userInfo, @"userInfo", nil]; > + else > info = [NSDictionary dictionaryWithObjectsAndKeys:notificationName, @"notificationName", nil]; Looks OK, but there is one sleazy shortcut we could take. No need to do the null check on userInfo because if it’s nil it will do the right thing, terminating the list early. > Tools/DumpRenderTree/mac/AccessibilityNotificationHandler.mm:100 > + static Class cls = nil; > + if (!cls) > + cls = NSClassFromString(@"WebAccessibilityObjectWrapper"); This is C++; no need for the if statement. This works: static Class cls = objc_getClass("WebAccessibilityObjectWrapper"); In fact, I would have just added "static" to the existing code instead of adding a new function. And even that change is not really needed; just makes this patch bigger for no significant benefit. > Tools/DumpRenderTree/mac/AccessibilityNotificationHandler.mm:129 > + return AccessibilityUIElement::makeJSAccessibilityUIElement(context, AccessibilityUIElement(value)); Is the explicit conversation to AccessibilityUIElement needed? Does it compile if you just leave it Oout? > Tools/DumpRenderTree/mac/AccessibilityNotificationHandler.mm:152 > + for (NSString *key in dictionary) { Probably more efficient to use the method that enumerates with a block, since it gives both key and value and eliminates all the hash table lookups. > Tools/DumpRenderTree/mac/AccessibilityNotificationHandler.mm:154 > + auto propertyName = adopt([key createJSStringRef]); Not great to do this outside the if since it’s only used inside the if. Created attachment 255501 [details]
patch
Created attachment 255502 [details]
patch
Comment on attachment 255502 [details] patch Clearing flags on attachment: 255502 Committed r185924: <http://trac.webkit.org/changeset/185924> All reviewed patches have been landed. Closing bug. |