[webkit-reviews] review granted: [Bug 24047] Need to simplify nested if's in WorkerRunLoop::runInMode : [Attachment 27840] Proposed fix.

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted David Levin
<levin at chromium.org>'s request for review:
Bug 24047: Need to simplify nested if's in WorkerRunLoop::runInMode
https://bugs.webkit.org/show_bug.cgi?id=24047

Attachment 27840: Proposed fix.
https://bugs.webkit.org/attachment.cgi?id=27840&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
> +    inline MessageQueueWaitResult
MessageQueue<DataType>::waitForMessageFilteredWithTimeout(DataType& result,
Predicate& predicate, double absoluteTime)
>      {
> -	   MutexLocker lock(m_mutex);
> +	   bool allowTimeOut = absoluteTime != infiniteTime();

Inconsistent naming here: "timeout" is a word, so it should be allowTimeout or
allowToTimeOut.

>	   bool timedOut = false;

This one is fine.

> +	   while (!m_killed  && !timedOut && (found =
m_queue.findIf(predicate)) == m_queue.end())

There are two spaces before &&.

> +	       timedOut = !m_condition.timedWait(m_mutex, absoluteTime) &&
allowTimeOut;

This looks suspicious. Can timedWait() return false for other reasons? If it
can, is the problem limited to cases where allowTimeOut is false?

> +    // Time is too far in the future (and would overflow int)  - wait
forever.
> +    if (absoluteTime - currentTime > static_cast<double>(INT_MAX) / 1000.0)
{
> +	   wait(mutex);
> +	   return true;
> +    }
>  
> +    double intervalMilliseconds = (absoluteTime - currentTime) * 1000.0;

Isn't this subject to rounding errors? I don't see what guarantees that
intervalMilliseconds won't overflow when converted to unsigned long.

> +    double absoluteTime = predicate.isDefaultMode() &&
m_sharedTimer->isActive() ? m_sharedTimer->fireTime() :
MessageQueue<RefPtr<Task> >::infiniteTime();

I think that parentheses around the ?: predicate would help readability.

r=me, but please consider the comments.