[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
https://bugs.webkit.org/show_bug.cgi?id=132527
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
> -
Ditto.
> 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