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

[bugzilla-daemon at webkit.org]

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 210448: Patch
https://bugs.webkit.org/attachment.cgi?id=210448&action=review

------- Additional Comments from Mario Sanchez Prada <mario at webkit.org>
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->notificationFunctionCa
llback(), 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->notif
icationFunctionCallback());
> +	   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)