Bug 202964 – Integrate resize event with HTML5 event loop

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

HTML5 specification says we should be fire resize event at step 10.5 during the step to update the rendering:
https://html.spec.whatwg.org/multipage/webappapis.html#update-the-rendering

Do that.

[Ryosuke Niwa]

Created attachment 380946 [details]
WIP

[Ryosuke Niwa]

Created attachment 381047 [details]
WIP2

[Ryosuke Niwa]

Created attachment 381048 [details]
WIP3

[Ryosuke Niwa]

Created attachment 381049 [details]
WIP4

[Ryosuke Niwa]

Created attachment 381171 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Ryosuke Niwa]

Created attachment 381197 [details]
Windows test fix attempt

[Ryosuke Niwa]

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

[Geoffrey Garen]

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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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

[Simon Fraser (smfr)]

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?

[Ryosuke Niwa]

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

[Radar WebKit Bug Importer]

<rdar://problem/56391539>

[Truitt Savell]

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"

[WebKit Commit Bot]

Re-opened since this is blocked by bug 203384

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 382415 [details]
Fixed SVG animation tests

[Ryosuke Niwa]

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

[Ryosuke Niwa]

Created attachment 382417 [details]
Fixed SVG animation tests

[Simon Fraser (smfr)]

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.

[Ryosuke Niwa]

Created attachment 382437 [details]
Patch for landing

[Ryosuke Niwa]

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