[Webkit-unassigned] [Bug 146177] AX: AXObjectCache should try to use an unignored accessibilityObject when posting a selection notification when on the border between two accessibilityObjects

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=146177

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #255452|review?                     |review+
              Flags|                            |

--- Comment #8 from Darin Adler <darin at apple.com> ---
Comment on attachment 255452
  --> https://bugs.webkit.org/attachment.cgi?id=255452
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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150624/1e64981b/attachment.html>