Bug 54810 - Fix isLayoutTimerActive for ports that set a minimumLayoutDelay
Summary: Fix isLayoutTimerActive for ports that set a minimumLayoutDelay
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Tony Gentilcore
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-19 11:26 PST by Tony Gentilcore
Modified: 2011-02-26 04:07 PST (History)
3 users (show)

See Also:


Attachments
Patch (4.13 KB, patch)
2011-02-19 11:31 PST, Tony Gentilcore
no flags Details | Formatted Diff | Diff
Patch for landing (3.56 KB, patch)
2011-02-24 15:51 PST, Tony Gentilcore
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Gentilcore 2011-02-19 11:26:10 PST
Fix isLayoutTimerActive for ports that set a minimumLayoutDelay
Comment 1 Tony Gentilcore 2011-02-19 11:31:10 PST
Created attachment 83081 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-02-19 12:46:53 PST
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.
Comment 3 Adam Barth 2011-02-21 12:34:24 PST
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.
Comment 4 Eric Seidel (no email) 2011-02-24 15:50:03 PST
Comment on attachment 83081 [details]
Patch

cq- for functionization.
Comment 5 Tony Gentilcore 2011-02-24 15:51:26 PST
Created attachment 83737 [details]
Patch for landing
Comment 6 Tony Gentilcore 2011-02-24 15:52:09 PST
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.
Comment 7 WebKit Commit Bot 2011-02-26 03:20:27 PST
Comment on attachment 83737 [details]
Patch for landing

Clearing flags on attachment: 83737

Committed r79775: <http://trac.webkit.org/changeset/79775>
Comment 8 WebKit Commit Bot 2011-02-26 03:20:33 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 WebKit Commit Bot 2011-02-26 04:07:13 PST
The commit-queue encountered the following flaky tests while processing attachment 83737 [details]:

http/tests/websocket/tests/multiple-connections.html bug 53825 (author: abarth@webkit.org)
The commit-queue is continuing to process your patch.