[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
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,
¬ificationNameArgument, 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()->'/'->'
More information about the webkit-reviews
mailing list