[webkit-reviews] review denied: [Bug 132527] AX: [ATK] [PATCH] add text-caret-moved signal to accessibilityController : [Attachment 230759] the patch, version 1.00

[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 230759: the patch, version 1.00
https://bugs.webkit.org/attachment.cgi?id=230759&action=review

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


Thanks for the patch. The idea looks good to me, although I think it would be
nice if we could include a new layout test using this new feature together with
the patch (e.g. migrating one of the unit tests in the old testatk.c file), to
confirm it works and have it properly justified.

Also, now that the WebKitGTK+ port removed the WebKit1 API from trunk, I'm not
sure whether it makes sense to keep mirroring the changes from WKTR here.

The only reason would be the EFL port, but I think they pretty much use WebKit2
only these days (yet I'm not sure). It would be nice if someone from EFL could
answer that question (e.g. Krzysztof?)

See my comments below (setting r- for now)...

>
Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.c
pp:93
> +    AtkRole objectRole = atk_object_get_role(accessible);
> +    const gchar* roleName = atk_role_get_name(objectRole);

Why do you need the role and the name of the object here? This variables are
used only to set the value of signalValue, which will be used only to pass it
to printAccessibilityEvent(), where the role will be retrieved and printed
anyway

(Same comment apply to DRT implementation)

>
Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.c
pp:122
> +	   signalValue.reset(g_strdup_printf("%s|%d",
> +	       roleName, g_value_get_int(&paramValues[1])));

you don't need to emit the roleName explicitly here, just the value for the
offset (the "%d")

(Same comment apply to DRT implementation)