Bug 57789 - Move eventqueue from Document to ScriptExecutionContext so that it can be accessed from workers
: Move eventqueue from Document to ScriptExecutionContext so that it can be acc...
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Other Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
: 60299 61231
: 56787
  Show dependency treegraph
 
Reported: 2011-04-04 14:23 PST by
Modified: 2011-10-27 16:23 PST (History)


Attachments
Patch (8.04 KB, patch)
2011-04-04 14:42 PST, David Grogan
no flags Review Patch | Details | Formatted Diff | Diff
Fix typo (7.94 KB, patch)
2011-04-04 15:17 PST, David Grogan
no flags Review Patch | Details | Formatted Diff | Diff
Patch (7.96 KB, patch)
2011-04-04 18:33 PST, David Grogan
no flags Review Patch | Details | Formatted Diff | Diff
Patch (30.83 KB, patch)
2011-04-22 18:51 PST, David Grogan
no flags Review Patch | Details | Formatted Diff | Diff
fix style and build problems (25.54 KB, patch)
2011-04-22 19:27 PST, David Grogan
no flags Review Patch | Details | Formatted Diff | Diff
manual upload that includes all files (29.91 KB, patch)
2011-04-22 19:50 PST, David Grogan
no flags Review Patch | Details | Formatted Diff | Diff
Patch (11.88 KB, patch)
2011-04-27 20:19 PST, David Grogan
no flags Review Patch | Details | Formatted Diff | Diff
Patch (46.62 KB, patch)
2011-10-24 19:57 PST, David Grogan
no flags Review Patch | Details | Formatted Diff | Diff
Patch (47.62 KB, patch)
2011-10-24 20:06 PST, David Grogan
no flags Review Patch | Details | Formatted Diff | Diff
Patch (47.65 KB, patch)
2011-10-25 13:10 PST, David Grogan
no flags Review Patch | Details | Formatted Diff | Diff
Patch (50.36 KB, patch)
2011-10-26 14:36 PST, David Grogan
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (50.35 KB, patch)
2011-10-26 15:46 PST, David Grogan
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (50.25 KB, patch)
2011-10-27 12:10 PST, David Grogan
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-04-04 14:23:16 PST
Move eventqueue from Document to ScriptExecutionContext so that it can be accessed from workers
------- Comment #1 From 2011-04-04 14:42:36 PST -------
Created an attachment (id=88133) [details]
Patch
------- Comment #2 From 2011-04-04 14:47:35 PST -------
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.
------- Comment #3 From 2011-04-04 14:50:50 PST -------
Will take a look, but Alexey probably has opinions too.
------- Comment #4 From 2011-04-04 15:13:01 PST -------
Attachment 88133 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8336031
------- Comment #5 From 2011-04-04 15:17:23 PST -------
Created an attachment (id=88141) [details]
Patch
------- Comment #6 From 2011-04-04 16:44:47 PST -------
Wow that was r+'ed quickly.... I guess we didn't really want other comments?
------- Comment #7 From 2011-04-04 16:47:33 PST -------
Comments still welcome.
------- Comment #8 From 2011-04-04 17:00:01 PST -------
(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.

> 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.
------- Comment #9 From 2011-04-04 18:30:14 PST -------
(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.

>> 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
------- Comment #10 From 2011-04-04 18:33:14 PST -------
Created an attachment (id=88177) [details]
Patch
------- Comment #11 From 2011-04-05 09:08:56 PST -------
(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.
------- Comment #12 From 2011-04-05 09:31:33 PST -------
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.
------- Comment #13 From 2011-04-05 09:33:15 PST -------
(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.
------- Comment #14 From 2011-04-05 09:40:40 PST -------
(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.
------- Comment #15 From 2011-04-05 09:42:30 PST -------
(In reply to comment #11)
> (In reply to comment #9)
> > (From update of attachment 88141 [details] [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.
------- Comment #16 From 2011-04-05 11:12:44 PST -------
(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).
------- Comment #17 From 2011-04-05 12:07:07 PST -------
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.
------- Comment #18 From 2011-04-05 12:48:39 PST -------
(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).
------- Comment #19 From 2011-04-05 20:02:45 PST -------
(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.
------- Comment #20 From 2011-04-05 20:45:37 PST -------
(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)
------- Comment #21 From 2011-04-05 20:51:44 PST -------
Events and tasks aren't DOM objects, so they shouldn't be stopped when ActiveDOMObject is suspended. They need a separate mechanism.
------- Comment #22 From 2011-04-07 11:16:20 PST -------
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.
------- Comment #23 From 2011-04-22 18:51:40 PST -------
Created an attachment (id=90828) [details]
Patch
------- Comment #24 From 2011-04-22 18:54:29 PST -------
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.
------- Comment #25 From 2011-04-22 18:54:59 PST -------
(From update of attachment 90828 [details])
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?
------- Comment #26 From 2011-04-22 18:55:26 PST -------
(From update of attachment 90828 [details])
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?
------- Comment #27 From 2011-04-22 19:10:42 PST -------
Attachment 90828 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8498574
------- Comment #28 From 2011-04-22 19:27:11 PST -------
Created an attachment (id=90831) [details]
fix style and build problems
------- Comment #29 From 2011-04-22 19:50:47 PST -------
Created an attachment (id=90833) [details]
manual upload that includes all files

webkit-patch and git weren't playing nicely wrt EventQueue.cpp and DocumentEventQueue.cpp
------- Comment #30 From 2011-04-22 20:13:53 PST -------
Attachment 90833 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8503021
------- Comment #31 From 2011-04-22 20:37:13 PST -------
Attachment 90833 [details] did not build on win:
Build output: http://queues.webkit.org/results/8493710
------- Comment #32 From 2011-04-23 07:30:09 PST -------
Attachment 90833 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8497731
------- Comment #33 From 2011-04-27 20:19:45 PST -------
Created an attachment (id=91416) [details]
Patch
------- Comment #34 From 2011-04-27 20:37:16 PST -------
(In reply to comment #33)
> Created an attachment (id=91416) [details] [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.
------- Comment #35 From 2011-04-28 13:07:25 PST -------
(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?
------- Comment #36 From 2011-04-28 15:55:55 PST -------
(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?
------- Comment #37 From 2011-04-28 16:41:49 PST -------
(From update of attachment 91416 [details])
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?
------- Comment #38 From 2011-04-28 17:02:27 PST -------
(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.
------- Comment #39 From 2011-05-09 19:27:48 PST -------
(In reply to comment #37)
> (From update of attachment 91416 [details] [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.
------- Comment #40 From 2011-05-09 19:47:48 PST -------
(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() ?
------- Comment #41 From 2011-05-10 20:23:45 PST -------
(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()?
------- Comment #42 From 2011-05-11 08:55:27 PST -------
(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.
------- Comment #43 From 2011-05-13 13:13:52 PST -------
(From update of attachment 91416 [details])
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.
------- Comment #44 From 2011-10-24 19:57:44 PST -------
Created an attachment (id=112294) [details]
Patch
------- Comment #45 From 2011-10-24 20:06:53 PST -------
Created an attachment (id=112295) [details]
Patch
------- Comment #46 From 2011-10-24 20:09:54 PST -------
Dave L, could you take a look at this revived patch?
------- Comment #47 From 2011-10-24 23:58:11 PST -------
(From update of attachment 112295 [details])
Attachment 112295 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/10205846
------- Comment #48 From 2011-10-25 13:10:51 PST -------
Created an attachment (id=112384) [details]
Patch
------- Comment #49 From 2011-10-25 19:05:10 PST -------
(From update of attachment 112384 [details])
Attachment 112384 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/10211404
------- Comment #50 From 2011-10-26 13:42:06 PST -------
(From update of attachment 112384 [details])
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.
------- Comment #51 From 2011-10-26 14:36:21 PST -------
Created an attachment (id=112594) [details]
Patch
------- Comment #52 From 2011-10-26 14:39:24 PST -------
(In reply to comment #50)
> (From update of attachment 112384 [details] [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.
------- Comment #53 From 2011-10-26 15:03:11 PST -------
(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.

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.)
------- Comment #54 From 2011-10-26 15:06:28 PST -------
(From update of attachment 112594 [details])
Looks good (especially when the "virtual" is removed in that one place.
------- Comment #55 From 2011-10-26 15:46:41 PST -------
Created an attachment (id=112608) [details]
Patch for landing
------- Comment #56 From 2011-10-27 07:34:59 PST -------
(From update of attachment 112608 [details])
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
------- Comment #57 From 2011-10-27 12:10:39 PST -------
Created an attachment (id=112724) [details]
Patch for landing
------- Comment #58 From 2011-10-27 16:15:28 PST -------
(From update of attachment 112724 [details])
Clearing flags on attachment: 112724

Committed r98656: <http://trac.webkit.org/changeset/98656>
------- Comment #59 From 2011-10-27 16:15:37 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #60 From 2011-10-27 16:21:50 PST -------
*** Bug 61231 has been marked as a duplicate of this bug. ***