Bug 23980 - WorkerRunLoop needs a way to run in a given mode similar to CFRunLoopInMode.
Summary: WorkerRunLoop needs a way to run in a given mode similar to CFRunLoopInMode.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: David Levin
URL:
Keywords:
Depends on: 23976
Blocks:
  Show dependency treegraph
 
Reported: 2009-02-16 14:52 PST by David Levin
Modified: 2009-02-19 14:46 PST (History)
1 user (show)

See Also:


Attachments
Proposed fix. (6.72 KB, patch)
2009-02-16 15:00 PST, David Levin
no flags Details | Formatted Diff | Diff
Proposed fix. (6.65 KB, patch)
2009-02-17 01:09 PST, David Levin
no flags Details | Formatted Diff | Diff
Proposed fix. (6.84 KB, patch)
2009-02-18 19:19 PST, David Levin
no flags Details | Formatted Diff | Diff
proposed fix. (6.73 KB, patch)
2009-02-19 00:07 PST, David Levin
ap: review-
Details | Formatted Diff | Diff
Proposed fix. (7.98 KB, patch)
2009-02-19 10:46 PST, David Levin
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Levin 2009-02-16 14:52:29 PST
It also needs this as a nested run loop support nested run loops.
Comment 1 David Levin 2009-02-16 15:00:03 PST
Created attachment 27712 [details]
Proposed fix.
Comment 2 Dmitry Titov 2009-02-17 00:13:24 PST
Perhaps preRunInMode and postRunInMode could be named differently - not by the time these methods are called at but by what they actually do, lets say ensureSharedTimer and releaseSharedTimer.

Also TaskWithMode could be just WorkerRunLoop::Task, seems there is no need to list its data members in the name.

Comment 3 David Levin 2009-02-17 00:40:32 PST
Comment on attachment 27712 [details]
Proposed fix.

Addressing dimich's suggestions.
Comment 4 David Levin 2009-02-17 01:09:57 PST
Created attachment 27720 [details]
Proposed fix.
Comment 5 David Levin 2009-02-18 19:19:35 PST
Created attachment 27775 [details]
Proposed fix.
Comment 6 David Levin 2009-02-18 23:58:17 PST
Comment on attachment 27775 [details]
Proposed fix.

I just saw some small mistakes with RefPtr that should be PassRefPtr.
Comment 7 David Levin 2009-02-19 00:07:31 PST
Created attachment 27780 [details]
proposed fix.
Comment 8 Alexey Proskuryakov 2009-02-19 03:31:12 PST
Comment on attachment 27780 [details]
proposed fix.

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.
Comment 9 David Levin 2009-02-19 10:46:10 PST
Created attachment 27795 [details]
Proposed fix.

Addressed comments.

> 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? 
Not implemented yet.  I'd like to discuss some details around your scenario.  I'll find you in irc or email you, but regardless as if needed, I'll do this in a future change.


> As commented in another bug, I wonder if we need waitForMessageFilteredWithTimeout.
I like this idea.  How about if I do it in another patch right after this? (I'm thinking about adding an Infinite constant that can be passed in and then just collapsing some of the if's in runInMode.  I'd also like to remove duplicate code in MessageQueue (as long as it is still readable).


> A brief description of the API in ChangeLog would be nice.
Added something that I like, but I may not have been brief enough ?
Comment 10 Alexey Proskuryakov 2009-02-19 11:09:47 PST
Comment on attachment 27795 [details]
Proposed fix.

Ok, let get this landed, and possibly refactor later, when all pieces of the puzzle are available.

r=me
Comment 11 David Levin 2009-02-19 14:46:46 PST
Commited as r41088.