[Webkit-unassigned] [Bug 120421] [GTK] Missing WTR AccessibilityUIElement::addNotificationListener implementation

[bugzilla-daemon at webkit.org]

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


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

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




--- Comment #8 from Mario Sanchez Prada <mario at webkit.org>  2013-09-12 03:30:18 PST ---
(From update of attachment 211366)
View in context: https://bugs.webkit.org/attachment.cgi?id=211366&action=review

Thanks for the patch Denis. It looks good already but still needs some more work. See my review below...

> Tools/ChangeLog:37
> +        * WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:
> +        Added.

I guess in cases like this is better to leave the "Added." in the previous line, even it's long, since it makes it more readable.

> Tools/WebKitTestRunner/CMakeLists.txt:27
> +    ${WEBKIT_TESTRUNNER_DIR}/InjectedBundle/atk
>      ${WEBKIT_TESTRUNNER_DIR}/InjectedBundle/Bindings

Wrong alphabetical order. I believe uppercase 'B' should come before lowercase 'a'

> Tools/WebKitTestRunner/GNUmakefile.am:137
> +	-I$(srcdir)/Tools/WebKitTestRunner/InjectedBundle/atk \

Same here.

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:26
> +

You don't need this extra line. See "#include Statements" in http://www.webkit.org/coding/coding-style.html

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:49
> +static guint stateChangeListenerId = 0;
> +static guint focusEventListenerId = 0;
> +static guint activeDescendantChangedListenerId = 0;
> +static guint childrenChangedListenerId = 0;
> +static guint propertyChangedListenerId = 0;
> +static guint visibleDataChangedListenerId = 0;
> +static NotificationHandlersMap notificationHandlers;
> +static AccessibilityNotificationHandler* globalNotificationHandler = 0;
> +static bool loggingAccessibilityEvents = false;
> +
> +static void printAccessibilityEvent(AtkObject* accessible, const gchar* signalName, const gchar* signalValue)

This refactoring might be a good opportunity to move into using unnamed namespaces for this static declarations, which are the recommended alternative in C++

Also, it might be a good opportunity too to move from glib specific types (e.g. guint) into more general types (e.g. unsigned) for things that are not going to be part of public APIs

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:59
> +    const gchar* objectName = atk_object_get_name(accessible);

Following the lead of the comment above, s/gchar/char. Similar considerations in the rest of the code after this point

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:107
> +#if PLATFORM(GTK)

Couldn't this be #if PLATFORM(GTK) || PLATFORM(EFL) ?

I guess the answer is "probably yes" but that it's perhaps better to leave it out of this patch because of consistency with what we did for DRT, since we can't test it really in EFL. However, I think it might be worth to let EFL guys know about it because that bit might help them pass some additional tests as well.

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:166
> +#if PLATFORM(GTK)

Same consideration here about EFL

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityNotificationHandlerAtk.cpp:197
> +#if PLATFORM(GTK)

And here

> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:1135
> +

Nit. I would probably remove this blank line.

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