[webkit-reviews] review denied: [Bug 23980] WorkerRunLoop needs a way to run in a given mode similar to CFRunLoopInMode. : [Attachment 27780] proposed fix.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Feb 19 03:31:12 PST 2009


Alexey Proskuryakov <ap at webkit.org> has denied David Levin
<levin at chromium.org>'s request for review:
Bug 23980: WorkerRunLoop needs a way to run in a given mode similar to
CFRunLoopInMode.
https://bugs.webkit.org/show_bug.cgi?id=23980

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

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
Some comments:

> +void WorkerRunLoop::setSharedTimer()
> +{
> +    if (!m_nestedCount)
> +	  
threadGlobalData().threadTimers().setSharedTimer(m_sharedTimer.get());
> +    m_nestedCount++;
> +}

In lazy reading mode, I don't understand this function. In which sense does it
"set" the timer? Also, it seems a bit strange that worker nesting count is
incremented in some timer-related function (even though it's only necessary for
timer management).

Should it be called just enterLoop() or something? Should RAII be used for
exitLoop for extra safety?

> +	   result = runInMode(context, NULL);

We use 0, not NULL in C++ code. But I also don't understand why String
construction isn't ambiguous here - it has lots of one-argument constructors. I
think that a named constant for default mode would help.

> +    MessageQueueWaitResult result = runInMode(context, &modePredicate);

Passing by reference is usual in C++.

Generally, I'm not sure how a task that can be served in multiple modes will
work. Is this not implemented yet, or am I missing something? E.g. a loading
progress task should be delivered both in non-nested loops, and in nested XHR
loop.

As commented in another bug, I wonder if we need
waitForMessageFilteredWithTimeout.

A brief description of the API in ChangeLog would be nice.

> +    postTaskForMode(task, String());

Again, this calls for a named constant.

It's sad that we need to wrap the task in another object just to add a mode
data member to it. I don't have any suggestion, and don't ask for this to be
fixed now, but it's something to keep in mind.

This looks nearly ready to go, but I think that there were enough comments for
another round.


More information about the webkit-reviews mailing list