[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 #10 from YoungTaeck Song <youngtaeck.song at samsung.com>  2011-06-17 01:28:14 PST ---
(In reply to comment #8)
> Informal r- from my side:
> 
> > Source/WebKit2/Platform/RunLoop.h:111
> > +        static Eina_Bool timerFired(void* data);
> 
> Can you make this method non-static and move it and the attributes to the private section?

I'm not sure I understand your meaning.

It should be static member method. 
Because ecore_timer_add function's 2th argument Ecore_Task_Cb is "Eina_Bool (*) (void *data);"
It can takes C++ static method or C function.
And It is already in the private section.

> 
> > Source/WebKit2/Platform/RunLoop.h:174
> > +    static void wakeUpEvent(void* data, void* buf, unsigned int size);
> 
> Does it need to be static?

Same as above.

> 
> > Source/WebKit2/Platform/WorkQueue.h:224
> > +    ThreadIdentifier m_workQueueThread;
> 
> Don't you need to initialize some of these attributes to sane values in the constructor?

I don`t use this attribute.
So I'll remove m_workQueueThread in next patch.

> 
> > Source/WebKit2/Platform/WorkQueue.h:234
> > +    static void* workQueueThread(WorkQueue*);
> 
> This name/signature is weird. You have m_workQueueThread, so a method called workQueueThread() is usually expected to return m_workQueueThread, not perform some random work and then just return 0 at the end (as you always return 0, I don't see why you return a void* by the way.

I'll remove m_workQueueThread in next patch. 
So I'll continue to use workQueueThread name.

And the type of workQueueThread should be 'void* (*)(void* argument);'.
So I return 0.
workQueueThread is argument of createThread function.
( ThreadIdentifier createThread(ThreadFunction entryPoint, void* data, const char* name) )
ThreadFunction type is 'void* (*)(void* argument);'

> 
> > Source/WebKit2/Platform/WorkQueue.h:238
> > +    static Eina_Bool timerFired(void*);    
> 
> Does this function and the other need to be static?

Same as above your first comment.

> 
> > Source/WebKit2/Platform/efl/RunLoopEfl.cpp:60
> > +    ((RunLoop*)data)->performWork();
> 
> C++ cast.

Thanks. I'll fix it in the next patch.

> 
> > Source/WebKit2/Platform/efl/RunLoopEfl.cpp:69
> > +    : m_timer(0)
> 
> It's safer to also initialize m_isRepeating.

I'll fix it in the next patch.

> 
> > Source/WebKit2/Platform/efl/RunLoopEfl.cpp:80
> > +    RunLoop::TimerBase* timer = (RunLoop::TimerBase*)data;
> 
> C++ cast.

Thanks. I'll fix it in the next patch.

> 
> > Source/WebKit2/Platform/efl/RunLoopEfl.cpp:97
> > +    m_timer = ecore_timer_add(fireInterval, timerFired, this);
> 
> Is there any risk of start() being called multiple times, thus leaking the previous timers?

You're right. Thanks. I'll fix it in the next patch.

> 
> > Source/WebKit2/Platform/efl/RunLoopEfl.cpp:109
> > +    return m_timer;
> 
> You're implicitly casting an Ecore_Timer pointer into a bool. It's better to be explicit and use "return (m_timer != 0);"

"return (m_timer != 0);" generates webkit-style error like this.
"Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons"
So I'll use "return (m_timer) ? true : false;"

> 
> > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:125
> > +    WorkItemEfl* item = (WorkItemEfl*)data;
> 
> Please use a C++ cast here instead of a C one.

I'll fix it in the next patch.

> 
> > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:134
> > +    ecore_timer_add(delay, timerFired, (void*)(new WorkItemEfl(item, this)));
> 
> Ditto (are you sure you really need to cast here?). Plus, don't you need to remove this timer later?

OK. I'll remove "(void*)".
If timerFired return ECORE_CALLBACK_CANCEL, this timer will be automatically removed.
(http://docs.enlightenment.org/auto/ecore/group__Ecore__Time__Group.html#gad84e3e9e2b70a915ce9d44263666bb07)

> 
> > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:137
> > +Eina_Bool WorkQueue::exitHandler(void  * data, int type, void* event)
> 
> void* data, not void  * data

My editor was weird. I'll fix it in the next patch.

> 
> > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:139
> > +    TermWorkItem* item = (TermWorkItem*)data;
> 
> C++ cast.

I'll fix it in the next patch.

> 
> > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:140
> > +    Ecore_Exe_Event_Del* ev = (Ecore_Exe_Event_Del*)event;
> 
> C++ cast.

I'll fix it in the 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.