Bug 204447 – Use the event loop instead of DocumentEventQueue and WorkerEventQueue

• Component: DOM
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Delete DocumentEventQueue and WorkerEventQueue in favor of integration with the HTML5 event loop.

[Ryosuke Niwa]

Created attachment 384036 [details]
WIP

[EWS Watchlist]

Comment on attachment 384036 [details]
WIP

Attachment 384036 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13270839

New failing tests:
fast/events/change-overflow-on-overflow-change.html
fast/events/overflow-events-writing-mode.html
fast/events/overflow-events.html
fast/events/overflowchanged-inside-selection-collapse-crash.html

[EWS Watchlist]

Created attachment 384040 [details]
Archive of layout-test-results from ews212 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews212  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Ryosuke Niwa]

Created attachment 384103 [details]
WIP2

[Ryosuke Niwa]

Created attachment 384121 [details]
WIP3

[Ryosuke Niwa]

Given the number of code changes that could result in flaky tests, etc... we should probably split the patch to delete DocumentEventQueue and WorkerEventQueue so that reverting would be easy in the case of test regressions.

[Radar WebKit Bug Importer]

<rdar://problem/57420691>

[Ryosuke Niwa]

Created attachment 384126 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 384126 [details]
Patch

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

> Source/WebCore/ChangeLog:22
> +

Note that I'm purposefully not deleting DocumentEventQueue and WorkerEventQueue for now to make reverts easy.

[Ryosuke Niwa]

Ping reviewers.

[Ryosuke Niwa]

Landed in https://trac.webkit.org/r252824

[Darin Adler]

Comment on attachment 384126 [details]
Patch

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

> Source/WebCore/page/PointerCaptureController.cpp:153
> +            // FIXME: Spec doesn't specify which task soruce to use.

Typo in this comment.

[Darin Adler]

Comment on attachment 384126 [details]
Patch

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

> Source/WebCore/page/PointerLockController.cpp:220
> +    if (auto protectedDocument = makeRefPtr(document))
> +        protectedDocument->queueTaskToDispatchEvent(TaskSource::UserInteraction, Event::create(type, Event::CanBubble::Yes, Event::IsCancelable::No));

Coding style question: Seems super-subtle to know that we need to protect the document here. Should queueTaskToDispatchEvent do it instead?

[Ryosuke Niwa]

(In reply to Darin Adler from comment #13)
> Comment on attachment 384126 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=384126&action=review
> 
> > Source/WebCore/page/PointerLockController.cpp:220
> > +    if (auto protectedDocument = makeRefPtr(document))
> > +        protectedDocument->queueTaskToDispatchEvent(TaskSource::UserInteraction, Event::create(type, Event::CanBubble::Yes, Event::IsCancelable::No));
> 
> Coding style question: Seems super-subtle to know that we need to protect
> the document here. Should queueTaskToDispatchEvent do it instead?

I've been thinking about this raw pointer vs. smart pointer issue for a while now, and my thinking is so far is that we can probably use some simple rule like these:

Any raw pointer or reference to a referenced counted type (i.e. class has ref/deref member functions) should be replaced by a Ref or RefPtr unless those are raw pointers or references passed to the function as an argument, or there is another Ref or RefPtr which stores the same object which does not get cleared or destroyed until all uses of those raw pointers or references.

A slight variant of this rule, which might be more precise to express is as follows:

When calling a non-trivial function G in a function F, every argument of G that is referenced count (i.e. has ref()/deref() member functions) should be stored in Ref or RefPtr in F. That is to say, there must be at least one Ref or RefPtr in the same code block or its ancestor block that is guaranteed to hold the same value as each argument that is referenced count. If one of those arguments of G is also an argument of F, then those arguments do not have Ref/RefPtr in F by transitive property (because the caller of F must have kept those arguments alive).

For example, in the following function f, the local variable `node` should be replaced by RefPtr<Node> but `document` doesn't need to be stored in Ref<Document> because it's an argument.

Node* firstGrandChild(Document& document) {
    Node* node = document.firstChild();
    if (!node)
        return nullptr;
    return node->firstChild();
}

I'm working with a few other folks to come up with a more concrete proposal about this; hopefully in near future.

[Ryosuke Niwa]

(In reply to Darin Adler from comment #12)
> Comment on attachment 384126 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=384126&action=review
> 
> > Source/WebCore/page/PointerCaptureController.cpp:153
> > +            // FIXME: Spec doesn't specify which task soruce to use.
> 
> Typo in this comment.

Fixed the typo in https://trac.webkit.org/r253086.