Bug 54810 – Fix isLayoutTimerActive for ports that set a minimumLayoutDelay

• Component: New Bugs
• Status: RESOLVED FIXED

[Tony Gentilcore]

Fix isLayoutTimerActive for ports that set a minimumLayoutDelay

[Tony Gentilcore]

Created attachment 83081 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 83081 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No new tests because no new functionality.

So we have no way to test that we're yielding between tokens?  Do we have any way to detect first paint from JS?  Certainly there is in FF, but I'm not sure there is in WebKit.

> Source/WebCore/dom/Document.cpp:2160
> +    return view() && view()->layoutPending() && minimumLayoutDelay() == m_extraLayoutDelay;

Maybe the minimum = extra check needs itself to be a function.  Given how strange that reads.

[Adam Barth]

Comment on attachment 83081 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        No new tests because no new functionality.
> 
> So we have no way to test that we're yielding between tokens?  Do we have any way to detect first paint from JS?  Certainly there is in FF, but I'm not sure there is in WebKit.

It's very difficult to test.  It might be observable, but I can't think of how to observe it without races.

>> Source/WebCore/dom/Document.cpp:2160
>> +    return view() && view()->layoutPending() && minimumLayoutDelay() == m_extraLayoutDelay;
> 
> Maybe the minimum = extra check needs itself to be a function.  Given how strange that reads.

Yeah, if I were reading this code in the future, it would be pretty mysterious.

[Eric Seidel (no email)]

Comment on attachment 83081 [details]
Patch

cq- for functionization.

[Tony Gentilcore]

Created attachment 83737 [details]
Patch for landing

[Tony Gentilcore]

I went with a bool "isPendingLayoutImmediate" to make this more clear. We can pull it out to a function if anything else needs it. But I'm not sure proliferation of this condition should be encouraged.

[WebKit Commit Bot]

Comment on attachment 83737 [details]
Patch for landing

Clearing flags on attachment: 83737

Committed r79775: <http://trac.webkit.org/changeset/79775>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 83737 [details]:

http/tests/websocket/tests/multiple-connections.html bug 53825 (author: [e-mail])
The commit-queue is continuing to process your patch.