[webkit-reviews] review denied: [Bug 62777] [EFL][WK2] Add RunLoopEfl and WorkQueueEfl : [Attachment 128957] Patch

[bugzilla-daemon at webkit.org]

Raphael Kubo da Costa <kubo at profusion.mobi> has denied YoungTaeck Song
<youngtaeck.song at samsung.com>'s request for review:
Bug 62777: [EFL][WK2] Add RunLoopEfl and WorkQueueEfl
https://bugs.webkit.org/show_bug.cgi?id=62777

Attachment 128957: Patch
https://bugs.webkit.org/attachment.cgi?id=128957&action=review

------- Additional Comments from Raphael Kubo da Costa <kubo at profusion.mobi>
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. Plus I'd move most of the declarations and implementations in WorkQueue.h
to the .cpp file itself as private structs/classes.

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

> Source/WebCore/platform/efl/RunLoopEfl.cpp:28
> +#include <stdio.h>
> +#include <unistd.h>

I don't think these headers are needed.

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

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

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

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

Style: m_fdWorkItems

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

Why static?

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

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

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

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

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

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:121
> +    m_FdWorkItems.append(adoptRef(new FdWorkItem(fd, function, this)));

Isn't the FdWorkItem object leaking?

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:137
> +    scheduleWorkAndWakeUp(adoptRef(new WorkItemEfl(function, this)));

Isn't the WorkItemEfl object being leaked?

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

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

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:173
> +    return false;

I'd rather be explicit and return 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?