[webkit-reviews] review denied: [Bug 132527] AX: [ATK] [PATCH] add text-caret-moved signal to accessibilityController : [Attachment 231629] caret-moved signal and caret offset tests v2.04

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <mario at webkit.org> has denied Jarek Czekalski
<jarekczek at poczta.onet.pl>'s request for review:
Bug 132527: AX: [ATK] [PATCH] add text-caret-moved signal to
accessibilityController
https://bugs.webkit.org/show_bug.cgi?id=132527

Attachment 231629:  caret-moved signal and caret offset tests v2.04
https://bugs.webkit.org/attachment.cgi?id=231629&action=review

------- Additional Comments from Mario Sanchez Prada <mario at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=231629&action=review


Hi Jarek, apologies for the delayed response, but last week I've been quite
busy (and mostly offline) most of the time and could not devote time to this at
all. Now I'm back, and reviewing your patch again, which I think it's almost
there. I just have a couple of comments/suggestions more.

See them below...

> LayoutTests/ChangeLog:29
> +	   * platform/gtk/accessibility/caret-offsets.html: Added.

Sorry I haven't realized about this before, but I think that this file is
actually too big (as it includes tests from both testWebkitAtkCaretOffsets()
and testWebkitAtkCaretOffsetsAndExtraneousWhiteSpaces()). Could you better
split it into two different .html files? Something like this: 

  - platform/gtk/accessibility/caret-offsets.html
  - platform/gtk/accessibility/caret-offsets-and-extraneous-white-spaces.html

>
Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.c
pp:147
>	       // Listener for one element just gets one argument, the
notification name.

This comment is obsolete now

>
Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.c
pp:150
> +		   0, lastArgument + 1 - 1, arguments + 1, 0);

lastArgument + 1 - 1? That looks like a strange idiom, even if it's correct. I
personally think something like this would be less cryptic:

 [...]

 size_t numOfExtraArgs = extraArgs.size();
 for (int i = 0; i < numOfExtraArgs; i++)
     arguments[i + 2] = extraArgs[i];

 if (elementNotificationHandler != notificationHandlers.end()) {
     // Listener for one element. As arguments, it gets the notification name
     // plus any extra argument that must be needed to pass to the callback
     JSObjectCallAsFunction(jsContext,
	
const_cast<JSObjectRef>(elementNotificationHandler->value->notificationFunction
Callback()),
	 0, numOfExtraArgs + 1, arguments + 1, 0);

 [...]


What do you think?

>
Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.c
pp:157
> +		   0, lastArgument + 1, arguments, 0);

If you agree with the proposed change above, then you'd need to use
numberOfExtraArgs + 2 here

>
Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.c
pp:273
> +	   guint id = atk_add_global_event_listener(axObjectEventListener,
*signalName);

Use non-glib types for basic data (e.g. unsigned instead of guint) for this
kind of variables in private files

>
Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.c
pp:278
> +	   if (!id) {
> +	       String message = String::format("atk_add_global_event_listener
failed for signal %s\n", *signalName);
> +	       InjectedBundle::shared().outputText(message);
> +	   } else
> +	       listenerIds.append(id);

Could you make this just an "if" with an "early continue"?

  for (...) {
   [...]
   if (!id) {
       String message = String::format("atk_add_global_event_listener failed
for signal %s\n", *signalName);
       InjectedBundle::shared().outputText(message);
       continue;
   }

   listenerIds.append(id);
  }