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

[bugzilla-daemon at webkit.org]

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


Leandro Pereira <leandro at profusion.mobi> changed:

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




--- Comment #21 from Leandro Pereira <leandro at profusion.mobi>  2011-08-05 10:42:12 PST ---
(From update of attachment 103044)
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.

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