[webkit-reviews] review denied: [Bug 120416] [GTK] AccessibilityUIElement::addNotificationListener() crashes on debug build : [Attachment 210626] Patch

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <mario at webkit.org> has denied Denis Nomiyama (dnomi)
<d.nomiyama at samsung.com>'s request for review:
Bug 120416: [GTK] AccessibilityUIElement::addNotificationListener() crashes on
debug build
https://bugs.webkit.org/show_bug.cgi?id=120416

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

------- Additional Comments from Mario Sanchez Prada <mario at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=210626&action=review


Thanks Denis for working on this. I think the patch is better and cleaner now.
Just setting the r- because of the small issues found and pointed below

> Tools/DumpRenderTree/atk/AccessibilityCallbacks.h:-36
> -const PlatformUIElement GlobalNotificationKey = 0;
> -

I love this change :)

> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:51
> +typedef HashMap<PlatformUIElement, AccessibilityNotificationHandler*>
NotificationHandlers;

'NotificationHandlers' does not seem to me like a good name for a type, it
looks more like a variable name to me.

What about NotificationHandlersMap?

> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:138
> +	       JSObjectCallAsFunction(jsContext,
elementNotificationHandler.get()->value->notificationFunctionCallback(), 0, 1,
&notificationNameArgument, 0);

HashTableIteratorAdapter has the '->' operator overloaded to behave as get(),
so you can't replace ".get()->" here for just "->"

> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:232
> +	       JSValueUnprotect(jsContext,
currentNotificationHandler.get()->value->notificationFunctionCallback());
> +	      
notificationHandlers.remove(currentNotificationHandler.get()->value->platformEl
ement());

Replace ".get()->" here for just "->" here as well

Additionally, you might want to check that
currentNotificationHandler->value->platformElement() is valid before using it?

> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:261
> +	   NotificationHandlers::iterator removeNotificationHandler =
notificationHandlers.find(notificationHandler->platformElement());

Same concern about notificationHandler->platformElement() here. It might be
interesting to check that it's a valid value before using it.

> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:263
> +	       JSValueUnprotect(jsContext,
removeNotificationHandler.get()->value->notificationFunctionCallback());

s/'.get()->'/'->'