Bug 119883 – [GTK] Missing DRT AccessibilityUIElement::addNotificationListener implementation

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Denis Nomiyama (dnomi)]

AccessibilityUIElement in the GTK+ port does not support event notification listeners (addNotificationListener).

[Denis Nomiyama (dnomi)]

Created attachment 209090 [details]
Patch

[EFL EWS Bot]

Comment on attachment 209090 [details]
Patch

Attachment 209090 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1510540

[Denis Nomiyama (dnomi)]

Created attachment 209276 [details]
Patch

Guarded the code with PLATFORM(GTK) to allow other ports (e.g. EFL) to compile this code. Also unskipped a layout test to verify this code.

[chris fleizach]

Comment on attachment 209276 [details]
Patch

We already have nearly this same functionality in AccessibilityController.

    // Global notification listener, captures notifications on any object.
    bool addNotificationListener(JSObjectRef functionCallback);
    void removeNotificationListener();

Can we re-use that model instead of adding the same thing to the AXUIElement?

[Denis Nomiyama (dnomi)]

(In reply to comment #4)
> (From update of attachment 209276 [details])
> We already have nearly this same functionality in AccessibilityController.
> 
>     // Global notification listener, captures notifications on any object.
>     bool addNotificationListener(JSObjectRef functionCallback);
>     void removeNotificationListener();
> 
> Can we re-use that model instead of adding the same thing to the AXUIElement?

Many thanks for the prompt reply.
I will give it a try, although it seems a bit tricky to implement it in the GTK+ code to access AccessibilityController from AccessibilityUIElement as done in the Win code:

sharedFrameLoadDelegate->accessibilityController()->winAddNotificationListener(m_element, functionCallback);

[chris fleizach]

You might also look at the Mac implementation in AccessibilityNotificationHandler which seems pretty straight forward to me

thanks

[Denis Nomiyama (dnomi)]

Created attachment 209378 [details]
Patch

Modified the patch to use AccessibilityNotificationHandler

[EFL EWS Bot]

Comment on attachment 209378 [details]
Patch

Attachment 209378 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1516980

[Denis Nomiyama (dnomi)]

Created attachment 209379 [details]
Patch

Fixed minor issue for compiling it on EFL

[chris fleizach]

Comment on attachment 209378 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209378&action=review

> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:118
> +#if PLATFORM(GTK)

does this have to be GTK only? we're already in the ATK file

> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:121
> +            // The global notification handler is stored with key 0.

can you #define the GlobalNotificationKey so you don't have to comment about key == 0 everywhere

> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:232
> +void removeAccessibilityNotificationHandler(AccessibilityNotificationHandler *notificationHandler)

* in wrong place

> Tools/DumpRenderTree/atk/AccessibilityNotificationHandlerAtk.cpp:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

seems like this copyright is erroneous

> Tools/DumpRenderTree/atk/AccessibilityNotificationHandlerAtk.cpp:59
> +    if (!notificationFunctionCallback)

seems like if you pass 0 in, it should remove the current function callback, and then return

> Tools/DumpRenderTree/atk/AccessibilityNotificationHandlerAtk.h:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

seems like this copyright is erroneous

> Tools/DumpRenderTree/atk/AccessibilityNotificationHandlerAtk.h:45
> +    AtkObject* platformElement(void) { return m_platformElement; }

this should be const

> Tools/DumpRenderTree/atk/AccessibilityNotificationHandlerAtk.h:47
> +    JSObjectRef notificationFunctionCallback(void) { return m_notificationFunctionCallback; }

ditto about const

> Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp:1060
> +#else

you don't really need the #else here at all

but same comment as above, can we just enable this for all ATK

[Build Bot]

Comment on attachment 209379 [details]
Patch

Attachment 209379 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1545224

New failing tests:
fast/workers/termination-with-port-messages.html

[Build Bot]

Created attachment 209388 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.4

[Denis Nomiyama (dnomi)]

Comment on attachment 209378 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209378&action=review

>> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:118
>> +#if PLATFORM(GTK)
> 
> does this have to be GTK only? we're already in the ATK file

This ATK file is also used by EFL, which doesn't share the same API to get the JS global context.
I can remove #if GTK from other places around AccessibilityNotificationHandler because they won't break the EFL build.
But unfortunately I can't implement this one as I don't have a EFL env to test it. So on EFL the mechanism will be almost ready and only missing the final call to the JS callback.

>> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:121
>> +            // The global notification handler is stored with key 0.
> 
> can you #define the GlobalNotificationKey so you don't have to comment about key == 0 everywhere

Ok. I will add it.

>> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:232
>> +void removeAccessibilityNotificationHandler(AccessibilityNotificationHandler *notificationHandler)
> 
> * in wrong place

Ops, sorry.

>> Tools/DumpRenderTree/atk/AccessibilityNotificationHandlerAtk.cpp:2
>> + * Copyright (C) 2011 Google Inc. All rights reserved.
> 
> seems like this copyright is erroneous

Ok. I will fix it.

>> Tools/DumpRenderTree/atk/AccessibilityNotificationHandlerAtk.cpp:59
>> +    if (!notificationFunctionCallback)
> 
> seems like if you pass 0 in, it should remove the current function callback, and then return

Good point. I will do it.

>> Tools/DumpRenderTree/atk/AccessibilityNotificationHandlerAtk.h:2
>> + * Copyright (C) 2011 Google Inc. All rights reserved.
> 
> seems like this copyright is erroneous

Ok. I will fix it.

>> Tools/DumpRenderTree/atk/AccessibilityNotificationHandlerAtk.h:45
>> +    AtkObject* platformElement(void) { return m_platformElement; }
> 
> this should be const

Ok. I will fix it.

>> Tools/DumpRenderTree/atk/AccessibilityNotificationHandlerAtk.h:47
>> +    JSObjectRef notificationFunctionCallback(void) { return m_notificationFunctionCallback; }
> 
> ditto about const

Ok

[Denis Nomiyama (dnomi)]

Created attachment 209474 [details]
Patch

Fixed previous patch according to review comments.

[chris fleizach]

Comment on attachment 209474 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209474&action=review

almost there!

> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:119
> +    if (notificationName.length())

need brackets for this if and then for, because contents in the middle are longer than a line

> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:121
> +            if ((it->key == accessible) || (it->key == GlobalNotificationKey)) {

these params around each term is not necessary

> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:234
> +    for (HashMap<PlatformUIElement, AccessibilityNotificationHandler*>::iterator it = notificationHandlers.begin(); it != notificationHandlers.end(); ++it)

we need brackets for this for loop because the contents are more than one line

> Tools/DumpRenderTree/atk/AccessibilityNotificationHandlerAtk.h:39
> +    JSObjectRef m_notificationFunctionCallback;

this seems like it should be a RefPtr or something that retains this function

[EFL EWS Bot]

Comment on attachment 209474 [details]
Patch

Attachment 209474 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1546354

[Denis Nomiyama (dnomi)]

Comment on attachment 209474 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209474&action=review

>> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:119
>> +    if (notificationName.length())
> 
> need brackets for this if and then for, because contents in the middle are longer than a line

Ok, I'll add them.

>> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:121
>> +            if ((it->key == accessible) || (it->key == GlobalNotificationKey)) {
> 
> these params around each term is not necessary

Ok, I'll remove them.

>> Tools/DumpRenderTree/atk/AccessibilityCallbacksAtk.cpp:234
>> +    for (HashMap<PlatformUIElement, AccessibilityNotificationHandler*>::iterator it = notificationHandlers.begin(); it != notificationHandlers.end(); ++it)
> 
> we need brackets for this for loop because the contents are more than one line

Ok.

>> Tools/DumpRenderTree/atk/AccessibilityNotificationHandlerAtk.h:39
>> +    JSObjectRef m_notificationFunctionCallback;
> 
> this seems like it should be a RefPtr or something that retains this function

I have tried to find a way to make it RefPtr but it didn't go well. JSObjectRef has a forward declaration from OpaqueJSValue, and RefPtr requires the type definition beforehand.
I have also searched for other places where JSObjectRef is used, and unfortunately I couldn't find any reference of how I could implement this. Would you have any recommendation?

[Denis Nomiyama (dnomi)]

Created attachment 209779 [details]
Patch

Modified patch according to review comments. Plus, improved AccessibilityCallbacksAtk.cpp to allow ports other than GTK to compile this code.

[chris fleizach]

Comment on attachment 209779 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209779&action=review

> Tools/DumpRenderTree/atk/AccessibilityNotificationHandlerAtk.cpp:21
> +

It looks like there shouldn't be a newline between config and the corresponding .h file

[Denis Nomiyama (dnomi)]

Created attachment 209782 [details]
Patch

[Denis Nomiyama (dnomi)]

Comment on attachment 209779 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209779&action=review

>> Tools/DumpRenderTree/atk/AccessibilityNotificationHandlerAtk.cpp:21
>> +
> 
> It looks like there shouldn't be a newline between config and the corresponding .h file

Many thanks for the reviews. I have fixed the newline and re-submitted the patch with cq?

[WebKit Commit Bot]

Comment on attachment 209782 [details]
Patch

Clearing flags on attachment: 209782

Committed r154697: <http://trac.webkit.org/changeset/154697>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.