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

[bugzilla-daemon at webkit.org]

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





--- Comment #9 from Denis Nomiyama (dnomi) <d.nomiyama at samsung.com>  2013-09-12 05:38:15 PST ---
(From update of attachment 211366)
View in context: https://bugs.webkit.org/attachment.cgi?id=211366&action=review

Many thanks for the review. I'll make the changes.
I have done some extra tests and found a problem with disconnectAccessibilityCallbacks(), which is not returning a value (true) at the end.
Another problem was that connectAccessibilityCallbacks() assumes that notification handlers are added to HashMap after the call. And I modified logAccessibilityEvents() and setNotificationFunctionCallback() to reflect this assumption.

>> Tools/ChangeLog:37
>> +        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.

Ok.

>> Tools/WebKitTestRunner/CMakeLists.txt:27
>>      ${WEBKIT_TESTRUNNER_DIR}/InjectedBundle/Bindings
> 
> Wrong alphabetical order. I believe uppercase 'B' should come before lowercase 'a'

Ops. I'll fix it.

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

Ok.

>> 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

Ok. I'll fix it.

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

Sure, I'll modify them.

>> 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.

Ok. Search for the respective bug report for EFL or will create one if not there yet.

>> Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp:1135
>> +
> 
> Nit. I would probably remove this blank line.

Ok. I'll remove it.

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