[Webkit-unassigned] [Bug 62777] [EFL][WK2] Add RunLoopEfl and WorkQueueEfl

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=62777





--- Comment #33 from Raphael Kubo da Costa <kubo at profusion.mobi>  2012-03-06 05:08:11 PST ---
Addressing the easier points first, the rest will come as time allows :)

(In reply to comment #31)
> (In reply to comment #29)
> > Plus I'd move most of the declarations and implementations in WorkQueue.h to the .cpp file itself as private structs/classes.
> 
> You have a point. but I do not want to have other forms of Other ports.

It's mostly Windows which declares a lot of stuff in WorkQueue.h.

> > > Source/WebCore/platform/efl/RunLoopEfl.cpp:59
> > > +    edje_shutdown();
> > > +    ecore_file_shutdown();
> > > +    ecore_evas_shutdown();
> > > +    ecore_shutdown();
> > 
> > If you conditionally init the modules in the constructor, you shouldn't unconditionally shut everything down in the destructor.
> 
> It is customary style of EFL. 
> Please see EFL code internally.
> There are many example for that.

No, it's not. Suppose ecore_evas_init() fails in the constructor. When you call edje_shutdown() in the destructor its internal counter will drop below 0, as the calls to init() and shutdown() are not balanced.

> > > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:82
> > > +                LOG_ERROR("Failed to read from WorkQueueThread pipe");
> > 
> > Don't you need wtf/Assertions.h for LOG_ERROR?
> 
> Yes, don't need wtf/Assertions.h.

My point is that wtf/Assertions.h is being included indirectly from some other header, it'd be safer to include it directly to avoid this kind of unseen dependency.

> > > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:173
> > > +    return false;
> > 
> > I'd rather be explicit and return ECORE_CALLBACK_CANCEL.
> 
> WebKit reviewers were ordered not to use EFL specifics like ECORE_CALLBACK_CANCEL.
> We have to use 'false' instead of ECORE_CALLBACK_CANCEL.

Eh? I don't think there's any rule like that set in stone. I only remember we talked about using bool instead of Eina_Bool whenever possible. Using ECORE_CALLBACK_CANCEL in this context makes it much more explicit what the return value actually means.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.