[Webkit-unassigned] [Bug 120416] [GTK] AccessibilityUIElement::addNotificationListener() crashes on debug build
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Sep 4 08:25:49 PDT 2013
https://bugs.webkit.org/show_bug.cgi?id=120416
Mario Sanchez Prada <mario at webkit.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #210448|review? |review-
Flag| |
--- Comment #5 from Mario Sanchez Prada <mario at webkit.org> 2013-09-04 08:25:07 PST ---
(From update of attachment 210448)
View in context: https://bugs.webkit.org/attachment.cgi?id=210448&action=review
Thanks for taking care of this. See my comments below...
> Tools/ChangeLog:10
> + m_notificationHandler expected RefPtr.
Nit. You might probably use an extra line here to separate paragraphs
> Tools/ChangeLog:14
> + * DumpRenderTree/atk/AccessibilityCallbacks.h: modified the global notification key from 0 to 1
Please use proper sentences starting with capital letters and ending with a period.
> Tools/ChangeLog:16
> + (axObjectEventListener): Moved the call to the callback function out of the loop
Missing period. Same applies to the remaining bullet points from this entry.
> Tools/DumpRenderTree/atk/AccessibilityCallbacks.h:35
> -const PlatformUIElement GlobalNotificationKey = 0;
> +const PlatformUIElement GlobalNotificationKey = reinterpret_cast<PlatformUIElement>(0x1);
This change seems unrelated to this fix, so please do not include it if it's not needed.
Besides, the whole thing of having this cryptic value in a header file to be used as a key in a hashmap present in the implementatoin file (hence private) it's a bit too obscure too me. Perhaps it would be nice to have make a patch in the future just for either find a better way to do it or at least document it?
> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:136
> + AccessibilityNotificationHandler* notificationHandler = 0;
> for (HashMap<PlatformUIElement, AccessibilityNotificationHandler*>::iterator it = notificationHandlers.begin(); it != notificationHandlers.end(); ++it) {
> - if (it->key == accessible || it->key == GlobalNotificationKey) {
> - JSRetainPtr<JSStringRef> jsNotificationEventName(Adopt, JSStringCreateWithUTF8CString(reinterpret_cast<const char*>(notificationName.utf8().data())));
> - JSValueRef notificationNameArgument = JSValueMakeString(jsContext, jsNotificationEventName.get());
> - AccessibilityNotificationHandler* notificationHandler = it->value;
> - if (notificationHandler->platformElement()) {
> - JSValueRef argument = notificationNameArgument;
> - // Listener for one element just gets one argument, the notification name.
> - JSObjectCallAsFunction(jsContext, notificationHandler->notificationFunctionCallback(), 0, 1, &argument, 0);
> - } else {
> - // A global listener gets the element and the notification name as arguments.
> - JSValueRef arguments[2];
> - arguments[0] = AccessibilityUIElement::makeJSAccessibilityUIElement(jsContext, AccessibilityUIElement(accessible));
> - arguments[1] = notificationNameArgument;
> - JSObjectCallAsFunction(jsContext, notificationHandler->notificationFunctionCallback(), 0, 2, arguments, 0);
> - }
> + if (it->key == accessible) {
> + notificationHandler = it->value;
> + break;
> }
> }
It looks to me like you could simplify this code by replacing it with a single line:
AccessibilityNotificationHandler* notificationHandler = notificationHandlers.find(accessible);
> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:138
> + JSRetainPtr<JSStringRef> jsNotificationEventName(Adopt, JSStringCreateWithUTF8CString(reinterpret_cast<const char*>(notificationName.utf8().data())));
CString::data() already returns a const char*, so I don't think you need this reinterpret_cast here.
> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:140
> + if (notificationHandler) {
It seems notificationHandler, if present, is only used inside this if branch.
Considering my previous comment, perhaps you could do just something like the following?
if (AccessibilityNotificationHandler* notificationHandler = notificationHandlers.find(accessible)) {
...
}
> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:141
> + JSValueRef argument = notificationNameArgument;
You don't need argument variable, you can use notificationNameArgument directly below, giving you a one-line if branch (so you can avoid the {} too)
> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:152
> + if (notificationHandlers.contains(GlobalNotificationKey)) {
> + // A global listener gets the element and the notification name as arguments.
> + JSValueRef arguments[2];
> + arguments[0] = AccessibilityUIElement::makeJSAccessibilityUIElement(jsContext, AccessibilityUIElement(accessible));
> + arguments[1] = notificationNameArgument;
> + JSObjectCallAsFunction(jsContext, notificationHandlers.find(GlobalNotificationKey)->value->notificationFunctionCallback(), 0, 2, arguments, 0);
> + }
I would probably do this without using the contains() method, by doing something like this:
if (AccessibilityNotificationHandler* globalHandler = notificationHandlers.find(GlobalNotificationKey)) {
// A global listener gets the element and the notification name as arguments.
JSValueRef arguments[2];
arguments[0] = AccessibilityUIElement::makeJSAccessibilityUIElement(jsContext, AccessibilityUIElement(accessible));
arguments[1] = notificationNameArgument;
JSObjectCallAsFunction(jsContext, globalHandler->value->notificationFunctionCallback(), 0, 2, arguments, 0);
}
> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:235
> + if (notificationHandlers.contains(notificationHandler->platformElement())) {
> + JSValueUnprotect(jsContext, notificationHandlers.find(notificationHandler->platformElement())->value->notificationFunctionCallback());
> + notificationHandlers.remove(notificationHandler->platformElement());
Following the idea of my previous comment, you can probably replace this two calls to contains() and find() with just one call to find() here as well.
Additionally, perhaps it would be interesting to check first that notificationHandler->platformElement() is not null?
> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:265
> + HashMap<PlatformUIElement, AccessibilityNotificationHandler*>::iterator it;
> + for (it = notificationHandlers.begin(); it != notificationHandlers.end(); ++it) {
> if (it->value == notificationHandler) {
> JSValueUnprotect(jsContext, notificationHandler->notificationFunctionCallback());
> - notificationHandlers.remove(it);
> + break;
> }
> }
> +
> + if (it.get())
> + notificationHandlers.remove(it);
It looks to me like the key thing here is the missing break and that, if you add that, you can keep the call to remove inside the loop and avoid doing it after it, leading to something more compact such as:
for (HashMap<PlatformUIElement, AccessibilityNotificationHandler*>::iterator it = notificationHandlers.begin(); it != notificationHandlers.end(); ++it) {
if (it->value == notificationHandler) {
JSValueUnprotect(jsContext, notificationHandler->notificationFunctionCallback());
notificationHandlers.remove(it);
break;
}
}
> LayoutTests/ChangeLog:15
> + - accessibility/multiselect-list-reports-active-option.html
> + - accessibility/notification-listeners.html
> + - accessibility/menu-list-sends-change-notification.html
> + - accessibility/aria-invalid.html
> + - accessibility/aria-checkbox-sends-notification.html
You don't have to explicitly list these tests here if you want. You can be more generic in this case :)
> LayoutTests/ChangeLog:18
> + Skipped accessibility/notification-listeners.html both on Release and Debug builds since it is not
> + crashing on Debug anymore.
You are not actually skipping tests, just updating the expectations to expect a failure. It might be worth commenting here that this issue is being tracked in a separate bug (bug 120669)
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list