[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 #35 from YoungTaeck Song <youngtaeck.song at samsung.com>  2012-03-08 00:54:04 PST ---
Thank you for kind review.
Please review next patch.(https://bugs.webkit.org/attachment.cgi?id=130789&action=review)

(In reply to comment #33)
> 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.

Ok. I moved TimerWorkItem class to cpp file. and removed WorkItemEfl and FdWorkItem class.

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

Ok, I see your point.
I fixed that at next patch.

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

Ok, I fixed that at next patch.

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

Sorry, I got a wrong information. 
I fixed that at next patch.

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