[Webkit-unassigned] [Bug 132527] AX: [ATK] [PATCH] add text-caret-moved signal to accessibilityController

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=132527


Mario Sanchez Prada <mario at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #230759|review?                     |review-
               Flag|                            |




--- Comment #3 from Mario Sanchez Prada <mario at webkit.org>  2014-05-06 05:18:59 PST ---
(From update of attachment 230759)
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.cpp: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.cpp: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)

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