[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





--- Comment #6 from Denis Nomiyama (dnomi) <d.nomiyama at samsung.com>  2013-09-05 06:37:49 PST ---
(From update of attachment 210448)
View in context: https://bugs.webkit.org/attachment.cgi?id=210448&action=review

>> Tools/ChangeLog:10
>> +        m_notificationHandler expected RefPtr.
> 
> Nit. You might probably use an extra line here to separate paragraphs

Ok. I'll do.

>> 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.

Ok.

>> 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.

Ok.

>> Tools/DumpRenderTree/atk/AccessibilityCallbacks.h:35
>> +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?

Before the changes for this review, I could move this fix to bug 70606, where the implementation of global notifications is being done. But the new patch will have a call to find() at axObjectEventListener() and the debug build will crash if the key is zero.

>> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:136
>>          }
> 
> It looks to me like you could simplify this code by replacing it with a single line:
> 
>   AccessibilityNotificationHandler* notificationHandler = notificationHandlers.find(accessible);

Nice one. I'll do something similar. Just have to modify it a bit because find() returns an iterator.

>> 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.

Ops. Sorry.

>> 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)) {
>      ...
>   }

Ok I'll simplify it.

>> 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)

Good point. I'll do it.

>> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:152
>> +        }
> 
> 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);
>   }

Nice one. I'll do something similar, just because find() returns an iterator.

>> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:235
>> +        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?

Sure. I'll do it.

>> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:265
>> +        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;
>      }
>   }

Ok. I think we can use find() here as well. I'll give it a try.

>> LayoutTests/ChangeLog:15
>> +        - 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 :)

Cool ok.

>> LayoutTests/ChangeLog:18
>> +        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)

Ok. I will fix the comment and will add the reference to the other bug.

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