<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><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> changed
<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>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #255452 Flags</td>
<td>review?
</td>
<td>review+
</td>
</tr></table>
<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#c8">Comment # 8</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=255452&action=diff" name="attach_255452" title="patch">attachment 255452</a> <a href="attachment.cgi?id=255452&action=edit" title="patch">[details]</a></span>
patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=255452&action=review">https://bugs.webkit.org/attachment.cgi?id=255452&action=review</a>
<span class="quote">> Source/WebCore/accessibility/AXObjectCache.cpp:1010
> + AccessibilityObject* object = getOrCreate(node);
> + postTextStateChangeNotification(object, intent, selection);</span >
Would read better without the local variable.
<span class="quote">> Source/WebCore/accessibility/AXObjectCache.cpp:1028
> + if (object && object->accessibilityIsIgnored()) {</span >
Does getOrCreate actually ever return null? The name makes it sound like it wouldn’t.
<span class="quote">> Source/WebCore/accessibility/AXObjectCache.cpp:1030
> + if (AccessibilityObject *nextSibling = object->nextSiblingUnignored(1))</span >
Incorrect formatting for WebKit coding style; * is in the wrong place. Also, I suggest using auto or auto* for this.
<span class="quote">> Source/WebCore/accessibility/AXObjectCache.cpp:1033
> + if (AccessibilityObject *previousSibling = object->previousSiblingUnignored(1))</span >
Ditto.
<span class="quote">> Source/WebCore/accessibility/AccessibilityObject.cpp:464
> + if (limit < 0)
> + limit = std::numeric_limits<int>::max();</span >
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.
<span class="quote">> Source/WebCore/accessibility/AccessibilityObject.cpp:465
> + for (previous = this->previousSibling(); previous && previous->accessibilityIsIgnored(); previous = previous->previousSibling()) {</span >
Normally we’d write this without this->
<span class="quote">> 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];</span >
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.
<span class="quote">> Tools/DumpRenderTree/mac/AccessibilityNotificationHandler.mm:100
> + static Class cls = nil;
> + if (!cls)
> + cls = NSClassFromString(@"WebAccessibilityObjectWrapper");</span >
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.
<span class="quote">> Tools/DumpRenderTree/mac/AccessibilityNotificationHandler.mm:129
> + return AccessibilityUIElement::makeJSAccessibilityUIElement(context, AccessibilityUIElement(value));</span >
Is the explicit conversation to AccessibilityUIElement needed? Does it compile if you just leave it Oout?
<span class="quote">> Tools/DumpRenderTree/mac/AccessibilityNotificationHandler.mm:152
> + for (NSString *key in dictionary) {</span >
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.
<span class="quote">> Tools/DumpRenderTree/mac/AccessibilityNotificationHandler.mm:154
> + auto propertyName = adopt([key createJSStringRef]);</span >
Not great to do this outside the if since it’s only used inside the if.</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>