Bug 119883 - [GTK] Missing DRT AccessibilityUIElement::addNotificationListener implementation
Summary: [GTK] Missing DRT AccessibilityUIElement::addNotificationListener implementation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 98350
  Show dependency treegraph
 
Reported: 2013-08-16 02:17 PDT by Denis Nomiyama (dnomi)
Modified: 2013-08-27 10:15 PDT (History)
15 users (show)

See Also:


Attachments
Patch (9.51 KB, patch)
2013-08-19 08:29 PDT, Denis Nomiyama (dnomi)
no flags Details | Formatted Diff | Diff
Patch (12.04 KB, patch)
2013-08-21 09:39 PDT, Denis Nomiyama (dnomi)
no flags Details | Formatted Diff | Diff
Patch (22.05 KB, patch)
2013-08-22 10:45 PDT, Denis Nomiyama (dnomi)
no flags Details | Formatted Diff | Diff
Patch (22.12 KB, patch)
2013-08-22 10:59 PDT, Denis Nomiyama (dnomi)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (511.03 KB, application/zip)
2013-08-22 13:45 PDT, Build Bot
no flags Details
Patch (21.38 KB, patch)
2013-08-23 10:35 PDT, Denis Nomiyama (dnomi)
no flags Details | Formatted Diff | Diff
Patch (21.50 KB, patch)
2013-08-27 08:42 PDT, Denis Nomiyama (dnomi)
no flags Details | Formatted Diff | Diff
Patch (21.44 KB, patch)
2013-08-27 09:30 PDT, Denis Nomiyama (dnomi)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Denis Nomiyama (dnomi) 2013-08-16 02:17:46 PDT
AccessibilityUIElement in the GTK+ port does not support event notification listeners (addNotificationListener).
Comment 1 Denis Nomiyama (dnomi) 2013-08-19 08:29:22 PDT
Created attachment 209090 [details]
Patch
Comment 2 EFL EWS Bot 2013-08-19 08:32:42 PDT
Comment on attachment 209090 [details]
Patch

Attachment 209090 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1510540
Comment 3 Denis Nomiyama (dnomi) 2013-08-21 09:39:15 PDT
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.
Comment 4 chris fleizach 2013-08-21 09:52:49 PDT
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?
Comment 5 Denis Nomiyama (dnomi) 2013-08-21 10:31:08 PDT
(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);
Comment 6 chris fleizach 2013-08-21 10:45:12 PDT
You might also look at the Mac implementation in AccessibilityNotificationHandler which seems pretty straight forward to me

thanks
Comment 7 Denis Nomiyama (dnomi) 2013-08-22 10:45:48 PDT
Created attachment 209378 [details]
Patch

Modified the patch to use AccessibilityNotificationHandler
Comment 8 EFL EWS Bot 2013-08-22 10:49:45 PDT
Comment on attachment 209378 [details]
Patch

Attachment 209378 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1516980
Comment 9 Denis Nomiyama (dnomi) 2013-08-22 10:59:19 PDT
Created attachment 209379 [details]
Patch

Fixed minor issue for compiling it on EFL
Comment 10 chris fleizach 2013-08-22 11:39:35 PDT
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
Comment 11 Build Bot 2013-08-22 13:45:01 PDT
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
Comment 12 Build Bot 2013-08-22 13:45:03 PDT
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
Comment 13 Denis Nomiyama (dnomi) 2013-08-23 10:31:07 PDT
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
Comment 14 Denis Nomiyama (dnomi) 2013-08-23 10:35:15 PDT
Created attachment 209474 [details]
Patch

Fixed previous patch according to review comments.
Comment 15 chris fleizach 2013-08-23 11:25:05 PDT
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
Comment 16 EFL EWS Bot 2013-08-23 12:06:35 PDT
Comment on attachment 209474 [details]
Patch

Attachment 209474 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1546354
Comment 17 Denis Nomiyama (dnomi) 2013-08-27 08:32:38 PDT
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?
Comment 18 Denis Nomiyama (dnomi) 2013-08-27 08:42:07 PDT
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.
Comment 19 chris fleizach 2013-08-27 09:11:08 PDT
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
Comment 20 Denis Nomiyama (dnomi) 2013-08-27 09:30:55 PDT
Created attachment 209782 [details]
Patch
Comment 21 Denis Nomiyama (dnomi) 2013-08-27 09:34:11 PDT
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?
Comment 22 WebKit Commit Bot 2013-08-27 10:15:00 PDT
Comment on attachment 209782 [details]
Patch

Clearing flags on attachment: 209782

Committed r154697: <http://trac.webkit.org/changeset/154697>
Comment 23 WebKit Commit Bot 2013-08-27 10:15:05 PDT
All reviewed patches have been landed.  Closing bug.