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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Sep 6 03:50:09 PDT 2013

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

Attachment 210626: Patch

------- 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' 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,
> +	      

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 =

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,


More information about the webkit-reviews mailing list