Bug 119883

Created attachment 209090 [details] Patch

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

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

(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);

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

Created attachment 209378 [details] Patch Modified the patch to use AccessibilityNotificationHandler

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

Created attachment 209379 [details] Patch Fixed minor issue for compiling it on EFL

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

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

Created attachment 209474 [details] Patch Fixed previous patch according to review comments.

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 on attachment 209474 [details] Patch Attachment 209474 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/1546354

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?

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

Created attachment 209782 [details] Patch

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 on attachment 209782 [details] Patch Clearing flags on attachment: 209782 Committed r154697: <http://trac.webkit.org/changeset/154697>

All reviewed patches have been landed. Closing bug.