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

Fri Mar 2 02:12:21 PST 2012

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

--- Comment #31 from YoungTaeck Song <youngtaeck.song at samsung.com>  2012-03-02 02:12:21 PST ---
(In reply to comment #29)
> (From update of attachment 128957 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128957&action=review
> 
> The WorkItem implementation looks very convoluted and I don't see why you are not using Ecore_Pipe, Ecore_Main_Loop and friends to make things easier to read. 

I can't use Ecore_Pipe, Because Ecore has limatation for threaded main loop.
Other ports, like GTK, Main loop can run on the thread. 
But Ecore does not.
So I made smallest version of Ecore_Main_Loop, WorkQueue::workQueueThread.

In other words, Ecore_Pipe's handler run on the Ecore_Main_Loop.
But I want pipe's handler on the thread.

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

> 
> > Source/WebCore/platform/RunLoop.h:106
> > +        Ecore_Timer* m_timer;
> 
> In the spirit of using OwnPtrs, what if you use an OwnPtr here? You'd be able to remove most of the checks for m_timer and calls to ecore_timer_del() in the implementation.

You're right.
I added the codes using OwnPtrs.

> 
> > Source/WebCore/platform/efl/RunLoopEfl.cpp:28
> > +#include <stdio.h>
> > +#include <unistd.h>
> 
> I don't think these headers are needed.

Thanks, I fixed that at next patch.

> 
> > Source/WebCore/platform/efl/RunLoopEfl.cpp:33
> > +#define WAKEUP_ECORE_PIPE_MESSAGE   "W"
> > +#define ECORE_PIPE_MESSAGE_SIZE     1
> 
> I'd rather have proper static const variables here instead (and preferably alphabetically sorted).

Ok, I fixed that at next patch.

> 
> > Source/WebCore/platform/efl/RunLoopEfl.cpp:49
> > +    if (!ecore_init())
> > +        ASSERT_NOT_REACHED();
> > +
> > +    if (!ecore_evas_init())
> > +        ASSERT_NOT_REACHED();
> > +
> > +    if (!ecore_file_init())
> > +        ASSERT_NOT_REACHED();
> > +
> > +    if (!edje_init())
> > +        ASSERT_NOT_REACHED();
> 
> ASSERT_NOT_REACHED will only have any effect in debug builds. Do you really need to initialize these here, or are these libraries initialized beforehand as well?

Sure, RunLoop is wrapper of Ecore_Main_Loop, So  they're needed to initialize here.
And I changed this code like WebKit1's ewk_init().

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

> 
> > Source/WebKit2/Platform/WorkQueue.h:247
> > +    Vector<RefPtr<FdWorkItem> > m_FdWorkItems;
> 
> Style: m_fdWorkItems

Ok, I fixed that at next patch.

> 
> > Source/WebKit2/Platform/WorkQueue.h:252
> > +    static HashMap<WebKit::PlatformProcessIdentifier, RefPtr<WorkItemEfl> > m_termWorkItems;
> 
> Why static?

It is used in static function WorkQueue::exitHandler.

> 
> > Source/WebKit2/Platform/WorkQueue.h:261
> > +    void sendMessageToThread(const char*);
> > +    static void* workQueueThread(WorkQueue*);
> > +    void scheduleWorkAndWakeUp(PassRefPtr<WorkItemEfl>);
> > +    void performWork();
> > +    void performFdWork();
> > +    size_t getNextTimerID();
> > +    static bool timerFired(void*);
> > +    static bool exitHandler(void*, int, void*);
> 
> I don't see why you didn't defined those as static functions in the cpp file.

I don't know what you mean.
You mean that "void* WorkQueue::workQueueThread(WorkQueue* workQueue) {} " change to "static void* WorkQueue::workQueueThread(WorkQueue* workQueue) {} " in the cpp file?
It won't be compiled.

> 
> > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:28
> > +#define WAKEUP_THREAD_MESSAGE   "W"
> > +#define FINISH_THREAD_MESSAGE   "F"
> > +#define THREAD_MESSAGE_SIZE     1
> 
> Same thing about using static const's here.

Ok, I fixed that at next patch.

> 
> > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:42
> > +    ecore_event_handler_add(ECORE_EXE_EVENT_DEL, reinterpret_cast<Eina_Bool(*)(void*, int, void*)>(exitHandler), 0);
> 
> Does reinterpred_cast<Ecore_Event_Handler_Cb>(exitHandler) work?

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

> 
> > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:118
> > +    for (size_t i = 0; i < m_FdWorkItems.size(); ++i)
> > +        if (m_FdWorkItems[i]->fd() != fd)
> 
> Vector::find(). You might consider using a HashMap so you don't have an O(n) search here.

I can't use Vector::find(). Because m_FdWorkItems's value is not 'int fd' but 'FdWorkItem'

void WorkQueue::performFdWork()
{
    if (select(m_maxFd + 1, &readFdSet, 0, 0, 0) >= 0) {
......
        for (size_t i = 0; i < m_fdWorkItems.size(); ++i) {
            if (FD_ISSET(m_fdWorkItems[i]->fd(), &readFdSet))
                m_fdWorkItems[i]->execute();
        }
This code are main parts using m_fdWorkItems about select(). They always have to examine all fd.
So I think it's not good to use HashMap.

> 
> > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:121
> > +    m_FdWorkItems.append(adoptRef(new FdWorkItem(fd, function, this)));
> 
> Isn't the FdWorkItem object leaking?

It's using RefPtr. It will be freed automatically.

> 
> > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:137
> > +    scheduleWorkAndWakeUp(adoptRef(new WorkItemEfl(function, this)));
> 
> Isn't the WorkItemEfl object being leaked?

It's using RefPtr. It will be freed automatically.

> 
> > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:143
> > +size_t WorkQueue::getNextTimerID()
> > +{
> > +    return m_timers.size();
> > +}
> 
> This is not necessary if you pass a handle to the right item in m_timers.

Ok, I removed getNextTimerID().
But I still need timerId for 'new TimerWorkItem'.

> 
> > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:158
> > +    m_timers[timerId] = ecore_timer_add(delay, reinterpret_cast<Eina_Bool(*)(void*)>(timerFired), new TimerWorkItem(timerId, function, this));
> 
> reinterpret_cast<Ecore_Task_Cb>(timerFired)? Isn't the TimerWorkItem object being leaked?

Ok, I fixed that at next patch about Ecore_Task_Cb.
And TimerWorkItem is wrapped in RefPtr. It will be freed automatically.

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

> 
> > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:179
> > +    m_termWorkItems.set(process, new WorkItemEfl(function, this));
> 
> Isn't the WorkItemEfl object being leaked?

It's using RefPtr. It will be freed automatically.

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