Bug 109937 – [EFL][WK2] Additional bits of having Accessibility in WebKit-EFL.

• Component: WebKit EFL
• Status: RESOLVED WONTFIX

[Krzysztof Czech]

Additional bits of having Accessibility in WebKit-EFL.

[Krzysztof Czech]

In order to expose information from WebKit (server) to clients applications (Assistive Technologies example. GNOME Orca) DBus is used. AT-SPI2 is something that mediates between server and client in terms of passing dbus messages. WebKit's events before land in AT-SPI2 should be translated into dbus messages by ATK-BRIDGE module, which should be properly loaded. In GNOME it's out of the box, in EFL it's not.
Generally this should be done transparently without WebKit support, but till EFL does not do this, it may be the only way.

I'm proposing a patch that loads ATK-BRIDGE and adds implementation of AtkUtilClass that registers listeners for specific ATK events and creates 
top level accessible object for WebKit.

I would like to discuss this approach.
I'm looking forward to hearing your opinion about this.

[Krzysztof Czech]

Created attachment 188583 [details]
Patch

[Mario Sanchez Prada]

(In reply to comment #1)
> In order to expose information from WebKit (server) to clients applications
> (Assistive Technologies example. GNOME Orca) DBus is used. AT-SPI2 is something 
> that mediates between server and client in terms of passing dbus messages. 
>
> WebKit's events before land in AT-SPI2 should be translated into dbus messages 
> by ATK-BRIDGE module, which should be properly loaded. In GNOME it's out of the 
> box, in EFL it's not.

Right.
 
> Generally this should be done transparently without WebKit support, but till 
> EFL does not do this, it may be the only way.

You mean forcing to load the ATK bridge from WebKitEfl, right? If so, and providing that for EFL the "desktop" does not do the job, then I guess you're pretty much stuck with two options: either you do it in WebKit or you do it in every single application using WebKit.

In this regard, I wonder whether it would certainly be more "clean" to do it the apps (or even in the EFL's widget toolkit, if that was possible), so you would not have to mess with AtkUtil in WebKit at all.

Still, I see a problem with that approach for the case WebKit2, since you still need to have the ATK bridge loaded for the Web process, so it can expose its a11y tree for the DOM and "connect it" to the one hanging from the UI process, and that won't happen automatically if you do that in the application or the EFL widgets toolkit, I guess.

So, you would still need to force the load of the ATK bridge module in the WebProcess but perhaps that would be enough and you still would avoid having to mess with AtkUtil? In any case, I'm not 100% sure about this, the "AtkUtil expert" here is Alejandro Pinheiro, so I guess it's better if he commented more on that idea.

> I'm proposing a patch that loads ATK-BRIDGE and adds implementation of 
> AtkUtilClass that registers listeners for specific ATK events

I would really love to hear Pinheiro's opinon on this. As I said, he's the AtkUtil expert here :)

> and creates top level accessible object for WebKit.

I have a doubt about this "top level accessible object":

As far as I can see from the patch, it seems to be just a dummy AtkObject that you're returning from the implementation of AtkUtil, but I can't see the relationship between that object and the a11y hierarchy exposed by the WebProcess: an AtkPlug object (which is the root Atk object in that process) associated to the WebPage object where the a11y hierarchy of Atk objects wrapping the DOM world "hangs" from, and which serves as the "bridge" with the UI process by means of the AtkSocket object that is implemented up there, as the associated a11y object for the WebView widget.

So, I really wonder why you want to return this dummy root object, which isolated from the world, instead of plainly returning that AtkPlug object, which is also a AtkObject that connects you to the a11y hierarchy wrapping the DOM.

> I would like to discuss this approach.
> I'm looking forward to hearing your opinion about this.

Just trying to add my 2 cents here. Please forgive me in advance if what I'm saying makes no sense at all :)

[WebKit Review Bot]

Attachment 188583 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/PlatformEfl.cmake', u'Source/WebKit2/WebProcess/WebPage/efl/WebPageAccessibilityRootObject.cpp', u'Source/WebKit2/WebProcess/WebPage/efl/WebPageAccessibilityRootObject.h', u'Source/WebKit2/WebProcess/WebPage/efl/WebPageAccessibilityUtil.cpp', u'Source/WebKit2/WebProcess/WebPage/efl/WebPageAccessibilityUtil.h', u'Source/WebKit2/WebProcess/efl/WebProcessMainEfl.cpp']" exit_code: 1
Source/WebKit2/WebProcess/WebPage/efl/WebPageAccessibilityRootObject.h:28:  web_page_accessibility_root_object_get_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit2/WebProcess/WebPage/efl/WebPageAccessibilityRootObject.cpp:33:  web_page_accessibility_root_object_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebKit2/WebProcess/WebPage/efl/WebPageAccessibilityRootObject.cpp:37:  web_page_accessibility_root_object_class_init is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 3 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Krzysztof Czech]

(In reply to comment #3)
> (In reply to comment #1)
> > In order to expose information from WebKit (server) to clients applications
> > (Assistive Technologies example. GNOME Orca) DBus is used. AT-SPI2 is something 
> > that mediates between server and client in terms of passing dbus messages. 
> >
> > WebKit's events before land in AT-SPI2 should be translated into dbus messages 
> > by ATK-BRIDGE module, which should be properly loaded. In GNOME it's out of the 
> > box, in EFL it's not.
> 
> Right.
> 
> > Generally this should be done transparently without WebKit support, but till 
> > EFL does not do this, it may be the only way.
> 
> You mean forcing to load the ATK bridge from WebKitEfl, right? If so, and providing that for EFL the "desktop" does not do the job, then I guess you're pretty much stuck with two options: either you do it in WebKit or you do it in every single application using WebKit.

Thanks Mario for your reply. Frankly speaking, I'm having the same concerns. Generally my intention was to have something that can be easily tested with MiniBrowser. I know this solution is not complete in terms of the whole Accessibility flow (lack of EFL, lack of UIprocess support, loading ATK bridge). I wanted to expose only WebProcess part.

You are right, the idea is to load ATK bridge from WebKitEFL since nor EFL or any single EFL's application do this.
> 
> In this regard, I wonder whether it would certainly be more "clean" to do it the apps (or even in the EFL's widget toolkit, if that was possible), so you would not have to mess with AtkUtil in WebKit at all.
This would the best scenario, when initialization is done transparently without WebKit knowledge. You are right EFL's widget toolkit should do this as GTK does. Unfortunately this is not happen in EFL.
> 
> Still, I see a problem with that approach for the case WebKit2, since you still need to have the ATK bridge loaded for the Web process, so it can expose its a11y tree for the DOM and "connect it" to the one hanging from the UI process, and that won't happen automatically if you do that in the application or the EFL widgets toolkit, I guess.

Generally, my intention was to load ATK bridge from WebProcess. While testing, I could see ATK bridge is linked to WebProcess. It looked a11y tree for DOM was properly exposed. GNOME Orca was able to read Web's content.
> 
> So, you would still need to force the load of the ATK bridge module in the WebProcess but perhaps that would be enough and you still would avoid having to mess with AtkUtil? In any case, I'm not 100% sure about this, the "AtkUtil expert" here is Alejandro Pinheiro, so I guess it's better if he commented more on that idea.

Would be great to hear his opinion.
> 
> > I'm proposing a patch that loads ATK-BRIDGE and adds implementation of 
> > AtkUtilClass that registers listeners for specific ATK events
> 
> I would really love to hear Pinheiro's opinon on this. As I said, he's the AtkUtil expert here :)
> 
> > and creates top level accessible object for WebKit.
> 
> I have a doubt about this "top level accessible object":
> 
> As far as I can see from the patch, it seems to be just a dummy AtkObject that you're returning from the implementation of AtkUtil, but I can't see the relationship between that object and the a11y hierarchy exposed by the WebProcess: an AtkPlug object (which is the root Atk object in that process) associated to the WebPage object where the a11y hierarchy of Atk objects wrapping the DOM world "hangs" from, and which serves as the "bridge" with the UI process by means of the AtkSocket object that is implemented up there, as the associated a11y object for the WebView widget.
> 
> So, I really wonder why you want to return this dummy root object, which isolated from the world, instead of plainly returning that AtkPlug object, which is also a AtkObject that connects you to the a11y hierarchy wrapping the DOM.

Generally, I also thought about it, that the good candidate is one from WebProcess. The thing is, it's a AtkPlug type. ATK bridge, while initialization is being done checks whether root is AtkPlug. If it is, it omits creation of dbus address line, where the server will listen and where clients will connect to. I hope this is a thing and that's way I provided this dummy object of type ATK_TYPE_OBJECT
> 
> > I would like to discuss this approach.
> > I'm looking forward to hearing your opinion about this.
> 
> Just trying to add my 2 cents here. Please forgive me in advance if what I'm saying makes no sense at all :)

[Mario Sanchez Prada]

(In reply to comment #5)
> [...]
> I wanted to expose only WebProcess part.

I see.

> [...]
> Generally, my intention was to load ATK bridge from WebProcess. While 
> testing, I could see ATK bridge is linked to WebProcess. It looked a11y tree 
> for DOM was properly exposed. GNOME Orca was able to read Web's content.

Yes, that works because the Web process is exposed on its own to AT-SPI, but you miss the "connection" with the UI Process and so Orca won't be able to know that such a web process actually belongs to a web browser, a mail client... or whatever.

That's why the AtkSocket <-> AtkPlug thing is important, I believe.

> > [...]
> > So, I really wonder why you want to return this dummy root object, which 
> > isolated from the world, instead of plainly returning that AtkPlug object, 
> > which is also a AtkObject that connects you to the a11y hierarchy wrapping 
> > the DOM.
> 
> Generally, I also thought about it, that the good candidate is one from 
> WebProcess. The thing is, it's a AtkPlug type. ATK bridge, while 
> initialization is being done checks whether root is AtkPlug. If it is, it 
> omits creation of dbus address line, where the server will listen and where 
> clients will connect to. I hope this is a thing and that's way I provided 
> this dummy object of type ATK_TYPE_OBJECT

I see. It's true that the AtkPlug/Socket machinery means specific things to be done in the ATK bridge (e.g. implementing socket_embed() and get_plug_id()), so I'm not surprised of this, even though I didn't realized until now :)

What I still wonder is whether it would not be better, or just more convenient for you, to avoid this dummy object and use instead the first and only child of this AtkPlug object, which certainly wraps WebCore's AccessibilityObject associated to the DOM's root.

If you think of AtkPlug as just a proxy object to communicate with the UI process, then I would say skipping that proxy and just using its only child could make a lot of sense, and should work fine for you anyway I think.

[Krzysztof Czech]

Created attachment 192177 [details]
Patch

[Krzysztof Czech]

(In reply to comment #6)
> (In reply to comment #5)
> > [...]
> > I wanted to expose only WebProcess part.
> 
> I see.
> 
> > [...]
> > Generally, my intention was to load ATK bridge from WebProcess. While 
> > testing, I could see ATK bridge is linked to WebProcess. It looked a11y tree 
> > for DOM was properly exposed. GNOME Orca was able to read Web's content.
> 
> Yes, that works because the Web process is exposed on its own to AT-SPI, but you miss the "connection" with the UI Process and so Orca won't be able to know that such a web process actually belongs to a web browser, a mail client... or whatever.
> 
> That's why the AtkSocket <-> AtkPlug thing is important, I believe.
> 
> > > [...]
> > > So, I really wonder why you want to return this dummy root object, which 
> > > isolated from the world, instead of plainly returning that AtkPlug object, 
> > > which is also a AtkObject that connects you to the a11y hierarchy wrapping 
> > > the DOM.
> > 
> > Generally, I also thought about it, that the good candidate is one from 
> > WebProcess. The thing is, it's a AtkPlug type. ATK bridge, while 
> > initialization is being done checks whether root is AtkPlug. If it is, it 
> > omits creation of dbus address line, where the server will listen and where 
> > clients will connect to. I hope this is a thing and that's way I provided 
> > this dummy object of type ATK_TYPE_OBJECT
> 
> I see. It's true that the AtkPlug/Socket machinery means specific things to be done in the ATK bridge (e.g. implementing socket_embed() and get_plug_id()), so I'm not surprised of this, even though I didn't realized until now :)
> 
> What I still wonder is whether it would not be better, or just more convenient for you, to avoid this dummy object and use instead the first and only child of this AtkPlug object, which certainly wraps WebCore's AccessibilityObject associated to the DOM's root.
> 
> If you think of AtkPlug as just a proxy object to communicate with the UI process, then I would say skipping that proxy and just using its only child could make a lot of sense, and should work fine for you anyway I think.

I agree, this dummy object provides no useful information and it can be avoided. I followed your suggestion and it really makes sense of using a child of the AtkPlug object. As you said, it's not detached from the WebKit's context and it represents a11y hierarchy.

[Mario Sanchez Prada]

Comment on attachment 192177 [details]
Patch

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

Thanks for considering my comments, I think now I understand the patch better so I have commented some things below to help you improve it. Still, please accept my apologies in advance if I mentioned something that makes no sense at all. I was really willing to try your patch here before commenting to make more valuable comments, but something went nuts in my machine and I guessed it's better to provide you some feedback now that maybe not feedback at all until the next week.

Also, I'd like to stress the fact that we really need to get Pinheiro's opinion in here. Tha AtkUtil stuff keeps not being 100% clear to me yet

> Source/WebKit2/ChangeLog:26
> +        * WebProcess/WebPage/efl/WebPageAccessibilityUtil.cpp: Added.
> +        (WebPageAccessibilityUtilListenerInfo):
> +        (WebPageAccessibilityUtil::WebPageAccessibilityUtil):
> +        (WebPageAccessibilityUtil::~WebPageAccessibilityUtil):
> +        (WebPageAccessibilityUtil::create):
> +        (WebPageAccessibilityUtil::addListener):
> +        (WebPageAccessibilityUtil::webPageAccessibilityUtilAddGlobalEventListener):
> +        (WebPageAccessibilityUtil::webPageAccessibilityUtilRemoveGlobalEventListener):
> +        (WebPageAccessibilityUtil::webPageAccessibilityUtilGetRoot):
> +        (WebPageAccessibilityUtil::webPageAccessibilityUtilRootObjectSet):
> +        (WebPageAccessibilityUtil::webPageAccessibilityUtilLoad):
> +        (WebPageAccessibilityUtil::webPageAccessibilityUtilInit):
> +        * WebProcess/WebPage/efl/WebPageAccessibilityUtil.h: Added.

Why not making WebPageAccessibilityUtil a GObject class implementing AtkUtil (which is a GObject too?) instead of using a C++ class.

I feel like you are over complicating things too much by doing it this way and that going for a GObject-style thing here, even if I recognise it implies some C boilerplate code, would make things more simple.

Also, I'm not 100% sure (again, Pinheiro is the expert in AtkUtil), but I believe an implementation of AtkUtil is something _global_ to the process, not local to every WebPage as it seems to suggest this code, and so get_root() should return always the same and only object regardless of when it's invoked.

The problem here is that, as far I can understand now, we don't have that "clear root AtkObject" as in GTK (in GTK+ atk_get_root() returns the AtkObject associated to the GTK Application) but "many root objects" instead, one per WebPage. And we can't just create a global AtkObject being the parent of all the topmost AtkObject's (the AtkPlug's) in the WebProcess because that would break the implementation of AtkPlug in the ATK bridge, which expects atk_object_get_parent() to return NULL over the AtkPlug object.

So, perhaps that's why you added that dummy object in the first place, I don't know... but now I think more of this and see this problem, I wonder whether it was not a good idea after all, or even if this thing of having a "dynamic root object" will work fine.

Pinheiro please, drop some light here! :)

> Source/WebKit2/WebProcess/WebPage/efl/WebPageAccessibilityUtil.cpp:35
> +AtkObject* WebPageAccessibilityUtil::s_rootObject = 0;
> +GHashTable* WebPageAccessibilityUtil::s_listenerList = 0;

You probably want to use GRefPtr instead of raw pointers, so memory management is automatic and do not need to manually unref/free memory.

Additionally, if this object is meant to be a global one, you could probably better implement a Singleton and put these two variables in the private section of the class.

> Source/WebKit2/WebProcess/WebPage/efl/WebPageAccessibilityUtil.cpp:46
> +    if (s_rootObject)
> +        g_object_unref(s_rootObject);

This code could be avoided if you move s_rootObject to a GRefPtr<AtkObject>

> Source/WebKit2/WebProcess/WebPage/efl/WebPageAccessibilityUtil.cpp:86
> +    gchar** splitString = g_strsplit(eventType, ":", 3);

You can use a GRefPtr<GPtrArray> for splitString too, and forget about manually handling gchar** and calling to g_strfreev later

You can take a look to Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp for an example on how to do something similar.

> Source/WebKit2/WebProcess/WebPage/efl/WebPageAccessibilityUtil.cpp:114
> +    return s_rootObject;

If you use GRefPtr<AtkObject>, you will return s_rootObject.get() here

> Source/WebKit2/WebProcess/WebPage/efl/WebPageAccessibilityUtil.cpp:123
> +    if (s_rootObject)
> +        g_object_unref(s_rootObject);

Again, this should not be needed

> Source/WebKit2/WebProcess/WebPage/efl/WebPageAccessibilityUtil.cpp:128
> +    if (s_rootObject)
> +        g_object_ref(s_rootObject);

...nor this

> Source/WebKit2/WebProcess/WebPage/efl/WebPageAccessibilityUtil.cpp:137
> +    Eina_Module* module = eina_module_new(modulePath);

I don't know EFL, but I feel like you are leaking something here (I don't see where you free this new memory).

Probably another candidate for (G)RefPtr (G)OwnPtr? (No idea whether Eina_Module is a ref counted object, a GObject or something else)

> Source/WebKit2/WebProcess/WebPage/efl/WebPageAccessibilityUtil.cpp:162
> +    webPageAccessibilityUtilRootObjectSet(root);

I believe you could replace this too lines with just:

  AtkObject* root = atk_object_ref_accessible_child(ATK_OBJECT(accessibilityObject), 0);
  if (s_rootObject != root)
      s_rootObject = adoptGRef(root);

...and remove the function webPageAccessibilityUtilRootObjectSet()

> Source/WebKit2/WebProcess/WebPage/efl/WebPageAccessibilityUtil.cpp:166
> +    static bool atkBridgeInitialized = 0;
> +    if (atkBridgeInitialized)
> +        return;

You would not need this if you implement a singleton. Well.. at least not here, even if you would need something similar of course :)

> Source/WebKit2/WebProcess/WebPage/efl/WebPageAccessibilityUtil.cpp:175
> +    AtkUtilClass* utilClass = ATK_UTIL_CLASS(g_type_class_ref(ATK_TYPE_UTIL));
> +    utilClass->add_global_event_listener = webPageAccessibilityUtilAddGlobalEventListener;
> +    utilClass->remove_global_event_listener = webPageAccessibilityUtilRemoveGlobalEventListener;
> +    utilClass->add_key_event_listener = 0;
> +    utilClass->remove_key_event_listener = 0;
> +    utilClass->get_root = webPageAccessibilityUtilGetRoot;
> +    utilClass->get_toolkit_name = 0;
> +    utilClass->get_toolkit_version = 0;

It's a bit unnatural to me to see this code in a C++ class. I really think you should go for proper GObject implementation here, unless there's something I'm missing specific to the EFL port.

> Source/WebKit2/WebProcess/WebPage/efl/WebPageAccessibilityUtil.cpp:176
> +    s_listenerList = g_hash_table_new_full(g_int_hash, g_int_equal, 0, g_free);

This hash table is never freed, hence is leaked. You will avoid it by using a GRefPtr for it, as suggested above.

> Source/WebKit2/WebProcess/WebPage/efl/WebPageAccessibilityUtil.h:36
> +    ~WebPageAccessibilityUtil();
> +
> +    static guint webPageAccessibilityUtilAddGlobalEventListener(GSignalEmissionHook, const gchar*);
> +    static void webPageAccessibilityUtilRemoveGlobalEventListener(guint);
> +    static AtkObject* webPageAccessibilityUtilGetRoot();
> +    static guint addListener(GSignalEmissionHook, const gchar*, const gchar*, const gchar*);

All these things will be normal private static functions private to the cpp file if you migrate to a regular GObject-like implementation

> Source/WebKit2/WebProcess/WebPage/efl/WebPageAccessibilityUtil.h:38
> +    void webPageAccessibilityUtilInit(WebPageAccessibilityObject*);

We really need to clarify this thing of WebPageAccessibilityUtil being a global object that it's initialized with Web pages (which are not global at all). I'm not 100% sure yet, but I feel like there is something wrong here.

Sorry for being a bit vague with this, but AtkUtil is something I never needed to deal with yet, Pinheiro is the expert (he did it for Ca11y and GTK, I think)

> Source/WebKit2/WebProcess/WebPage/efl/WebPageAccessibilityUtil.h:47
> +    WebPageAccessibilityUtil();
> +
> +    void webPageAccessibilityUtilLoad();
> +    void webPageAccessibilityUtilRootObjectSet(AtkObject*);
> +
> +    typedef void (*AtkBridgeInit)(void);
> +    typedef void (*AtkBridgeShutdown)(void);

All this would just be in the cpp file if you move to GObject.

> Source/WebKit2/WebProcess/WebPage/efl/WebPageAccessibilityUtil.h:52
> +    AtkBridgeInit m_atkBridgeInit;
> +    AtkBridgeShutdown m_atkBridgeShutdown;
> +
> +    static AtkObject* s_rootObject;
> +    static GHashTable* s_listenerList;

...and these other would be part of the private struct, again in the GObject file. About the static ones, that should not be a problem if the GObject is implemented as a Singleton

> Source/WebKit2/WebProcess/WebPage/efl/WebPageEfl.cpp:52
> +static OwnPtr<WebPageAccessibilityUtil> accessibilityUtilObject;

This would be a GRefPtr<WebPageAccessibilityUtil> after moving to GObject if you want to keep using the ref count. However, being a global object you can probably return just a WebPageAccessibilityUtil* and store it in the platformInitialize() function

[Krzysztof Czech]

(In reply to comment #9)
> (From update of attachment 192177 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192177&action=review
> 
> Thanks for considering my comments, I think now I understand the patch better so I have commented some things below to help you improve it. Still, please accept my apologies in advance if I mentioned something that makes no sense at all. I was really willing to try your patch here before commenting to make more valuable comments, but something went nuts in my machine and I guessed it's better to provide you some feedback now that maybe not feedback at all until the next week.
> 
> Also, I'd like to stress the fact that we really need to get Pinheiro's opinion in here. Tha AtkUtil stuff keeps not being 100% clear to me yet
> 
> > Source/WebKit2/ChangeLog:26
> > +        * WebProcess/WebPage/efl/WebPageAccessibilityUtil.cpp: Added.
> > +        (WebPageAccessibilityUtilListenerInfo):
> > +        (WebPageAccessibilityUtil::WebPageAccessibilityUtil):
> > +        (WebPageAccessibilityUtil::~WebPageAccessibilityUtil):
> > +        (WebPageAccessibilityUtil::create):
> > +        (WebPageAccessibilityUtil::addListener):
> > +        (WebPageAccessibilityUtil::webPageAccessibilityUtilAddGlobalEventListener):
> > +        (WebPageAccessibilityUtil::webPageAccessibilityUtilRemoveGlobalEventListener):
> > +        (WebPageAccessibilityUtil::webPageAccessibilityUtilGetRoot):
> > +        (WebPageAccessibilityUtil::webPageAccessibilityUtilRootObjectSet):
> > +        (WebPageAccessibilityUtil::webPageAccessibilityUtilLoad):
> > +        (WebPageAccessibilityUtil::webPageAccessibilityUtilInit):
> > +        * WebProcess/WebPage/efl/WebPageAccessibilityUtil.h: Added.
> 
> Why not making WebPageAccessibilityUtil a GObject class implementing AtkUtil (which is a GObject too?) instead of using a C++ class.
> 
> I feel like you are over complicating things too much by doing it this way and that going for a GObject-style thing here, even if I recognise it implies some C boilerplate code, would make things more simple.
> 
> Also, I'm not 100% sure (again, Pinheiro is the expert in AtkUtil), but I believe an implementation of AtkUtil is something _global_ to the process, not local to every WebPage as it seems to suggest this code, and so get_root() should return always the same and only object regardless of when it's invoked.
> 
You are perfectly right that AtkUtil is something global and WebKit should not even know about this. Would also be great if this was somewhere in EFL. I'm just trying to workaround this problem, that's why I'm trying to do it in WebKit.
Generally, more I'm getting into it, I'm closer to the conclusion that WebKit is not a good place for initializing ATK-BRIDGE stuff.

to workaround this problem
> The problem here is that, as far I can understand now, we don't have that "clear root AtkObject" as in GTK (in GTK+ atk_get_root() returns the AtkObject associated to the GTK Application) but "many root objects" instead, one per WebPage. And we can't just create a global AtkObject being the parent of all the topmost AtkObject's (the AtkPlug's) in the WebProcess because that would break the implementation of AtkPlug in the ATK bridge, which expects atk_object_get_parent() to return NULL over the AtkPlug object.
> 
> So, perhaps that's why you added that dummy object in the first place, I don't know... but now I think more of this and see this problem, I wonder whether it was not a good idea after all, or even if this thing of having a "dynamic root object" will work fine.
> 
> Pinheiro please, drop some light here! :)
> 
> > Source/WebKit2/WebProcess/WebPage/efl/WebPageAccessibilityUtil.cpp:35
> > +AtkObject* WebPageAccessibilityUtil::s_rootObject = 0;
> > +GHashTable* WebPageAccessibilityUtil::s_listenerList = 0;
> 
> You probably want to use GRefPtr instead of raw pointers, so memory management is automatic and do not need to manually unref/free memory.
> 
> Additionally, if this object is meant to be a global one, you could probably better implement a Singleton and put these two variables in the private section of the class.
> 
> > Source/WebKit2/WebProcess/WebPage/efl/WebPageAccessibilityUtil.cpp:46
> > +    if (s_rootObject)
> > +        g_object_unref(s_rootObject);
> 
> This code could be avoided if you move s_rootObject to a GRefPtr<AtkObject>
> 
> > Source/WebKit2/WebProcess/WebPage/efl/WebPageAccessibilityUtil.cpp:86
> > +    gchar** splitString = g_strsplit(eventType, ":", 3);
> 
> You can use a GRefPtr<GPtrArray> for splitString too, and forget about manually handling gchar** and calling to g_strfreev later
> 
> You can take a look to Source/WebKit2/UIProcess/API/gtk/WebKitFileChooserRequest.cpp for an example on how to do something similar.
> 
> > Source/WebKit2/WebProcess/WebPage/efl/WebPageAccessibilityUtil.cpp:114
> > +    return s_rootObject;
> 
> If you use GRefPtr<AtkObject>, you will return s_rootObject.get() here
> 
> > Source/WebKit2/WebProcess/WebPage/efl/WebPageAccessibilityUtil.cpp:123
> > +    if (s_rootObject)
> > +        g_object_unref(s_rootObject);
> 
> Again, this should not be needed
> 
> > Source/WebKit2/WebProcess/WebPage/efl/WebPageAccessibilityUtil.cpp:128
> > +    if (s_rootObject)
> > +        g_object_ref(s_rootObject);
> 
> ...nor this
> 
> > Source/WebKit2/WebProcess/WebPage/efl/WebPageAccessibilityUtil.cpp:137
> > +    Eina_Module* module = eina_module_new(modulePath);
> 
> I don't know EFL, but I feel like you are leaking something here (I don't see where you free this new memory).
> 
> Probably another candidate for (G)RefPtr (G)OwnPtr? (No idea whether Eina_Module is a ref counted object, a GObject or something else)
> 
> > Source/WebKit2/WebProcess/WebPage/efl/WebPageAccessibilityUtil.cpp:162
> > +    webPageAccessibilityUtilRootObjectSet(root);
> 
> I believe you could replace this too lines with just:
> 
>   AtkObject* root = atk_object_ref_accessible_child(ATK_OBJECT(accessibilityObject), 0);
>   if (s_rootObject != root)
>       s_rootObject = adoptGRef(root);
> 
> ...and remove the function webPageAccessibilityUtilRootObjectSet()
> 
> > Source/WebKit2/WebProcess/WebPage/efl/WebPageAccessibilityUtil.cpp:166
> > +    static bool atkBridgeInitialized = 0;
> > +    if (atkBridgeInitialized)
> > +        return;
> 
> You would not need this if you implement a singleton. Well.. at least not here, even if you would need something similar of course :)
> 
> > Source/WebKit2/WebProcess/WebPage/efl/WebPageAccessibilityUtil.cpp:175
> > +    AtkUtilClass* utilClass = ATK_UTIL_CLASS(g_type_class_ref(ATK_TYPE_UTIL));
> > +    utilClass->add_global_event_listener = webPageAccessibilityUtilAddGlobalEventListener;
> > +    utilClass->remove_global_event_listener = webPageAccessibilityUtilRemoveGlobalEventListener;
> > +    utilClass->add_key_event_listener = 0;
> > +    utilClass->remove_key_event_listener = 0;
> > +    utilClass->get_root = webPageAccessibilityUtilGetRoot;
> > +    utilClass->get_toolkit_name = 0;
> > +    utilClass->get_toolkit_version = 0;
> 
> It's a bit unnatural to me to see this code in a C++ class. I really think you should go for proper GObject implementation here, unless there's something I'm missing specific to the EFL port.
> 
> > Source/WebKit2/WebProcess/WebPage/efl/WebPageAccessibilityUtil.cpp:176
> > +    s_listenerList = g_hash_table_new_full(g_int_hash, g_int_equal, 0, g_free);
> 
> This hash table is never freed, hence is leaked. You will avoid it by using a GRefPtr for it, as suggested above.
> 
> > Source/WebKit2/WebProcess/WebPage/efl/WebPageAccessibilityUtil.h:36
> > +    ~WebPageAccessibilityUtil();
> > +
> > +    static guint webPageAccessibilityUtilAddGlobalEventListener(GSignalEmissionHook, const gchar*);
> > +    static void webPageAccessibilityUtilRemoveGlobalEventListener(guint);
> > +    static AtkObject* webPageAccessibilityUtilGetRoot();
> > +    static guint addListener(GSignalEmissionHook, const gchar*, const gchar*, const gchar*);
> 
> All these things will be normal private static functions private to the cpp file if you migrate to a regular GObject-like implementation
> 
> > Source/WebKit2/WebProcess/WebPage/efl/WebPageAccessibilityUtil.h:38
> > +    void webPageAccessibilityUtilInit(WebPageAccessibilityObject*);
> 
> We really need to clarify this thing of WebPageAccessibilityUtil being a global object that it's initialized with Web pages (which are not global at all). I'm not 100% sure yet, but I feel like there is something wrong here.
> 
> Sorry for being a bit vague with this, but AtkUtil is something I never needed to deal with yet, Pinheiro is the expert (he did it for Ca11y and GTK, I think)
> 
> > Source/WebKit2/WebProcess/WebPage/efl/WebPageAccessibilityUtil.h:47
> > +    WebPageAccessibilityUtil();
> > +
> > +    void webPageAccessibilityUtilLoad();
> > +    void webPageAccessibilityUtilRootObjectSet(AtkObject*);
> > +
> > +    typedef void (*AtkBridgeInit)(void);
> > +    typedef void (*AtkBridgeShutdown)(void);
> 
> All this would just be in the cpp file if you move to GObject.
> 
> > Source/WebKit2/WebProcess/WebPage/efl/WebPageAccessibilityUtil.h:52
> > +    AtkBridgeInit m_atkBridgeInit;
> > +    AtkBridgeShutdown m_atkBridgeShutdown;
> > +
> > +    static AtkObject* s_rootObject;
> > +    static GHashTable* s_listenerList;
> 
> ...and these other would be part of the private struct, again in the GObject file. About the static ones, that should not be a problem if the GObject is implemented as a Singleton
> 
> > Source/WebKit2/WebProcess/WebPage/efl/WebPageEfl.cpp:52
> > +static OwnPtr<WebPageAccessibilityUtil> accessibilityUtilObject;
> 
> This would be a GRefPtr<WebPageAccessibilityUtil> after moving to GObject if you want to keep using the ref count. However, being a global object you can probably return just a WebPageAccessibilityUtil* and store it in the platformInitialize() function

[Krzysztof Czech]

> Also, I'm not 100% sure (again, Pinheiro is the expert in AtkUtil), but I believe an implementation of AtkUtil is something _global_ to the process, not local to every WebPage as it seems to suggest this code, and so get_root() should return always the same and only object regardless of when it's invoked.
> 
> The problem here is that, as far I can understand now, we don't have that "clear root AtkObject" as in GTK (in GTK+ atk_get_root() returns the AtkObject associated to the GTK Application) but "many root objects" instead, one per WebPage. And we can't just create a global AtkObject being the parent of all the topmost AtkObject's (the AtkPlug's) in the WebProcess because that would break the implementation of AtkPlug in the ATK bridge, which expects atk_object_get_parent() to return NULL over the AtkPlug object.
> 
> So, perhaps that's why you added that dummy object in the first place, I don't know... but now I think more of this and see this problem, I wonder whether it was not a good idea after all, or even if this thing of having a "dynamic root object" will work fine.

You are perfectly right that AtkUtil is something global and WebKit should not even know about this. Would also be great if this was somewhere in EFL. I'm just trying to workaround this problem, that's why I'm trying to do it in WebKit.
Generally, more I'm getting into it, I'm closer to the conclusion that WebKit is not a good place for initializing ATK-BRIDGE stuff.

[Build Bot]

Comment on attachment 192177 [details]
Patch

Attachment 192177 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17050812

New failing tests:
inspector/styles/styles-include-host-rules-crash.html

[Gyuyoung Kim]

Comment on attachment 192177 [details]
Patch

Cleared review? from attachment 192177 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).

[Michael Catanzaro]

Closing this bug because the EFL port has been removed from trunk.

If you feel this bug applies to a different upstream WebKit port and was closed in error, please either update the title and reopen the bug, or leave a comment to request this.