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

[bugzilla-daemon at webkit.org]

Eric Seidel <eric 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 52746: patch
https://bugs.webkit.org/attachment.cgi?id=52746&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
I'm not sure the memory management is right in this change.  Why are either of
these weak pointers safe:

0     id m_platformElement;
 91	JSObjectRef m_notificationFunctionCallback;

I'm glad you documented this:
 105	 // This is to avoid any race conditions between tests turning this
flag on and off. Instead
I agree with your choice to leave it on.

Why call it "notification" if it's really the "notificationName"?
 2670	      NSDictionary* userInfo = [NSDictionary
dictionaryWithObjectsAndKeys:notification, @"notification", nil];

Are we guaranteed that the entire AccessibilityUIElement tree will be destroyed
when the page navigates?  Otherwise I worry about results possibly bleeding
into other tests.

0     // Already added once.
 831	 if (m_notificationHandler)
 832	     return false;
This seems like a programmer error, we should ASSERT or log or something here,
no?

We should document (comment) the memory management for these members:
180182	   PlatformUIElement m_element;
181	 JSObjectRef m_notificationFunctionCallback;
 183	 void* m_notificationHandler;

Otherwise I think this is OK.  I didn't look at the tests very closely.  I'm
concerned about the various memory management issues listed above, so r-.

I'm feeling mostly better now, so reviews should be much quicker now that I"m
not sleeping most of the day. :)