[webkit-reviews] review denied: [Bug 120421] [GTK] Missing WTR AccessibilityUIElement::addNotificationListener implementation : [Attachment 211366] Patch

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <mario at webkit.org> has denied Denis Nomiyama (dnomi)
<d.nomiyama at samsung.com>'s request for review:
Bug 120421: [GTK] Missing WTR AccessibilityUIElement::addNotificationListener
implementation
https://bugs.webkit.org/show_bug.cgi?id=120421

Attachment 211366: Patch
https://bugs.webkit.org/attachment.cgi?id=211366&action=review

------- Additional Comments from Mario Sanchez Prada <mario at webkit.org>
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.c
pp: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.c
pp: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.c
pp: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.c
pp: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.c
pp:166
> +#if PLATFORM(GTK)

Same consideration here about EFL

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

And here

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

Nit. I would probably remove this blank line.