[webkit-reviews] review granted: [Bug 37112] aria-liveregion-notifications.html fails on leopard release bot : [Attachment 53868] patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted chris fleizach
<cfleizach at apple.com>'s request for review:
Bug 37112: aria-liveregion-notifications.html fails on leopard release bot
https://bugs.webkit.org/show_bug.cgi?id=37112

Attachment 53868: patch
https://bugs.webkit.org/attachment.cgi?id=53868&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
+	 * Scripts/prepare-ChangeLog:

This doesn't seem intentional. Actual prepare-ChangeLog changes are also in
this patch.

+- (void)setCallback:(JSObjectRef)callback
+{
+    if (!callback)
+	 return;
+    
+    m_notificationFunctionCallback = callback;
+    JSValueProtect([mainFrame globalContext], m_notificationFunctionCallback);

+}

My understanding is that you intentionally don't unprotect
m_notificationFunctionCallback, because addNotificationListener() can't be
called twice. I think it's worth an assertion or a comment here, because this
is not a usual setter pattern.

+    // Mac programmers should not be adding more than one notification
listener per element.
+    // Other platforms may be different.

As an aside, is there any chance of having cross-platform accessibility tests?
I do not know if that even makes sense, but we've been able to make some
cross-platform tests even for inline text input.

You can consider adding a FIXME about m_element and m_notificationHandler
needing CFRetain/CFRelease. It would be good to make DRT work with garbage
collected ObjC one day.

Also, I still don't understand what the race condition is here (see comment
31).

r=me