[webkit-reviews] review denied: [Bug 115332] [EFL][WK2] Separate dispatch context from WorkQueue. : [Attachment 204130] Patch

[bugzilla-daemon at webkit.org]

Christophe Dumez <dchris at gmail.com> has denied Byungwoo Lee
<bw80.lee at samsung.com>'s request for review:
Bug 115332: [EFL][WK2] Separate dispatch context from WorkQueue.
https://bugs.webkit.org/show_bug.cgi?id=115332

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

------- Additional Comments from Christophe Dumez <dchris at gmail.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=204130&action=review


Since you're touching the generic WK2 WorkQueue code, you'll likely need a WK2
owner to at least sign off on it.

> Source/WebKit2/ChangeLog:10
> +	   Previously, WorkQueue class have all context about dispatch.

"the WorkQueue class had all the context..."

> Source/WebKit2/ChangeLog:12
> +	   and those are accessed on the work queue thread through WorkQueue

"those were"

> Source/WebKit2/ChangeLog:16
> +	   complicates handling workqueue ref-counting and makes dangling

"causes dangling"

> Source/WebKit2/Platform/WorkQueue.h:58
>  #include <Ecore.h>

Do we still need this include?

> Source/WebKit2/Platform/WorkQueue.h:172
> +    DispatchQueue* m_dispatchQueue;

Maybe a comment to explain why this is a raw pointer (and not a OwnPtr for
e.g.)

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:2
> + * Copyright (C) 2012 Samsung Electronics. All rights reserved.

2012, 2013?

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:36
> +static const char wakeupThreadMessage = 'W';

"wakeUpThreadMessage"

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:57
> +PassOwnPtr<TimerWorkItem> TimerWorkItem::create(WorkQueue* workQueue, const
Function<void()>& function, double delay)

Let's include the unit in the "delay" argument name for clarity. e.g.
delaySeconds.

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:62
> +    double expireTime = currentTime() + delay;

expirationTime ?

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:63
> +    if (expireTime < 0)

How can this happen?

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:71
> +    , m_expireTime(expireTime)

m_expirationTime?

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:89
> +    m_threadLoop = true;

m_isThreadRunnning?
Could probably be initialized in the initializer list.

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:111
> +    if (!item)

Is this really normal? Maybe this can be replaced by an assertion?

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:118
> +void DispatchQueue::dispatchRelease()

Maybe something like "stopThread()" would be more easily understandable?

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:126
> +void DispatchQueue::registerSocketEventHandler(int fileDescriptor, const
Function<void()>& function)

I would rename this to setSocketEventHandler() since only one can be set.

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:128
> +    if (m_socketDescriptor != invalidSocketDescriptor)

Can probably be an assertion.

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:139
> +void DispatchQueue::unregisterSocketEventHandler(int fileDescriptor)

I would rename this to clearSocketEventHandler() and remove the argument.

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:150
> +    while (true) {

Would while(m_threadLoop) make more sense here? (i.e. help exit earlier).

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:190
> +	   // If a timer work item expired, dispatch the function of the work
item.

This comment is not needed.

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:202
> +	       if (read(m_readFromPipeDescriptor, &message, sizeof(char)) ==
-1)

char size is guaranteed to be 1 so we don't really need a sizeof.

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:215
> +    if (!item)

Why can this happen? Could this be replaced by an assertion?

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:221
> +    // m_timerWorkItems should be ordered by expire time.

"The items should be ordered by expiration time."

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:237
> +    delete dispatchQueue;

Maybe a comment to explain that the dispatchQueue needs to be destroyed in the
thread?

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:241
> +void DispatchQueue::wakeup()

wakeUpThread?

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:248
> +struct timeval* DispatchQueue::getNextTimeOut()

This should probably be const.

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:254
> +    static struct timeval timeValue;

I don't think we need the "struct" since this is C++.

> Source/WebKit2/Platform/efl/DispatchQueue.cpp:260
> +	   timeValue.tv_usec = static_cast<long>((timeOut - timeValue.tv_sec) *
1000000);

1000000 should probably be a global static const variable for readability. e.g.
"nanoSecondsPerSecond".

> Source/WebKit2/Platform/efl/DispatchQueue.h:52
> +    double expireTime() const { return m_expireTime; }

expirationTime

> Source/WebKit2/Platform/efl/DispatchQueue.h:53
> +    bool expired(double currentTime) const { return currentTime >=
m_expireTime; }

"hasExpired"

> Source/WebKit2/Platform/efl/DispatchQueue.h:80
> +    struct timeval* getNextTimeOut();

struct probably not needed.

> Source/WebKit2/PlatformEfl.cmake:2
> +    Platform/efl/DispatchQueue.cpp

All the other files end with "Efl", why not this one?