[Webkit-unassigned] [Bug 120416] [GTK] AccessibilityUIElement::addNotificationListener() crashes on debug build

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=120416


Mario Sanchez Prada <mario at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #210626|review?                     |review-
               Flag|                            |




--- Comment #9 from Mario Sanchez Prada <mario at webkit.org>  2013-09-06 03:49:26 PST ---
(From update of attachment 210626)
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->platformElement());

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()->'/'->'

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.