Bug 204447

Created attachment 384036 [details] WIP

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

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

Created attachment 384103 [details] WIP2

Created attachment 384121 [details] WIP3

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.

<rdar://problem/57420691>

Created attachment 384126 [details] Patch

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.

Ping reviewers.

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

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.

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?

(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.

(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.