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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri May 16 04:21:36 PDT 2014


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

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

--- Comment #11 from Mario Sanchez Prada <mario at webkit.org>  2014-05-16 04:21:57 PST ---
(From update of attachment 231455)
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/testatk.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
> -    


> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp: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.cpp: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.cpp:-139
> -

I think you can keep this line :)

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

Thanks for doing this refactoring :)

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

One line only

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

More information about the webkit-unassigned mailing list