Bug 202964

Created attachment 380946 [details] WIP

Created attachment 381047 [details] WIP2

Created attachment 381048 [details] WIP3

Created attachment 381049 [details] WIP4

Created attachment 381171 [details] Patch

Comment on attachment 381171 [details] Patch Attachment 381171 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/13142433 New failing tests: fast/shadow-dom/trusted-event-scoped-flags.html

Created attachment 381178 [details] Archive of layout-test-results from ews214 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews214 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

Created attachment 381197 [details] Windows test fix attempt

Ping reviews (EWS is all green except armv7 & i386 JSC bots).

Comment on attachment 381197 [details] Windows test fix attempt View in context: https://bugs.webkit.org/attachment.cgi?id=381197&action=review r=me > Source/WebCore/page/Page.cpp:1297 > + layoutIfNeeded(); Are the additional layoutIfNeeded() calls below also required? (Kind of sad to do up to three layouts just because certain events dirtied the DOM.) > Source/WebCore/page/Page.cpp:2879 > + auto* document = frame->document(); > + if (!document) > + continue; A frame always has a document, so you can skip this null check.

Comment on attachment 381197 [details] Windows test fix attempt View in context: https://bugs.webkit.org/attachment.cgi?id=381197&action=review >> Source/WebCore/page/Page.cpp:1297 >> + layoutIfNeeded(); > > Are the additional layoutIfNeeded() calls below also required? (Kind of sad to do up to three layouts just because certain events dirtied the DOM.) Yes. Otherwise, we don't update the states for intersection observers & resize observers. However, some of these layout updates are no-op if they don't make any changes. Regardless, we can probably optimize away some of layouts needed here once we've fully moved to the new model.

(In reply to Geoffrey Garen from comment #10) > Comment on attachment 381197 [details] > Windows test fix attempt > > View in context: > https://bugs.webkit.org/attachment.cgi?id=381197&action=review > > r=me > > > Source/WebCore/page/Page.cpp:2879 > > + auto* document = frame->document(); > > + if (!document) > > + continue; > > A frame always has a document, so you can skip this null check. Hm... I don't think this is true. FrameLoader::clear sets Frame::m_doc to nullptr. It's true that this function is currently not called within FrameLoader::clear but I think we should make this code future proof.

Comment on attachment 381197 [details] Windows test fix attempt View in context: https://bugs.webkit.org/attachment.cgi?id=381197&action=review > Source/WebCore/ChangeLog:11 > + Exisitng code in WebCore which was dispatching or scheduling dispatching of resize events now simply sets Exisitng > Source/WebCore/page/FrameView.cpp:3379 > + LOG(Events, "FrameView %p sendResizeEventIfNeeded scheduling resize event for document %p, size %dx%d", this, frame().document(), currentSize.width(), currentSize.height()); LOG_WITH_STREAM will do size formatting for you. We could also make it print a human-readable thing for a Document&. > Source/WebCore/page/Page.cpp:1306 > + Vector<Ref<Document>> documents = collectDocuments(); // The requestAnimationFrame callbacks may change the frame hierarchy of the page It's going to be really easy to make the mistake, when we extend this function, of re-using 'documents' from before a previous step, and thus using a stale list. Can we factor the code to make this less likely? Maybe a lambda for each step that goes into a vector, and a wrapper function that gathers documents each time? > Source/WebCore/page/Page.h:753 > void forEachDocument(const WTF::Function<void(Document&)>&); > + Vector<Ref<Document>> collectDocuments(); Why don't we use forEachDocument in the rendering steps?

Committed r251269: <https://trac.webkit.org/changeset/251269>

<rdar://problem/56391539>

It looks like the new test fast/events/resize-subframe-in-rendering-update.html added in https://trac.webkit.org/changeset/251269/webkit is a flakey failure on Mac and iOS: https://results.webkit.org/?suite=layout-tests&test=fast%2Fevents%2Fresize-subframe-in-rendering-update.html Diff: --- /Volumes/Data/slave/mojave-release-tests-wk1/build/layout-test-results/fast/events/resize-subframe-in-rendering-update-expected.txt +++ /Volumes/Data/slave/mojave-release-tests-wk1/build/layout-test-results/fast/events/resize-subframe-in-rendering-update-actual.txt @@ -7,6 +7,8 @@ iframeA.style.width = "200px"; updateLayout(iframeA) iframeAA.style.width = "200px"; updateLayout(iframeAA) PASS logs.length is 0 +After 0s setTimeout +PASS logs.length is 0 After requestAnimationFrame PASS logs.length is 3 PASS logs.join(", ") is "A, AA, B"

Re-opened since this is blocked by bug 203384

Comment on attachment 381197 [details] Windows test fix attempt View in context: https://bugs.webkit.org/attachment.cgi?id=381197&action=review >> Source/WebCore/page/Page.cpp:1306 >> + Vector<Ref<Document>> documents = collectDocuments(); // The requestAnimationFrame callbacks may change the frame hierarchy of the page > > It's going to be really easy to make the mistake, when we extend this function, of re-using 'documents' from before a previous step, and thus using a stale list. Can we factor the code to make this less likely? Maybe a lambda for each step that goes into a vector, and a wrapper function that gathers documents each time? Well, the subsequent steps are using the same documents now. Let me refactor that in a separate patch.

Created attachment 382415 [details] Fixed SVG animation tests

(In reply to Simon Fraser (smfr) from comment #13) > Comment on attachment 381197 [details] > Windows test fix attempt > > View in context: > https://bugs.webkit.org/attachment.cgi?id=381197&action=review > > > Source/WebCore/ChangeLog:11 > > + Exisitng code in WebCore which was dispatching or scheduling dispatching of resize events now simply sets > > Exisitng Fixed. > > Source/WebCore/page/FrameView.cpp:3379 > > + LOG(Events, "FrameView %p sendResizeEventIfNeeded scheduling resize event for document %p, size %dx%d", this, frame().document(), currentSize.width(), currentSize.height()); > > LOG_WITH_STREAM will do size formatting for you. We could also make it print > a human-readable thing for a Document&. Sure, fixed. > > Source/WebCore/page/Page.h:753 > > void forEachDocument(const WTF::Function<void(Document&)>&); > > + Vector<Ref<Document>> collectDocuments(); > > Why don't we use forEachDocument in the rendering steps? Sure, done.

Created attachment 382417 [details] Fixed SVG animation tests

Comment on attachment 382417 [details] Fixed SVG animation tests View in context: https://bugs.webkit.org/attachment.cgi?id=382417&action=review > Source/WebCore/page/FrameView.cpp:3425 > + LOG_WITH_STREAM(Events, stream << "FrameView" << this << "sendResizeEventIfNeeded scheduling resize event for document" << frame().document() << ", size " << currentSize.width() << currentSize.height()); just " size " << currentSize > Tools/DumpRenderTree/mac/DumpRenderTree.mm:1731 > + fprintf(stderr, "DumpRenderTree dump\n"); > + WTFReportBacktrace(); Remove.

Created attachment 382437 [details] Patch for landing

Committed r251867: <https://trac.webkit.org/changeset/251867>