[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