Bug 57789 – Move eventqueue from Document to ScriptExecutionContext so that it can be accessed from workers

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[David Grogan]

Move eventqueue from Document to ScriptExecutionContext so that it can be accessed from workers

[David Grogan]

Created attachment 88133 [details]
Patch

[David Grogan]

Mihai, I'm adding you because jorlow mentioned he'd discussed this with you.  Feel free to pass it on to someone else if you don't want to do it.

[Mihai Parparita]

Will take a look, but Alexey probably has opinions too.

[Early Warning System Bot]

Attachment 88133 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8336031

[David Grogan]

Created attachment 88141 [details]
Fix typo

[David Levin]

Wow that was r+'ed quickly.... I guess we didn't really want other comments?

[David Grogan]

Comments still welcome.

[David Levin]

Comment on attachment 88141 [details]
Fix typo

View in context: https://bugs.webkit.org/attachment.cgi?id=88141&action=review

> Source/WebCore/ChangeLog:5
> +        Move event queue from Document to ScriptExecutionContext so that it can be accessed from workers

This is what you are doing but a better comment answers why.

> Source/WebCore/ChangeLog:8
> +        Change EventQueue to lazily construct m_eventQueueTimer.

This looks like it should be a function level comment.

> Source/WebCore/ChangeLog:14
> +        No new tests, just a refactoring.

The refactoring seems odd given that EventQueue has all of this handling for Node which aren't available in Workers.

The member variable m_nodesWithQueuedScrollEvents; and the method enqueueScrollEvent are not applicable to Workers.

> Source/WebCore/ChangeLog:17
> +        (WebCore::Document::Document):

function level comments are encouraged in WebKit.  See other well written ChangeLog entries.

> Source/WebCore/dom/EventQueue.cpp:-23
> - * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 

Unnecessary whitespace changes are discouraged.

> Source/WebCore/dom/EventQueue.cpp:-47
> -    EventQueue* m_eventQueue;    

Unnecessary whitespace changes are discouraged.

> Source/WebCore/dom/EventQueue.cpp:68
> +        m_pendingEventTimer = adoptPtr(new EventQueueTimer(this, m_scriptExecutionContext));

It looks like you are moving this from EventQueue, but calling adoptPtr directly is a bad sign.

The constructor should be hidden and expose a static create method.

> Source/WebCore/dom/EventQueue.cpp:93
> +    if (m_queuedEvents.isEmpty() && m_pendingEventTimer)

How can you have an event in the queue but no timer?

> Source/WebCore/dom/EventQueue.cpp:100
> +    ASSERT(m_pendingEventTimer);

Why was this assert added? (Good to comment in ChangeLog about this.)

> Source/WebCore/dom/ScriptExecutionContext.h:59
> +    class EventQueue;

This is out of order.

[David Grogan]

Comment on attachment 88141 [details]
Fix typo

View in context: https://bugs.webkit.org/attachment.cgi?id=88141&action=review

>> Source/WebCore/ChangeLog:5
>> +        Move event queue from Document to ScriptExecutionContext so that it can be accessed from workers
> 
> This is what you are doing but a better comment answers why.

The idea was

What: Move event queue from Document to ScriptExecutionContext
Why: so that it can be accessed from workers

Though I now added that _IndexedDB_ needs to access it from workers.

>> Source/WebCore/ChangeLog:8
>> +        Change EventQueue to lazily construct m_eventQueueTimer.
> 
> This looks like it should be a function level comment.

Moved.

>> Source/WebCore/ChangeLog:14
>> +        No new tests, just a refactoring.
> 
> The refactoring seems odd given that EventQueue has all of this handling for Node which aren't available in Workers.
> 
> The member variable m_nodesWithQueuedScrollEvents; and the method enqueueScrollEvent are not applicable to Workers.

Yeah, that's awkward, I didn't realize those weren't applicable to workers though it seems obvious in hindsight.  I'll either move the scroll stuff to Document or, more likely, to a subclass of EventQueue that is instantiated in Document.  Or any other suggestion you have.

>> Source/WebCore/dom/EventQueue.cpp:-23
>> - * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 
> 
> Unnecessary whitespace changes are discouraged.

Didn't realize.

>> Source/WebCore/dom/EventQueue.cpp:68
>> +        m_pendingEventTimer = adoptPtr(new EventQueueTimer(this, m_scriptExecutionContext));
> 
> It looks like you are moving this from EventQueue, but calling adoptPtr directly is a bad sign.
> 
> The constructor should be hidden and expose a static create method.

Moved.

>> Source/WebCore/dom/EventQueue.cpp:93
>> +    if (m_queuedEvents.isEmpty() && m_pendingEventTimer)
> 
> How can you have an event in the queue but no timer?

You can't.  This function could be called before any events are enqueued though.

>> Source/WebCore/dom/EventQueue.cpp:100
>> +    ASSERT(m_pendingEventTimer);
> 
> Why was this assert added? (Good to comment in ChangeLog about this.)

Because I think this function can't be called when m_pendingEventTimer is null.  Such an occurrence indicates a logic failure that I want to know about.  Granted, the next ASSERT would fail but the message produced by this one would be clearer.

>> Source/WebCore/dom/ScriptExecutionContext.h:59
>> +    class EventQueue;
> 
> This is out of order.

fixed

[David Grogan]

Created attachment 88177 [details]
Patch

[Dmitry Titov]

(In reply to comment #9)
> (From update of attachment 88141 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88141&action=review
> 
> >> Source/WebCore/ChangeLog:5
> >> +        Move event queue from Document to ScriptExecutionContext so that it can be accessed from workers
> > 
> > This is what you are doing but a better comment answers why.
> 
> The idea was
> 
> What: Move event queue from Document to ScriptExecutionContext
> Why: so that it can be accessed from workers
> 
> Though I now added that _IndexedDB_ needs to access it from workers.

It doesn't answer David's question I think. Yes, IndexedDB currently uses this event queue on Document.

However, Workers (unlike Document) already have their own event (task) queue which they completely control. It is not clear why do we need another generic queue in addition to that. Having 2 queues will sooner or later cause problems with ordering of tasks, termination, suspension and other things that all require some control on how queues operate.

I think there is a bigger design question here, I understand you are omving some code from Document to Worker but it doesn't always mean the code should be just copied. "Why" would have to include some thought on why existing mechanisms are not adequate and can't be improved, for example.

I'm going to r- the patch for now as it's not clear why exactly it is needed. It very well might be, but there is no good explanation at least.

[David Levin]

Also, this issue isn't addressed in the patch. (I'm reluctant to let it go through because without this because then the motivation to clean it up goes away. Maybe it should be a pre-step to this patch.)

>> Source/WebCore/ChangeLog:14
>> +        No new tests, just a refactoring.
> 
> The refactoring seems odd given that EventQueue has all of this handling for Node which aren't available in Workers.
> 
> The member variable m_nodesWithQueuedScrollEvents; and the method enqueueScrollEvent are not applicable to Workers.

Yeah, that's awkward, I didn't realize those weren't applicable to workers though it seems obvious in hindsight.  I'll either move the scroll stuff to Document or, more likely, to a subclass of EventQueue that is instantiated in Document.  Or any other suggestion you have.

[Mihai Parparita]

(In reply to comment #12)
> Yeah, that's awkward, I didn't realize those weren't applicable to workers though it seems obvious in hindsight.  I'll either move the scroll stuff to Document or, more likely, to a subclass of EventQueue that is instantiated in Document.  Or any other suggestion you have.

I don't think moving them into Document is a good idea, it's already got too many methods and members. A DocumentEventQueue subclass seems like the most reasonable thing.

[David Grogan]

(In reply to comment #12)
> Also, this issue isn't addressed in the patch. (I'm reluctant to let it go through because without this because then the motivation to clean it up goes away.

Of course.  I was waiting to see if you wanted to express opinions.

> Maybe it should be a pre-step to this patch.)

Is that what you'd prefer?

> 
> >> Source/WebCore/ChangeLog:14
> >> +        No new tests, just a refactoring.
> > 
> > The refactoring seems odd given that EventQueue has all of this handling for Node which aren't available in Workers.
> > 
> > The member variable m_nodesWithQueuedScrollEvents; and the method enqueueScrollEvent are not applicable to Workers.
> 
> Yeah, that's awkward, I didn't realize those weren't applicable to workers though it seems obvious in hindsight.  I'll either move the scroll stuff to Document or, more likely, to a subclass of EventQueue that is instantiated in Document.  Or any other suggestion you have.

[David Grogan]

(In reply to comment #11)
> (In reply to comment #9)
> > (From update of attachment 88141 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=88141&action=review
> > 
> > >> Source/WebCore/ChangeLog:5
> > >> +        Move event queue from Document to ScriptExecutionContext so that it can be accessed from workers
> > > 
> > > This is what you are doing but a better comment answers why.
> > 
> > The idea was
> > 
> > What: Move event queue from Document to ScriptExecutionContext
> > Why: so that it can be accessed from workers
> > 
> > Though I now added that _IndexedDB_ needs to access it from workers.
> 
> It doesn't answer David's question I think. Yes, IndexedDB currently uses this event queue on Document.
> 
> However, Workers (unlike Document) already have their own event (task) queue which they completely control. It is not clear why do we need another generic queue in addition to that. Having 2 queues will sooner or later cause problems with ordering of tasks, termination, suspension and other things that all require some control on how queues operate.
> 
> I think there is a bigger design question here, I understand you are omving some code from Document to Worker but it doesn't always mean the code should be just copied. "Why" would have to include some thought on why existing mechanisms are not adequate and can't be improved, for example.
> 
> I'm going to r- the patch for now as it's not clear why exactly it is needed. It very well might be, but there is no good explanation at least.

That's reasonable.  Thanks for all the info.  I'll look into the worker-specific event queue and try to determine if this new one is necessary.

[David Levin]

(In reply to comment #14)
> (In reply to comment #12)
> > Also, this issue isn't addressed in the patch. (I'm reluctant to let it go through because without this because then the motivation to clean it up goes away.
> 
> Of course.  I was waiting to see if you wanted to express opinions.
> 
> > Maybe it should be a pre-step to this patch.)
> 
> Is that what you'd prefer?
Yes. Smaller patches are easier to review in general. So when something can be separated out easily, it is nice to make it a separate patch. Based on past experience, a small patch get through faster than a larger patch (with better quality -- though the overall time may be slower for a few smaller patches vs a large one).

[Dmitry Titov]

Looking at things like a tight 'dispatchEvent()' loop in EventQueue::pendingEventTimerFired(), I feel even more hesitation that this is the right move for Workers. For example, that loop will need to interact with a message loop of the Worker to figure out when to stop (perhaps looking at WorkerContext->isClosing())

We probably just need to post async events to the main Worker's MessageLoop as Tasks, rather then have a separate mechanism for queuing/dispatching storage events. We already dispatch XHR, File API, SQL DB, MessagePort and other events using this mechanism.

That would also help avoid refactoring scrolling and other Document-level implementation out of the EventQueue.

[David Levin]

(In reply to comment #17)
> Looking at things like a tight 'dispatchEvent()' loop in EventQueue::pendingEventTimerFired(), I feel even more hesitation that this is the right move for Workers. For example, that loop will need to interact with a message loop of the Worker to figure out when to stop (perhaps looking at WorkerContext->isClosing())
> 
> We probably just need to post async events to the main Worker's MessageLoop as Tasks, rather then have a separate mechanism for queuing/dispatching storage events. We already dispatch XHR, File API, SQL DB, MessagePort and other events using this mechanism.
> 
> That would also help avoid refactoring scrolling and other Document-level implementation out of the EventQueue.

Note that the task mechanism is already in ScriptExecutionContext (so it is usable from both Workers and Documents).

[David Grogan]

(In reply to comment #17)
> Looking at things like a tight 'dispatchEvent()' loop in EventQueue::pendingEventTimerFired(), I feel even more hesitation that this is the right move for Workers. For example, that loop will need to interact with a message loop of the Worker to figure out when to stop (perhaps looking at WorkerContext->isClosing())
> 
> We probably just need to post async events to the main Worker's MessageLoop as Tasks, rather then have a separate mechanism for queuing/dispatching storage events.

I'm going to pursue this but will need to add something like ScriptExecutionContext::cancelTask to get the functionality provided by EventQueue::cancelEvent.

> We already dispatch XHR, File API, SQL DB, MessagePort and other events using this mechanism.
> 
> That would also help avoid refactoring scrolling and other Document-level implementation out of the EventQueue.

[Mihai Parparita]

(In reply to comment #19)
> > We probably just need to post async events to the main Worker's MessageLoop as Tasks, rather then have a separate mechanism for queuing/dispatching storage events.
> 
> I'm going to pursue this but will need to add something like ScriptExecutionContext::cancelTask to get the functionality provided by EventQueue::cancelEvent.

I wasn't actually aware of the ScriptExecutionTask functionality when I added EventQueue, you may want to consider re-implementing EventQueue as a wrapper around it that (assuming it provides the same guarantees, e.g. suspend/resume that it gets via DOMTimer/ActiveDOMObject)

[Alexey Proskuryakov]

Events and tasks aren't DOM objects, so they shouldn't be stopped when ActiveDOMObject is suspended. They need a separate mechanism.

[Dmitry Titov]

If Events are posted as Tasks into WorkerRunLoop, they will follow the same rules as other Tasks that are already there. FWIW, Workers do not have suspend/resume semantics at the moment, so the corresponding behavior for them can be solved later, when and if this functionality is added. There are more places in workers code that will have to address suspension if it is to be implemented, most likely on WorkerRunLoop level.

The Workers do implement termination behavior however, which has a few scenarios distinct from Document:
- the page that spawned the worker closes
- the script on the page that spawned the worker calls worker.terminate()
- the script on the page that spawned the worker looses all references to it, including event handlers
- the worker calls close() inside itself
- there is a script error inside worker

For some of them, the exact sequence is determined by spec - for example, if there are 2 events A and B queued, but during A worker calls close(), that event handler completes but event B is never dispatched.

It is good to have tests on new async events in those termination scenarios.

[David Grogan]

Created attachment 90828 [details]
Patch

[WebKit Review Bot]

Attachment 90828 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 4 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Grogan]

Comment on attachment 90828 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=90828&action=review

This isn't complete yet but I'm hoping to get some feedback before I go further, in case I totally missed the boat.

> Source/WebCore/dom/WorkerEventQueue.cpp:57
> +        if (eventTarget->toDOMWindow())

Can this happen in a worker?

[David Grogan]

Comment on attachment 90828 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=90828&action=review

This isn't complete yet but I'm hoping to get some feedback before I go further, in case I totally missed the boat.

>> Source/WebCore/dom/WorkerEventQueue.cpp:57
>> +        if (eventTarget->toDOMWindow())
> 
> Can this happen in a worker?

Can this happen in a worker?

[Early Warning System Bot]

Attachment 90828 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8498574

[David Grogan]

Created attachment 90831 [details]
fix style and build problems

[David Grogan]

Created attachment 90833 [details]
manual upload that includes all files

webkit-patch and git weren't playing nicely wrt EventQueue.cpp and DocumentEventQueue.cpp

[Early Warning System Bot]

Attachment 90833 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8503021

[Build Bot]

Attachment 90833 [details] did not build on win:
Build output: http://queues.webkit.org/results/8493710

[Collabora GTK+ EWS bot]

Attachment 90833 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8497731

[David Grogan]

Created attachment 91416 [details]
Patch

[David Grogan]

(In reply to comment #33)
> Created an attachment (id=91416) [details]
> Patch

Dmitry, I think this is what we talked about the other day.

I get a couple of weird layout test failures, though, so I must be misunderstanding something.

If I run storage/domstorage/events/case-sensitive.html by itself it passes.  If I run it as part of a sequence, it fails with:
 Reset storage event list
 storageEventList = new Array()
 storage.foo = 'test'
-PASS storageEventList.length is 0
+FAIL storageEventList.length should be 0. Was 1.
 storage.foo = 'TEST'
-PASS storageEventList.length is 1
+FAIL storageEventList.length should be 1. Was 2.


storage/domstorage/events/documentURI.html has a similar failure under similar circumstances.

[Dmitry Titov]

(In reply to comment #34)

I think the tests in question are faulty (flaky). They assume that timer tasks (generated by setTimeout()) and storage event tasks are queued into the same UA task queue. However, the HTML spec specifically says the storage events use "DOM manipulation" task source and timer events use "timer task" task source.

(see http://dev.w3.org/html5/webstorage/#event-storage and http://www.whatwg.org/specs/web-apps/current-work/multipage/timers.html and search for 'source')

The task source (http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis.html#task-source) specifically regulates whether or not the UA guarantees in-order execution - for the tasks in the same task source it is guaranteed that they will be executed in order, while between task sources it can be random.

The test contains the code below that will quickly exit w/o waiting for events to be dispatched if the timeout message is dispatched before the storage message is. Since timeouts are clamped at 10ms, the test can be really flakey.

storageEventList = new Array();
window.onstorage = function (e) {
    window.parent.storageEventList.push(e);
}

// do somethign with storage...
 runAfterStorageEvents(myCallback);

function runAfterStorageEvents(callback) {
    var currentCount = storageEventList.length;
    function onTimeout() {
        if (currentCount != storageEventList.length)
            runAfterStorageEvents(callback);
        else
            callback();
    }
    setTimeout(onTimeout, 0);  <-- if this fires before onstorage event, there is no wait.
}

The runAfterStorageEvents() function has to be rewritten to wait for specified expected number of storage events, rather then for a delta in event count that happened to squeeze between timer callbacks.

Does it make sense?

[David Grogan]

(In reply to comment #35)

This makes a bunch of sense.  I'll fix the tests.

1) Where can I find evidence of the task queue concept in our code?  My naive reading of our usage of MessageLoop suggests that there really is just one queue.

2) Which does this mean?
> timeouts are clamped at 10ms
A timeout task specified to run after 0ms will wait at _most_ 10ms or at _least_ 10ms?

Thanks a lot for digging into this.

> 
> I think the tests in question are faulty (flaky). They assume that timer tasks (generated by setTimeout()) and storage event tasks are queued into the same UA task queue. However, the HTML spec specifically says the storage events use "DOM manipulation" task source and timer events use "timer task" task source.
> 
> (see http://dev.w3.org/html5/webstorage/#event-storage and http://www.whatwg.org/specs/web-apps/current-work/multipage/timers.html and search for 'source')
> 
> The task source (http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis.html#task-source) specifically regulates whether or not the UA guarantees in-order execution - for the tasks in the same task source it is guaranteed that they will be executed in order, while between task sources it can be random.
> 
> The test contains the code below that will quickly exit w/o waiting for events to be dispatched if the timeout message is dispatched before the storage message is. Since timeouts are clamped at 10ms, the test can be really flakey.
> 
> storageEventList = new Array();
> window.onstorage = function (e) {
>     window.parent.storageEventList.push(e);
> }
> 
> // do somethign with storage...
>  runAfterStorageEvents(myCallback);
> 
> function runAfterStorageEvents(callback) {
>     var currentCount = storageEventList.length;
>     function onTimeout() {
>         if (currentCount != storageEventList.length)
>             runAfterStorageEvents(callback);
>         else
>             callback();
>     }
>     setTimeout(onTimeout, 0);  <-- if this fires before onstorage event, there is no wait.
> }
> 
> The runAfterStorageEvents() function has to be rewritten to wait for specified expected number of storage events, rather then for a delta in event count that happened to squeeze between timer callbacks.
> 
> Does it make sense?

[Mihai Parparita]

Comment on attachment 91416 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=91416&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

If this is purely a refactoring with no visible/testable functional changes, then please add a line in the ChangeLog saying that, instead of leaving this in.

> Source/WebCore/dom/Document.cpp:1708
> +//    m_eventQueue->cancelQueuedEvents();

Please don't leave in commented out code (assuming it's necessary to remove this line - why?)

> Source/WebCore/dom/EventQueue.cpp:30
> +#include "CrossThreadTask.h"

What do you use from this #include?

> Source/WebCore/dom/EventQueue.cpp:67
> +    void dispatchEvent(ScriptExecutionContext*, PassRefPtr<Event> event)

Add an empty newline between methods.

> Source/WebCore/dom/EventQueue.cpp:84
> +      , m_scrollEvent(scrollEvent)

Do you actually need this, or can you just check if the type is scroll event?

> Source/WebCore/dom/EventQueue.cpp:102
> +    m_scriptExecutionContext->postTask(EventDispatcherTask::create(event, isScrollEvent, this));

Does this have the same semantics as DOMTimer? Enqueued events in a document should not fire while modal dialogs are up (which DOMTimer gets as a side effect of being an ActiveDOMObject). fast/events/scroll-event-during-modal-dialog.html is a layout test that checks for this behavior.

> Source/WebCore/dom/EventQueue.h:55
> +    void removeScrollEvent(PassRefPtr<Event>);

This should be private.

> Source/WebCore/dom/EventQueue.h:68
> +    friend class EventQueueTimer;

This is no longer necessary, you're removed EventQueueTimer.

> Source/WebCore/dom/EventQueue.h:69
> +    class EventDispatcherTask;

Why is this forward declaration necessary?

[Dmitry Titov]

(In reply to comment #36)
> (In reply to comment #35)
> 
> This makes a bunch of sense.  I'll fix the tests.
> 
> 1) Where can I find evidence of the task queue concept in our code?  My naive reading of our usage of MessageLoop suggests that there really is just one queue.

There are no explicit queues, but if you look at MainthreadMac.mm for exampel, you will see that timer events are scheduled via posting a new timer to the run loop, while other tasks are scheduled via [NSObject performSelector:onThread:]

this sort of creates 2 queues - one for timers and one for other events.

> 
> 2) Which does this mean?
> > timeouts are clamped at 10ms
> A timeout task specified to run after 0ms will wait at _most_ 10ms or at _least_ 10ms?

at_least. See DOMTimer.cpp.

[David Grogan]

(In reply to comment #37)
> (From update of attachment 91416 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91416&action=review
> 
> > Source/WebCore/dom/EventQueue.cpp:102
> > +    m_scriptExecutionContext->postTask(EventDispatcherTask::create(event, isScrollEvent, this));
> 
> Does this have the same semantics as DOMTimer? Enqueued events in a document should not fire while modal dialogs are up (which DOMTimer gets as a side effect of being an ActiveDOMObject). fast/events/scroll-event-during-modal-dialog.html is a layout test that checks for this behavior.

fast/events/scroll-event-during-modal-dialog.html passes with this task-posting implementation; I suppose there's another mechanism that prevents the events from firing.

[David Grogan]

(In reply to comment #19)
> (In reply to comment #17)
> I'm going to pursue this but will need to add something like ScriptExecutionContext::cancelTask to get the functionality provided by EventQueue::cancelEvent.

ScriptExecutionContext::cancelTask should be straightforward for non-chromium ports: there's a cancelCallOnMainThread() implemented alongside callOnMainThread().

Chromium has no implementation for cancelCallOnMainThread(), though.  MessageLoop appears to not support such behavior.  Is there no way to cancel a task once it has been sent to MessageLoop::postTask() ?

[David Grogan]

(In reply to comment #40)
> (In reply to comment #19)
> > (In reply to comment #17)
> > I'm going to pursue this but will need to add something like ScriptExecutionContext::cancelTask to get the functionality provided by EventQueue::cancelEvent.
> 
> ScriptExecutionContext::cancelTask should be straightforward for non-chromium ports: there's a cancelCallOnMainThread() implemented alongside callOnMainThread().
> 
> Chromium has no implementation for cancelCallOnMainThread(), though.  MessageLoop appears to not support such behavior.  Is there no way to cancel a task once it has been sent to MessageLoop::postTask() ?

+ajwong as someone who's been monkeying around in message_loop lately.

Albert, can you shed any light on how difficult it would be to add something like MessageLoop::cancelTask()?

[Albert J. Wong]

(In reply to comment #41)
> (In reply to comment #40)
> > (In reply to comment #19)
> > > (In reply to comment #17)
> > > I'm going to pursue this but will need to add something like ScriptExecutionContext::cancelTask to get the functionality provided by EventQueue::cancelEvent.
> > 
> > ScriptExecutionContext::cancelTask should be straightforward for non-chromium ports: there's a cancelCallOnMainThread() implemented alongside callOnMainThread().
> > 
> > Chromium has no implementation for cancelCallOnMainThread(), though.  MessageLoop appears to not support such behavior.  Is there no way to cancel a task once it has been sent to MessageLoop::postTask() ?
> 
> +ajwong as someone who's been monkeying around in message_loop lately.
> 
> Albert, can you shed any light on how difficult it would be to add something like MessageLoop::cancelTask()?

Putting it directly inside MessageLoop is fairly difficult.  I'm also not 100% certain what the semantics should be.  Are you allowed to call cancelTask from a different thread from what the MessageLoop is using?  If so, what do you do if the task is currently executing?  Blocking isn't really a good idea...

In Chromium, for the single threaded case (where you know that the task is getting posted, canceled, and executed, all from the same thread), then the general solution is to use something like a ScopedRunnableMethodFactory where the deletion of the factory cancels all tasks that were created by it.  We're also going to soon add support for creating a task on a WeakPtr<> to an object so that when the WeakPtr is invalidated, the task will No-Op itself.  But this all only works sanely from the single threaded perspective.  I don't think anyone has solved multi-threaded cancelation yet.

[David Grogan]

Comment on attachment 91416 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=91416&action=review

>> Source/WebCore/ChangeLog:8
>> +        No new tests. (OOPS!)
> 
> If this is purely a refactoring with no visible/testable functional changes, then please add a line in the ChangeLog saying that, instead of leaving this in.

Done.

>> Source/WebCore/dom/Document.cpp:1708
>> +//    m_eventQueue->cancelQueuedEvents();
> 
> Please don't leave in commented out code (assuming it's necessary to remove this line - why?)

fixed

>> Source/WebCore/dom/EventQueue.cpp:30
>> +#include "CrossThreadTask.h"
> 
> What do you use from this #include?

Remnant of some stuff I'm not using anymore.  Removed.

>> Source/WebCore/dom/EventQueue.cpp:67
>> +    void dispatchEvent(ScriptExecutionContext*, PassRefPtr<Event> event)
> 
> Add an empty newline between methods.

done

>> Source/WebCore/dom/EventQueue.cpp:84
>> +      , m_scrollEvent(scrollEvent)
> 
> Do you actually need this, or can you just check if the type is scroll event?

Nope, don't need it.  Changed.

>> Source/WebCore/dom/EventQueue.h:55
>> +    void removeScrollEvent(PassRefPtr<Event>);
> 
> This should be private.

done

>> Source/WebCore/dom/EventQueue.h:68
>> +    friend class EventQueueTimer;
> 
> This is no longer necessary, you're removed EventQueueTimer.

Removed.

>> Source/WebCore/dom/EventQueue.h:69
>> +    class EventDispatcherTask;
> 
> Why is this forward declaration necessary?

It wasn't. But it is necessary in the new patch.

[David Grogan]

Created attachment 112294 [details]
Patch

[David Grogan]

Created attachment 112295 [details]
Patch

[David Grogan]

Dave L, could you take a look at this revived patch?

[Daniel Bates]

Comment on attachment 112295 [details]
Patch

Attachment 112295 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/10205846

[David Grogan]

Created attachment 112384 [details]
Patch

[Daniel Bates]

Comment on attachment 112384 [details]
Patch

Attachment 112384 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/10211404

[David Levin]

Comment on attachment 112384 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112384&action=review

Please fix the builds that failed and address the few issues that I had.

Thanks!

> Source/WebCore/dom/Document.h:36
> +#include "DocumentEventQueue.h"

Won't a fwd decl of DocumentEventQueue suffice (just as EventQueue did before)?

> Source/WebCore/dom/DocumentEventQueue.h:57
> +    void close();

Add virtual

> Source/WebCore/workers/WorkerEventQueue.h:51
> +    void close();

add virtual on all of these

> Source/WebCore/workers/WorkerEventQueue.h:54
> +    virtual void removeEvent(Event*);

Why is this virtual? (Who overrides it?)

Similarly why do we have a protected section? Who derives from this class?

If that happens in a later patch, perhaps add the virtual and protected then and for now leave this as private and the method as non-virtual.

[David Grogan]

Created attachment 112594 [details]
Patch

[David Levin]

(In reply to comment #50)
> (From update of attachment 112384 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112384&action=review
> 
> > Source/WebCore/dom/Document.h:36
> > +#include "DocumentEventQueue.h"
> 
> Won't a fwd decl of DocumentEventQueue suffice (just as EventQueue did before)?

This doesn't appear to be addressed.

> > Source/WebCore/workers/WorkerEventQueue.h:54
> > +    virtual void removeEvent(Event*);
> 
> Why is this virtual? (Who overrides it?)

Ditto.

[David Grogan]

Comment on attachment 112384 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112384&action=review

>>> Source/WebCore/dom/Document.h:36
>>> +#include "DocumentEventQueue.h"
>> 
>> Won't a fwd decl of DocumentEventQueue suffice (just as EventQueue did before)?
> 
> This doesn't appear to be addressed.

The problem is the covariant return type on eventQueue().  The compiler has to see that DocumentEventQueue inherits from EventQueue.

Can I forward-declare that relationship somehow?  clang didn't like my feeble attempts.

>>> Source/WebCore/workers/WorkerEventQueue.h:54
>>> +    virtual void removeEvent(Event*);
>> 
>> Why is this virtual? (Who overrides it?)
>> 
>> Similarly why do we have a protected section? Who derives from this class?
>> 
>> If that happens in a later patch, perhaps add the virtual and protected then and for now leave this as private and the method as non-virtual.
> 
> Ditto.

Nothing derives from this class, the protected section and virtual keyword are remnants from a previous patch, thanks for catching it.  (I just missed removing this 'virtual' in the current patch, will re-upload.)

[David Levin]

Comment on attachment 112594 [details]
Patch

Looks good (especially when the "virtual" is removed in that one place.

[David Grogan]

Created attachment 112608 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 112608 [details]
Patch for landing

Rejecting attachment 112608 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
 -- saving rejects to file Source/WebCore/storage/IDBDatabase.cpp.rej
patching file Source/WebCore/storage/IDBRequest.cpp
patching file Source/WebCore/storage/IDBTransaction.cpp
patching file Source/WebCore/workers/WorkerContext.cpp
patching file Source/WebCore/workers/WorkerContext.h
patching file Source/WebCore/workers/WorkerEventQueue.cpp
patching file Source/WebCore/workers/WorkerEventQueue.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/10231142

[David Grogan]

Created attachment 112724 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 112724 [details]
Patch for landing

Clearing flags on attachment: 112724

Committed r98656: <http://trac.webkit.org/changeset/98656>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[David Grogan]

*** Bug 61231 has been marked as a duplicate of this bug. ***