[Webkit-unassigned] [Bug 109937] [EFL][WK2] Additional bits of having Accessibility in WebKit-EFL.
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Mar 8 07:19:16 PST 2013
https://bugs.webkit.org/show_bug.cgi?id=109937
--- Comment #10 from Krzysztof Czech <k.czech at samsung.com> 2013-03-08 07:21:40 PST ---
(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
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list