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

[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 231455: caret-moved signal and caret offset tests
https://bugs.webkit.org/attachment.cgi?id=231455&action=review

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


Sorry for the delay, Jarek. This week is being crazy here, but I'd rather
review this now because next week will be even more complex :)

This patch is awesome, I'm placing the r- flag now because there are some
things that need fixing, but it seems to be indeed in the right direction. See
my comments below...

> LayoutTests/ChangeLog:8
> +	   * platform/gtk/accessibility/js-test-atk.js: Added, to place

I think it's ok to have a new file to place this helpers (and probably others
that are used in different ATK tests) in a common place, but I'd rather put it
in a platform/gtk/resources directory, for consistency with what's done
everywhere else. Also (and sorry for the bike shedding), I'd rather name it
something like atk-helpers.js or the like, not need to use a "js-" prefix in a
.js file :)

> LayoutTests/platform/gtk/accessibility/caret-offsets.html:5
> +<script src="js-test-atk.js"></script>

Remember to update this path if you do the change suggested above

> LayoutTests/platform/gtk/accessibility/caret-offsets.html:9
> +<script>
> +descriptionText = "This test is replicated from old file testatk.c and tests
various scenarios<br>" +
> +		     "of caret movement: setting caret offset and receiving
text-caret-moved signal.";
> +</script>

We normally use the description() JS function for this purpose (see for
instance accessibility/button-title-uses-inner-img-alt.html)

> LayoutTests/platform/gtk/accessibility/caret-offsets.html:44
> +if (window.layoutController) {

s/layoutController/testRunner (layoutController was the old name of the
testRunner)

> LayoutTests/platform/gtk/accessibility/caret-offsets.html:47
> +if (window.layoutController) {
> +    testRunner.overridePreference("WebKitEnableCaretBrowsing", true);
> +    testRunner.dumpAsText();
> +}

Could you move this "if { ... }" block after the functions? (to keep the
functions at the beginning)?

> LayoutTests/platform/gtk/accessibility/caret-offsets.html:53
> +	   caretMovedData += role + '|' + offset;

You are using caretMovedData as a global variable, so you should declare it
with "var caretMovedData" outside every block and every function (not needed
but good practice)

> LayoutTests/platform/gtk/accessibility/caret-offsets.html:57
> +function setCaretOffsetHelper(object, offset)

Perhaps naming this 'resetCaretOffsetForObject()' would be clearer

> LayoutTests/platform/gtk/accessibility/caret-offsets.html:81
> +    /* Following tests are replicated from testatk.c file, functions
> +    testWebkitAtkCaretOffsets()
> +    testWebkitAtkCaretOffsetsAndExtranousWhiteSpaces()
> +    that was present in webkit1. Removed from
> +    trunk in r166977, so the last version was
> +   
https://trac.webkit.org/browser/trunk/Tools/TestWebKitAPI/Tests/WebKitGtk/testa
tk.c?rev=166976
> +    What could not be verified in the same way:
> +    1. Whether element implements ATK_TEXT. If setCaretOffset succeeds, it
confirms
> +	  that element is text as well. But in cases when setCaretOffset must
fail
> +	  I see no way to confirm it is text element.
> +    2. Whether position is correct through a call to
atk_text_get_caret_offset.
> +	  text-caret-moved signal value is tested instead.
> +    Anyway these cases don't look crucial, so they are skipped.
> +    */

I think this is a great comment to put in the LayoutTests/ChangeLog, but I'm
not sure you need to put it here because, as you mentioned, those cases are not
crucial anyway :)

> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:62
> +// Up to 2014 it was obligatory to mirror the changes from
> +//
WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp,
> +// but the habit has been dropped:
https://bugs.webkit.org/show_bug.cgi?id=132527#c6

Again, I think this is a nice comment for the ChangeLog, but I wouldn't put it
here since the reason is quite simple anyway: The WebKit1 versions of the GTK
and EFL ports (the only ones using ATK) are no longer maintained upstream

> Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h:261
> -    
> +

Don't include this change (it's not related)

>
Tools/WebKitTestRunner/InjectedBundle/Bindings/AccessibilityUIElement.idl:-203
> -    

Ditto.

>
Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.c
pp:43
> +WTF::Vector<guint> listenerIds;

Loved this change, but please keep the unsigned type instead of guint (we only
use GLib types for public interfaces)

>
Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.c
pp:143
> +	   int lastArgument = 1;
> +	   for (int i = 0; i < extraArgs.size(); i++)
> +	       arguments[lastArgument += 1] = extraArgs[i];

Instead of doing this to keep the value of lastArgument outside the loop and
use it later, you could use a while loop with only one iterator (e.g.
"currentArgument") and then use it later to know the last argument position
(e.g. currentArgument+1).

If, however, you still prefer to use the for loop, please split that statement
in too so you don't use the 'lastArgument += 1' as the index for the array
(that's not clear)

>
Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.c
pp:-139
> -

I think you can keep this line :)

>
Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.c
pp:269
> +    for (const char** signalName = signalNames; *signalName; signalName++) {


Thanks for doing this refactoring :)

>
Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.c
pp:273
> +	       String message = String::format("atk_add_global_event_listener
failed for signal %s\n",
> +		   *signalName);

One line only