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

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied 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 53032: patch
https://bugs.webkit.org/attachment.cgi?id=53032&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
>From what I see, it appears that one of subsequent tests may mysteriously quit
during garbage collection (since you used assert and not ASSERT, there won't
even be a stack trace).

Asserting in addNotificationListener() also seems wrong - the function needs to
be renamed to something like setNotificationListener() if it doesn't allow
"adding" more than one.

Requiring tests to clean up after themselves is really not what we do. In fact,
I made the same mistake with some plug-in test methods a few week ago, and got
bitten by it almost immediately! It was really confusing.

More comments, some of these minot nitpicks:

+static BOOL AXShouldRepostNotifications = NO;

Old code also had this issue, but variable names should start with lower case
letter per coding style guidelines. I usually advise to not initialize static
variables explicitly - since they are guaranteed to start with zero, explicit
initialization is just visual noise. It's almost the same as initializing
RefPtr members in constructors with zero.

+    // The PlatformUIElement should be retained.
     PlatformUIElement m_element;

I don't believe calling retain is sufficient. It's a no-op if garbage
collection is enabled, and I don't think that Objective-C garbage collection
looks inside C++ objects. So, it should use CFRetain/CFRelease.

It's a shame we can't use RetainPtr here.

+#if PLATFORM(MAC)
+#ifdef __OBJC__
+    id m_notificationHandler;
+#endif 
+#endif

This is wrong - the size of the object will be smaller if the header included
in a C/C++ file. The correct way to handle this is how PlatformUIElement is
handled.

+    // Once when object starts requesting notifications, its on for the
duration of the program.

Typos: "once when", and "its" instead of "it's".

+    // This is to avoid any race conditions between tests turning this flag on
and off. Instead

I'm not sure if I understand what race conditions exist here. Aren't
notifications delivered synchronously?

+    NSString* notificationName = [[notification userInfo]
objectForKey:@"notificationName"];

We still position stars away from ObjC class names, so this should be "NSString
*notificationName". Same issue elsewhere.

+    JSObjectCallAsFunction([mainFrame globalContext],
m_notificationFunctionCallback, NULL, 1, &argument, NULL);

0, not NULL.

+- (void)setCallback:(JSObjectRef)callback
+{
+    m_notificationFunctionCallback = callback;
+}

m_notificationFunctionCallback doesn't seem to be GC protected anywhere. This
is not a practical issue for existing tests, as the callback is protected by
the virtue of being defined on global object, but it would be a problem for
JavaScript code like this:

liveRegion.addNotificationListener(function() { alert(0) } );
// Force garbage collection
// Do something that dispatches the notification.