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

[bugzilla-daemon at webkit.org]

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


Raphael Kubo da Costa <kubo at profusion.mobi> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #128957|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #29 from Raphael Kubo da Costa <kubo at profusion.mobi>  2012-02-27 14:58:24 PST ---
(From update of attachment 128957)
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?

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