Bug 23980 – WorkerRunLoop needs a way to run in a given mode similar to CFRunLoopInMode.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[David Levin]

It also needs this as a nested run loop support nested run loops.

[David Levin]

Created attachment 27712 [details]
Proposed fix.

[Dmitry Titov]

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.

[David Levin]

Comment on attachment 27712 [details]
Proposed fix.

Addressing dimich's suggestions.

[David Levin]

Created attachment 27720 [details]
Proposed fix.

[David Levin]

Created attachment 27775 [details]
Proposed fix.

[David Levin]

Comment on attachment 27775 [details]
Proposed fix.

I just saw some small mistakes with RefPtr that should be PassRefPtr.

[David Levin]

Created attachment 27780 [details]
proposed fix.

[Alexey Proskuryakov]

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.

[David Levin]

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 ?

[Alexey Proskuryakov]

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

[David Levin]

Commited as r41088.