Bug 57789 - Move eventqueue from Document to ScriptExecutionContext so that it can be accessed from workers
Summary: Move eventqueue from Document to ScriptExecutionContext so that it can be acc...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: David Grogan
URL:
Keywords:
: 61231 (view as bug list)
Depends on: 60299 61231
Blocks: 56787
  Show dependency treegraph
 
Reported: 2011-04-04 14:23 PDT by David Grogan
Modified: 2011-10-27 16:23 PDT (History)
12 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description David Grogan 2011-04-04 14:23:16 PDT
Move eventqueue from Document to ScriptExecutionContext so that it can be accessed from workers
Comment 1 David Grogan 2011-04-04 14:42:36 PDT
Created attachment 88133 [details]
Patch
Comment 2 David Grogan 2011-04-04 14:47:35 PDT
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 Mihai Parparita 2011-04-04 14:50:50 PDT
Will take a look, but Alexey probably has opinions too.
Comment 4 Early Warning System Bot 2011-04-04 15:13:01 PDT
Attachment 88133 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8336031
Comment 5 David Grogan 2011-04-04 15:17:23 PDT
Created attachment 88141 [details]
Fix typo
Comment 6 David Levin 2011-04-04 16:44:47 PDT
Wow that was r+'ed quickly.... I guess we didn't really want other comments?
Comment 7 David Grogan 2011-04-04 16:47:33 PDT
Comments still welcome.
Comment 8 David Levin 2011-04-04 17:00:01 PDT
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.
Comment 9 David Grogan 2011-04-04 18:30:14 PDT
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
Comment 10 David Grogan 2011-04-04 18:33:14 PDT
Created attachment 88177 [details]
Patch
Comment 11 Dmitry Titov 2011-04-05 09:08:56 PDT
(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.
Comment 12 David Levin 2011-04-05 09:31:33 PDT
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 Mihai Parparita 2011-04-05 09:33:15 PDT
(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 David Grogan 2011-04-05 09:40:40 PDT
(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 David Grogan 2011-04-05 09:42:30 PDT
(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.
Comment 16 David Levin 2011-04-05 11:12:44 PDT
(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 Dmitry Titov 2011-04-05 12:07:07 PDT
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 David Levin 2011-04-05 12:48:39 PDT
(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 David Grogan 2011-04-05 20:02:45 PDT
(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 Mihai Parparita 2011-04-05 20:45:37 PDT
(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 Alexey Proskuryakov 2011-04-05 20:51:44 PDT
Events and tasks aren't DOM objects, so they shouldn't be stopped when ActiveDOMObject is suspended. They need a separate mechanism.
Comment 22 Dmitry Titov 2011-04-07 11:16:20 PDT
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 David Grogan 2011-04-22 18:51:40 PDT
Created attachment 90828 [details]
Patch
Comment 24 WebKit Review Bot 2011-04-22 18:54:29 PDT
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 David Grogan 2011-04-22 18:54:59 PDT
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?
Comment 26 David Grogan 2011-04-22 18:55:26 PDT
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?
Comment 27 Early Warning System Bot 2011-04-22 19:10:42 PDT
Attachment 90828 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8498574
Comment 28 David Grogan 2011-04-22 19:27:11 PDT
Created attachment 90831 [details]
fix style and build problems
Comment 29 David Grogan 2011-04-22 19:50:47 PDT
Created attachment 90833 [details]
manual upload that includes all files

webkit-patch and git weren't playing nicely wrt EventQueue.cpp and DocumentEventQueue.cpp
Comment 30 Early Warning System Bot 2011-04-22 20:13:53 PDT
Attachment 90833 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8503021
Comment 31 Build Bot 2011-04-22 20:37:13 PDT
Attachment 90833 [details] did not build on win:
Build output: http://queues.webkit.org/results/8493710
Comment 32 Collabora GTK+ EWS bot 2011-04-23 07:30:09 PDT
Attachment 90833 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8497731
Comment 33 David Grogan 2011-04-27 20:19:45 PDT
Created attachment 91416 [details]
Patch
Comment 34 David Grogan 2011-04-27 20:37:16 PDT
(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.
Comment 35 Dmitry Titov 2011-04-28 13:07:25 PDT
(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 David Grogan 2011-04-28 15:55:55 PDT
(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 Mihai Parparita 2011-04-28 16:41:49 PDT
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?
Comment 38 Dmitry Titov 2011-04-28 17:02:27 PDT
(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 David Grogan 2011-05-09 19:27:48 PDT
(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.
Comment 40 David Grogan 2011-05-09 19:47:48 PDT
(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 David Grogan 2011-05-10 20:23:45 PDT
(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 Albert J. Wong 2011-05-11 08:55:27 PDT
(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 David Grogan 2011-05-13 13:13:52 PDT
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.
Comment 44 David Grogan 2011-10-24 19:57:44 PDT
Created attachment 112294 [details]
Patch
Comment 45 David Grogan 2011-10-24 20:06:53 PDT
Created attachment 112295 [details]
Patch
Comment 46 David Grogan 2011-10-24 20:09:54 PDT
Dave L, could you take a look at this revived patch?
Comment 47 Daniel Bates 2011-10-24 23:58:11 PDT
Comment on attachment 112295 [details]
Patch

Attachment 112295 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/10205846
Comment 48 David Grogan 2011-10-25 13:10:51 PDT
Created attachment 112384 [details]
Patch
Comment 49 Daniel Bates 2011-10-25 19:05:10 PDT
Comment on attachment 112384 [details]
Patch

Attachment 112384 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/10211404
Comment 50 David Levin 2011-10-26 13:42:06 PDT
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.
Comment 51 David Grogan 2011-10-26 14:36:21 PDT
Created attachment 112594 [details]
Patch
Comment 52 David Levin 2011-10-26 14:39:24 PDT
(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.
Comment 53 David Grogan 2011-10-26 15:03:11 PDT
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.)
Comment 54 David Levin 2011-10-26 15:06:28 PDT
Comment on attachment 112594 [details]
Patch

Looks good (especially when the "virtual" is removed in that one place.
Comment 55 David Grogan 2011-10-26 15:46:41 PDT
Created attachment 112608 [details]
Patch for landing
Comment 56 WebKit Review Bot 2011-10-27 07:34:59 PDT
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
Comment 57 David Grogan 2011-10-27 12:10:39 PDT
Created attachment 112724 [details]
Patch for landing
Comment 58 WebKit Review Bot 2011-10-27 16:15:28 PDT
Comment on attachment 112724 [details]
Patch for landing

Clearing flags on attachment: 112724

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