Bug 62777 – [EFL][WK2] Add RunLoopEfl and WorkQueueEfl

• Component: WebKit EFL
• Status: RESOLVED FIXED

[YoungTaeck Song]

Add RunLoopEfl and WorkQueueEfl

[YoungTaeck Song]

Created attachment 97403 [details]
Add RunLoopEfl and WorkQueueEfl for Efl port

[Gyuyoung Kim]

Comment on attachment 97403 [details]
Add RunLoopEfl and WorkQueueEfl for Efl port

View in context: https://bugs.webkit.org/attachment.cgi?id=97403&action=review

> Source/WebKit2/Platform/WorkQueue.h:228
> +    Mutex m_FdWorkItemsLock;

I think it is better to sort variables of same types.

[Ryuan Choi]

View in context: https://bugs.webkit.org/attachment.cgi?id=97403&action=review

> Source/WebKit2/Platform/WorkQueue.h:57
>  #if PLATFORM(QT)
>  #include <QSocketNotifier>
> -#include "PlatformProcessIdentifier.h"
>  class QObject;
>  class QThread;
>  #elif PLATFORM(GTK)
> -#include "PlatformProcessIdentifier.h"
>  typedef struct _GMainContext GMainContext;
>  typedef struct _GMainLoop GMainLoop;
>  typedef gboolean (*GSourceFunc) (gpointer data);
> +#elif PLATFORM(EFL)
> +#include <Ecore.h>
> +#endif
> +
> +#if PLATFORM(QT) || PLATFORM(GTK) || PLATFORM(EFL)
> +#include "PlatformProcessIdentifier.h"
>  #endif

Don't you forget to change PlatformProcessIdentifier.h?
Each port defines PlatformProcessIdentifier in PlatformProcessIdentifier.h
But, Efl port was missing if I remember correctly.

[YoungTaeck Song]

Created attachment 97412 [details]
Modify PlatformProcessIdentifier.h

[Gyuyoung Kim]

Comment on attachment 97412 [details]
Modify PlatformProcessIdentifier.h

View in context: https://bugs.webkit.org/attachment.cgi?id=97412&action=review

> Source/WebKit2/ChangeLog:8
> +        Add RunLoopEfl and WorkQueueEfl for Efl port

Change patch description according to new patch.

[YoungTaeck Song]

(In reply to comment #2)
> (From update of attachment 97403 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97403&action=review
> 
> > Source/WebKit2/Platform/WorkQueue.h:228
> > +    Mutex m_FdWorkItemsLock;
> 
> I think it is better to sort variables of same types.
I was ordered by the group. I think it is better.

[YoungTaeck Song]

(In reply to comment #3)
> View in context: https://bugs.webkit.org/attachment.cgi?id=97403&action=review
> 
> > Source/WebKit2/Platform/WorkQueue.h:57
> >  #if PLATFORM(QT)
> >  #include <QSocketNotifier>
> > -#include "PlatformProcessIdentifier.h"
> >  class QObject;
> >  class QThread;
> >  #elif PLATFORM(GTK)
> > -#include "PlatformProcessIdentifier.h"
> >  typedef struct _GMainContext GMainContext;
> >  typedef struct _GMainLoop GMainLoop;
> >  typedef gboolean (*GSourceFunc) (gpointer data);
> > +#elif PLATFORM(EFL)
> > +#include <Ecore.h>
> > +#endif
> > +
> > +#if PLATFORM(QT) || PLATFORM(GTK) || PLATFORM(EFL)
> > +#include "PlatformProcessIdentifier.h"
> >  #endif
> 
> Don't you forget to change PlatformProcessIdentifier.h?
> Each port defines PlatformProcessIdentifier in PlatformProcessIdentifier.h
> But, Efl port was missing if I remember correctly.

Sorry, I missed it. I submitted the new patch including modificaton of PlatformProcessIdentifier.h.

[Raphael Kubo da Costa (:rakuco)]

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?

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

Does it need to be static?

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

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

> Source/WebKit2/Platform/WorkQueue.h:238
> +    static Eina_Bool timerFired(void*);    

Does this function and the other need to be static?

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

C++ cast.

> Source/WebKit2/Platform/efl/RunLoopEfl.cpp:69
> +    : m_timer(0)

It's safer to also initialize m_isRepeating.

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

C++ cast.

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

> 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);"

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:125
> +    WorkItemEfl* item = (WorkItemEfl*)data;

Please use a C++ cast here instead of a C one.

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

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

void* data, not void  * data

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

C++ cast.

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

C++ cast.

[Sam Weinig]

Comment on attachment 97412 [details]
Modify PlatformProcessIdentifier.h

View in context: https://bugs.webkit.org/attachment.cgi?id=97412&action=review

> Source/WebKit2/Platform/WorkQueue.h:222
> +        public:
> +            WorkItemEfl(PassOwnPtr<WorkItem> item, WorkQueue* queue) : m_item(item), m_queue(queue) { }
> +            ~WorkItemEfl() { }
> +    
> +            WorkItem* item() const { return m_item.get(); }
> +            WorkQueue* queue() const { return m_queue; }
> +    
> +        private:
> +            OwnPtr<WorkItem> m_item;
> +            WorkQueue* m_queue;
> +    };
> +    
> +    class FdWorkItem : public WorkItemEfl {
> +        public:
> +            FdWorkItem(int fd, PassOwnPtr<WorkItem> item, WorkQueue* queue)   
> +                : WorkItemEfl(item, queue), m_fd(fd) { }
> +            ~FdWorkItem() { }
> +            int fd() const { return m_fd; }
> +    
> +        private:
> +            int m_fd;
> +    };
> +    
> +    class TermWorkItem : public WorkItemEfl {
> +        public:
> +            TermWorkItem(int pid, PassOwnPtr<WorkItem> item, WorkQueue* queue)
> +                : WorkItemEfl(item, queue), m_pid(pid) { }
> +            ~TermWorkItem() { }
> +            int pid() const { return m_pid; }
> +    
> +        private:
> +            int m_pid;
> +    };

The public: and private: should not be indented.  We also traditionally put each line of the initialization syntax on its own line.

[YoungTaeck Song]

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

[YoungTaeck Song]

(In reply to comment #9)
> (From update of attachment 97412 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=97412&action=review
> 
> > Source/WebKit2/Platform/WorkQueue.h:222
> > +        public:
> > +            WorkItemEfl(PassOwnPtr<WorkItem> item, WorkQueue* queue) : m_item(item), m_queue(queue) { }
> > +            ~WorkItemEfl() { }
> > +    
> > +            WorkItem* item() const { return m_item.get(); }
> > +            WorkQueue* queue() const { return m_queue; }
> > +    
> > +        private:
> > +            OwnPtr<WorkItem> m_item;
> > +            WorkQueue* m_queue;
> > +    };
> > +    
> > +    class FdWorkItem : public WorkItemEfl {
> > +        public:
> > +            FdWorkItem(int fd, PassOwnPtr<WorkItem> item, WorkQueue* queue)   
> > +                : WorkItemEfl(item, queue), m_fd(fd) { }
> > +            ~FdWorkItem() { }
> > +            int fd() const { return m_fd; }
> > +    
> > +        private:
> > +            int m_fd;
> > +    };
> > +    
> > +    class TermWorkItem : public WorkItemEfl {
> > +        public:
> > +            TermWorkItem(int pid, PassOwnPtr<WorkItem> item, WorkQueue* queue)
> > +                : WorkItemEfl(item, queue), m_pid(pid) { }
> > +            ~TermWorkItem() { }
> > +            int pid() const { return m_pid; }
> > +    
> > +        private:
> > +            int m_pid;
> > +    };
> 
> The public: and private: should not be indented.  We also traditionally put each line of the initialization syntax on its own line.

Thanks. I'll fix it at the next patch

[YoungTaeck Song]

Created attachment 97562 [details]
patch.

[Raphael Kubo da Costa (:rakuco)]

It looks much better to me. Thanks for the clarifications WRT the static methods, I needed to brush up that part of my C++ fu :)

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:82
> +    int ret;

I don't think you actually need this variable, you could call select directly in the if clause below.

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:130
> +int WorkQueue::getNextTimerID()

Vector::size() returns a size_t, you should keep the same return type.

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:150
> +    int timerId = getNextTimerID();

Remember to change timerId's type when you change getNextTimerID().

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:160
> +        item->queue()->scheduleWorkAndWakeUp(item);

Isn't item leaking if item->pid() != ev->pid?

[YoungTaeck Song]

Created attachment 98123 [details]
patch.

rewrite scheduleWorkOnTermination

[YoungTaeck Song]

(In reply to comment #13)
> It looks much better to me. Thanks for the clarifications WRT the static methods, I needed to brush up that part of my C++ fu :)
> 
> > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:82
> > +    int ret;
> 
> I don't think you actually need this variable, you could call select directly in the if clause below.
> 
> > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:130
> > +int WorkQueue::getNextTimerID()
> 
> Vector::size() returns a size_t, you should keep the same return type.
> 
> > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:150
> > +    int timerId = getNextTimerID();
> 
> Remember to change timerId's type when you change getNextTimerID().
> 
> > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:160
> > +        item->queue()->scheduleWorkAndWakeUp(item);
> 
> Isn't item leaking if item->pid() != ev->pid?

Thanks for your comments.
I fixed it at next fetch.

[Raphael Kubo da Costa (:rakuco)]

I think it looks OK now, informal r+ from my side.

[Ryuan Choi]

All of code is fine to me without below blanks

View in context: https://bugs.webkit.org/attachment.cgi?id=98123&action=review

> Source/WebKit2/Platform/WorkQueue.h:205
> +    

please remove unnecessary blank at end of line.

> Source/WebKit2/Platform/WorkQueue.h:208
> +        FdWorkItem(int fd, PassOwnPtr<WorkItem> item, WorkQueue* queue)   

Ditto.

> Source/WebKit2/Platform/WorkQueue.h:233
> +    

Ditto.

> Source/WebKit2/Platform/WorkQueue.h:253
> +    static Eina_Bool timerFired(void*);    

Ditto.

[YoungTaeck Song]

Created attachment 99039 [details]
fetch

Add EFL initialization function in RunLoop::RunLoop

[Raphael Kubo da Costa (:rakuco)]

> Source/WebKit2/Platform/efl/RunLoopEfl.cpp:62
> +    eina_shutdown();

You are not calling eina_init() in the constructor.

[YoungTaeck Song]

Created attachment 103044 [details]
fetch

Bug Fetch.
- remove the code calling eina_shutdown(), I'm not calling eina_init().
- In WorkQueue::performFdWork(), Add the code reading pipe. because select() can't block.

[Leandro Pereira]

Comment on attachment 103044 [details]
fetch

View in context: https://bugs.webkit.org/attachment.cgi?id=103044&action=review

Informal r- for the reasons below.

> Source/WebKit2/Platform/efl/RunLoopEfl.cpp:13
> + * Copyright (C) 2011 Samsung Electronics. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + *    notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + *    notice, this list of conditions and the following disclaimer in the
> + *    documentation and/or other materials provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''

Is this provided by Apple or Samsung? Please review the license before submitting the file.

> Source/WebKit2/Platform/efl/RunLoopEfl.cpp:108
> +    Eina_Bool ret;
> +
> +    timer->fired();
> +    
> +    if (!timer->m_isRepeating) {
> +        timer->m_timer = 0;
> +        ret = ECORE_CALLBACK_CANCEL;
> +    } else
> +        ret = ECORE_CALLBACK_RENEW;
> +
> +    return ret;

Just a minor nit, but ``ret'' variable could be removed and this function could be written as:

RunLoop::TimerBase* timer = ...;

timer->fired();

if (!timer->m_isRepeating) {
   timer->m_timer = 0;
   return ECORE_CALLBACK_CANCEL;
}

return ECORE_CALLBACK_RENEW;

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:55
> +void WorkQueue::platformInvalidate()
> +{
> +    close(m_readFd);
> +    close(m_writeFd);
> +
> +    for (size_t i = 0; i < m_timers.size(); ++i)
> +        ecore_timer_del(m_timers[i]);

Is there any chance this could be called from more than one thread? If so, protecting this with a mutex (see GTK impl.) would be a good idea.

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:60
> +void WorkQueue::performWork()
> +{
> +    m_workItemQueueLock.lock();

I'd prefer if a MutexLocker were used here. Manual mutex locking/unlocking is very fragile.

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:78
> +    fd_set readfds = m_fds;

Use ``readFdSet'' or something like that instead of ``readfds''.

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:122
> +    if (write(m_writeFd, "w", 1) == -1) // WakeUp WorkQueue Thread
> +        LOG_ERROR("WakeUp WorkQueue Thread Failed");

Please use proper sentences on comments and log messages.

[YoungTaeck Song]

Created attachment 120929 [details]
fetch for RunLoopEfl and WorkQueueEfl

[WebKit Review Bot]

Attachment 120929 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/Platform/WorkQueue.h:55:  "PlatformProcessIdentifier.h" already included at Source/WebKit2/Platform/WorkQueue.h:46  [build/include] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[YoungTaeck Song]

Created attachment 120932 [details]
fetch for RunLoopEfl and WorkQueueEfl

[YoungTaeck Song]

Very sorry I'm late to answer.

(In reply to comment #21)
> (From update of attachment 103044 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103044&action=review
> 
> Informal r- for the reasons below.
> 
> > Source/WebKit2/Platform/efl/RunLoopEfl.cpp:13
> > + * Copyright (C) 2011 Samsung Electronics. All rights reserved.
> > + *
> > + * Redistribution and use in source and binary forms, with or without
> > + * modification, are permitted provided that the following conditions
> > + * are met:
> > + * 1. Redistributions of source code must retain the above copyright
> > + *    notice, this list of conditions and the following disclaimer.
> > + * 2. Redistributions in binary form must reproduce the above copyright
> > + *    notice, this list of conditions and the following disclaimer in the
> > + *    documentation and/or other materials provided with the distribution.
> > + *
> > + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''
> 
> Is this provided by Apple or Samsung? Please review the license before submitting the file.

===> I fixed it in the next patch.

> 
> > Source/WebKit2/Platform/efl/RunLoopEfl.cpp:108
> > +    Eina_Bool ret;
> > +
> > +    timer->fired();
> > +    
> > +    if (!timer->m_isRepeating) {
> > +        timer->m_timer = 0;
> > +        ret = ECORE_CALLBACK_CANCEL;
> > +    } else
> > +        ret = ECORE_CALLBACK_RENEW;
> > +
> > +    return ret;
> 
> Just a minor nit, but ``ret'' variable could be removed and this function could be written as:
> 
> RunLoop::TimerBase* timer = ...;
> 
> timer->fired();
> 
> if (!timer->m_isRepeating) {
>    timer->m_timer = 0;
>    return ECORE_CALLBACK_CANCEL;
> }
> 
> return ECORE_CALLBACK_RENEW;

==> I fixed it in the next patch.

> 
> > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:55
> > +void WorkQueue::platformInvalidate()
> > +{
> > +    close(m_readFd);
> > +    close(m_writeFd);
> > +
> > +    for (size_t i = 0; i < m_timers.size(); ++i)
> > +        ecore_timer_del(m_timers[i]);
> 
> Is there any chance this could be called from more than one thread? If so, protecting this with a mutex (see GTK impl.) would be a good idea.

==> I modified WorkQueue::platformInvalidate() in the next patch.
The functions in WorkQueue::platformInvalidate() are all thread-safe.

> 
> > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:60
> > +void WorkQueue::performWork()
> > +{
> > +    m_workItemQueueLock.lock();
> 
> I'd prefer if a MutexLocker were used here. Manual mutex locking/unlocking is very fragile.

===> MutexLocker has block scope. 
But m_workItemQueueLock should be unlock in the middle of WorkQueue::performWork().
(Please see WIN impl.)

> 
> > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:78
> > +    fd_set readfds = m_fds;
> 
> Use ``readFdSet'' or something like that instead of ``readfds''.

==> I fixed it in the next patch.

> 
> > Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:122
> > +    if (write(m_writeFd, "w", 1) == -1) // WakeUp WorkQueue Thread
> > +        LOG_ERROR("WakeUp WorkQueue Thread Failed");
> 
> Please use proper sentences on comments and log messages.

==> I fixed it in the next patch.

[YoungTaeck Song]

Created attachment 123263 [details]
fetch for RunLoopEfl and WorkQueueEfl 

Because RunLoop was moved to the webcore, I submit the new patch.

[Ryuan Choi]

Comment on attachment 123263 [details]
fetch for RunLoopEfl and WorkQueueEfl 

View in context: https://bugs.webkit.org/attachment.cgi?id=123263&action=review

> Source/WebCore/platform/efl/RunLoopEfl.cpp:49
> +    m_pipe = ecore_pipe_add(wakeUpEvent, this);

Can we use OwnPtr for this ?

[YoungTaeck Song]

Created attachment 128957 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 128957 [details]
Patch

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?

[YoungTaeck Song]

Created attachment 129848 [details]
patch

[YoungTaeck Song]

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

[Gyuyoung Kim]

Comment on attachment 129848 [details]
patch

Attachment 129848 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11751882

[Raphael Kubo da Costa (:rakuco)]

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

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

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

[YoungTaeck Song]

Created attachment 130789 [details]
Patch

[YoungTaeck Song]

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.

[YoungTaeck Song]

Created attachment 130792 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 130792 [details]
Patch

Attachment 130792 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11863258

[Ryuan Choi]

youngtaeck,
please rebase the patch for the green bots.

[YoungTaeck Song]

Created attachment 132195 [details]
Patch

[YoungTaeck Song]

Created attachment 133406 [details]
Patch

[Ryuan Choi]

Comment on attachment 133406 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133406&action=review

> Source/WebCore/platform/efl/RunLoopEfl.cpp:-2
> - * Copyright (C) 2012 ProFUSION embedded systems. All rights reserved.

IMO, just adding 'Copyright (C) 2012 Samsung Electronics' is better.

[YoungTaeck Song]

Created attachment 133448 [details]
Patch

[Ryuan Choi]

Comment on attachment 133448 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133448&action=review

> Source/WebCore/platform/efl/RunLoopEfl.cpp:38
> +static const char* const wakupEcorePipeMessage = "W";

Nitpick, static const char wakeupEcorePipeMessage[] = "W";

> Source/WebCore/platform/efl/RunLoopEfl.cpp:48
> +        goto errorEcore;

how about removing errorEcore and returning ?

> Source/WebKit2/Platform/WorkQueue.h:188
> +    class TimerWorkItem;

IMO, this is unnecessary.

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:49
> +static const char* const finishThreadMessage = "F";
> +static const char* const wakupThreadMessage = "W";

Ditto.

[YoungTaeck Song]

Created attachment 133707 [details]
Patch

[Ryuan Choi]

Comment on attachment 133707 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133707&action=review

> Source/WebCore/platform/efl/RunLoopEfl.cpp:109
> +RunLoop::TimerBase::TimerBase(RunLoop* runLoop)

Let's remove runLoop if we don't need.

[YoungTaeck Song]

Created attachment 133717 [details]
Patch

[YoungTaeck Song]

Created attachment 133721 [details]
Patch

[Ryuan Choi]

Comment on attachment 133721 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133721&action=review

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:163
> +void WorkQueue::scheduleWorkAndWakeUp(const Function<void()>& function)
> +{
> +    if (function.isNull())
> +        return;
> +
> +    MutexLocker locker(m_workItemQueueLock);
> +    m_workItemQueue.append(function);
> +    sendMessageToThread(wakupThreadMessage);
> +}
> +
> +void WorkQueue::dispatch(const Function<void()>& function)
> +{
> +    scheduleWorkAndWakeUp(function);
> +}
> +

Sorry about incremental review.

But, I think that scheduleWorkAndWakeUp is also unnecessary.

And 
I am not sure but `ASSERT(function.isNull())` is better than `if (function.isNull())`.

[YoungTaeck Song]

Created attachment 133725 [details]
Patch

[YoungTaeck Song]

Created attachment 133726 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 133726 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133726&action=review

> Source/WebCore/platform/efl/RunLoopEfl.cpp:99
> +void RunLoop::wakeUpEvent(void* data, void* buf, unsigned int size)

Trivial nit : It looks *buffer* is better than *buf*

> Source/WebKit2/Platform/efl/WorkQueueEfl.cpp:140
> +void WorkQueue::unregisterSocketEventHandler(int fd)

ditto. Abbreviation is not WebKit coding style. It looks we can use fileDescriptor instead of fd.

[YoungTaeck Song]

Created attachment 133727 [details]
Patch

[Ryuan Choi]

(In reply to comment #52)
> Created an attachment (id=133727) [details]
> Patch

Looks good to me.

[Gyuyoung Kim]

Comment on attachment 133727 [details]
Patch

Looks fine.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 133727 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=133727&action=review

I haven't read the whole patch yet, but it looks better than the previous versions. I'm a bit unconvinced that using plain select(2) is the best approach here, as it has its fair share of scalability problems. Have you considered using poll() instead, for example?

> Source/WebCore/platform/RunLoop.h:171
> +    static void wakeUpEvent(void*, void*, unsigned int);

Let me nitpick a bit more :-)

In general, declarations of methods with basic data types, such as int or void* are expected to contain variable names, as it is not clear at first sight what they are supposed to mean.

> Source/WebCore/platform/efl/RunLoopEfl.cpp:44
> +    m_initEfl = false;

Nitpicking a bit more, this kind of variable initialization should be done before the constructor body:
  RunLoop::RunLoop()
      : m_initEfl(false)
  {
  }

[YoungTaeck Song]

Created attachment 133739 [details]
Patch

[YoungTaeck Song]

Thank you for spending much time about this patch.

(In reply to comment #55)
> (From update of attachment 133727 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133727&action=review
> 
> I haven't read the whole patch yet, but it looks better than the previous versions. I'm a bit unconvinced that using plain select(2) is the best approach here, as it has its fair share of scalability problems. Have you considered using poll() instead, for example?
> 

I don't know about select()'s fair share of scalability problems. Can you explain to me?
In my patch, just two file descriptor is checked, So I think that there is no difference no matter what I use poll, select and epoll.
(I checked this web page. http://daniel.haxx.se/docs/poll-vs-select.html)
When I made this code, I saw Ecore Main Loop source as a reference.
Ecore Main Loop are using select().

> > Source/WebCore/platform/RunLoop.h:171
> > +    static void wakeUpEvent(void*, void*, unsigned int);
> 
> Let me nitpick a bit more :-)
> 
> In general, declarations of methods with basic data types, such as int or void* are expected to contain variable names, as it is not clear at first sight what they are supposed to mean.

I fixed that as follow, "static void wakeUpEvent(void* data, void*, unsigned int);"
because 2,3 arguments are not used.

> 
> > Source/WebCore/platform/efl/RunLoopEfl.cpp:44
> > +    m_initEfl = false;
> 
> Nitpicking a bit more, this kind of variable initialization should be done before the constructor body:
>   RunLoop::RunLoop()
>       : m_initEfl(false)
>   {
>   }

fixed

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #57)
> I don't know about select()'s fair share of scalability problems. Can you explain to me?
> In my patch, just two file descriptor is checked, So I think that there is no difference no matter what I use poll, select and epoll.
> (I checked this web page. http://daniel.haxx.se/docs/poll-vs-select.html)
> When I made this code, I saw Ecore Main Loop source as a reference.
> Ecore Main Loop are using select().

One of the links I had in mind when I mentioned selects()'s scalability problems was <http://www.kegel.com/c10k.html>.

Now that we've talked about this more in depth over IRC, I guess there isn't much more to say from my side. Informal rs+, as WebKit2 isn't one of the things I'm most involved with.

[Hajime Morrita]

Comment on attachment 133739 [details]
Patch

rubberstamping.

[WebKit Review Bot]

Comment on attachment 133739 [details]
Patch

Clearing flags on attachment: 133739

Committed r112353: <http://trac.webkit.org/changeset/112353>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.