[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

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

-- 
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/20150623/d997aba7/attachment-0001.html>