[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 #25 from YoungTaeck Song <youngtaeck.song at samsung.com>  2012-01-03 04:42:52 PST ---
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.

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