<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - AX: AXObjectCache should try to use an unignored accessibilityObject when posting a selection notification when on the border between two accessibilityObjects"
href="https://bugs.webkit.org/show_bug.cgi?id=146177#c3">Comment # 3</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - AX: AXObjectCache should try to use an unignored accessibilityObject when posting a selection notification when on the border between two accessibilityObjects"
href="https://bugs.webkit.org/show_bug.cgi?id=146177">bug 146177</a>
from <span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=255275&action=diff" name="attach_255275" title="patch">attachment 255275</a> <a href="attachment.cgi?id=255275&action=edit" title="patch">[details]</a></span>
patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=255275&action=review">https://bugs.webkit.org/attachment.cgi?id=255275&action=review</a>
Same comments apply to all the copied and pasted versions in DumpRenderTree; I commented on the WebKitTestRunner code.
<span class="quote">> Source/WebCore/accessibility/AXObjectCache.cpp:1020
> + Node* node = position.deprecatedNode();</span >
Why is it right to use deprecatedNode here? Ryosuke, is that right?
<span class="quote">> 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;</span >
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.
<span class="quote">> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityNotificationHandler.mm:129
> +static JSValueRef MakeValueRefForValue(JSContextRef context, id value)</span >
WebKit project does not capitalize function names.
<span class="quote">> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityNotificationHandler.mm:132
> + return JSValueMakeString(context, JSRetainPtr<JSStringRef>(Adopt, [value createJSStringRef]).get());</span >
We really need an adopt function to help write this kind of thing. The constructor syntax is super awkward.
<span class="quote">> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityNotificationHandler.mm:139
> + return toJS(context, WTF::getPtr(WTR::AccessibilityUIElement::create(static_cast<PlatformUIElement>(value))));</span >
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.
<span class="quote">> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityNotificationHandler.mm:147
> +static JSValueRef MakeArrayRefForArray(JSContextRef context, NSArray *array)</span >
WebKit project does not capitalize function names.
<span class="quote">> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityNotificationHandler.mm:150
> + JSValueRef arguments[count];</span >
We normally don’t use the variable-sized array extension, but it’s probably oK for code only used in the test tool.
<span class="quote">> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityNotificationHandler.mm:158
> +static JSValueRef MakeObjectRefForDictionary(JSContextRef context, NSDictionary *dictionary)</span >
WebKit project does not capitalize function names.
<span class="quote">> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityNotificationHandler.mm:164
> +
> + JSRetainPtr<JSStringRef> propertyName = JSRetainPtr<JSStringRef>(Adopt, [key createJSStringRef]);</span >
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]);</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>