Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test to constellation to avoid writing reftest image if there are pending frames. #8612

Merged
merged 1 commit into from Dec 17, 2015

Conversation

@glennw
Copy link
Member

glennw commented Nov 20, 2015

This changes several tests that contain <iframe></iframe> from FAIL to TIMEOUT. This is correct
since there is a bug that prevents these iframes from ever rendering.

may genuinely be fixed by this change.~~~

<!-- Reviewable:start -->

[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8612)

<!-- Reviewable:end -->
@glennw
Copy link
Member Author

glennw commented Nov 20, 2015

r? @jdm

@glennw
Copy link
Member Author

glennw commented Nov 20, 2015

@jdm I didn't do any timing, but it felt like this make the wpt tests take a lot longer to execute. Unfortunately it's necessary so there's not much we can do about that anyway.

@glennw
Copy link
Member Author

glennw commented Nov 20, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Nov 20, 2015

Trying commit 89a9fab with merge 8909053...

bors-servo added a commit that referenced this pull request Nov 20, 2015
Add test to constellation to avoid writing reftest image if there are pending frames.

This changes several tests that contain <iframe></iframe> from FAIL to TIMEOUT. This is correct
since there is a bug that prevents these iframes from ever rendering.

There are also a few previous FAILs that changed to OK. These may be intermittents or they
may genuinely be fixed by this change.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8612)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Nov 20, 2015

💔 Test failed - mac-rel-css

@glennw glennw force-pushed the glennw:pending-frames branch from 89a9fab to 1007fdc Nov 20, 2015
@glennw
Copy link
Member Author

glennw commented Nov 20, 2015

@jdm I expect this will also fix some of our remaining intermittent failures.

@glennw
Copy link
Member Author

glennw commented Nov 20, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Nov 20, 2015

Trying commit 1007fdc with merge 02c8871...

bors-servo added a commit that referenced this pull request Nov 20, 2015
Add test to constellation to avoid writing reftest image if there are pending frames.

This changes several tests that contain <iframe></iframe> from FAIL to TIMEOUT. This is correct
since there is a bug that prevents these iframes from ever rendering.

~~~There are also a few previous FAILs that changed to OK. These may be intermittents or they
may genuinely be fixed by this change.~~~

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8612)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Nov 20, 2015

💔 Test failed - linux-rel

@jdm
Copy link
Member

jdm commented Nov 20, 2015

That panic is #8614.

@jdm
Copy link
Member

jdm commented Nov 20, 2015

@bors-servo: r+
Good find!

@bors-servo
Copy link
Contributor

bors-servo commented Nov 20, 2015

📌 Commit 1007fdc has been approved by jdm

@glennw
Copy link
Member Author

glennw commented Nov 20, 2015

1 similar comment
@glennw
Copy link
Member Author

glennw commented Nov 20, 2015

@larsbergstrom
Copy link
Contributor

larsbergstrom commented Nov 20, 2015

@bors-servo r=jdm try- retry

@bors-servo
Copy link
Contributor

bors-servo commented Nov 20, 2015

📌 Commit 1007fdc has been approved by jdm

bors-servo added a commit that referenced this pull request Nov 20, 2015
Add test to constellation to avoid writing reftest image if there are pending frames.

This changes several tests that contain <iframe></iframe> from FAIL to TIMEOUT. This is correct
since there is a bug that prevents these iframes from ever rendering.

~~~There are also a few previous FAILs that changed to OK. These may be intermittents or they
may genuinely be fixed by this change.~~~

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8612)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Dec 16, 2015

💔 Test failed - mac-rel-css

@jdm
Copy link
Member

jdm commented Dec 16, 2015

This design makes me worried about cases where we don't properly trigger reflow. I wonder if the following would work better:

  • the compositor assumes each frame is pending
  • when the compositor reaches the state where it needs to know if a frame is ready:
    • if the frame is marked pending, it sends an async message to the script thread and updates the frame state to awaiting reply
    • if the frame is marked awaiting reply, the compositor is not ready to take a screenshot
  • when the script thread receives the message, it sends an async message to the compositor based on the document ready state and the presence of reftest-wait
  • when the compositor receives the message, it updates the state of the relevant frame based on the contents of the message (ie. the frame is no longer marked awaiting reply)
@jdm
Copy link
Member

jdm commented Dec 16, 2015

-S-awaiting-review +S-needs-code-changes


Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


components/script/dom/window.rs, line 1004 [r5] (raw file):
Let's have a comment that explains this block.


Comments from the review on Reviewable.io

@glennw
Copy link
Member Author

glennw commented Dec 17, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Dec 17, 2015

Trying commit b387633 with merge bd2a00a...

bors-servo added a commit that referenced this pull request Dec 17, 2015
Add test to constellation to avoid writing reftest image if there are pending frames.

This changes several tests that contain <iframe></iframe> from FAIL to TIMEOUT. This is correct
since there is a bug that prevents these iframes from ever rendering.

~~~There are also a few previous FAILs that changed to OK. These may be intermittents or they
may genuinely be fixed by this change.~~~

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8612)
<!-- Reviewable:end -->
@jdm
Copy link
Member

jdm commented Dec 17, 2015

Good catch! I have no more comments here; this is ready to merge if it passes tests repeatably :)
-S-awaiting-review


Reviewed 1 of 1 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from the review on Reviewable.io

@jdm jdm removed the S-awaiting-review label Dec 17, 2015
@bors-servo
Copy link
Contributor

bors-servo commented Dec 17, 2015

… pending frames.

Also change when pipelines become active.

This makes the constellation activate a pipeline as the current frame
when it is ready to do initial reflow, rather than when it is ready
to paint.

This fixes a number of intermittent failures that could previously occur
if an iframe was not visible - which would mean it was never moved from
a pending frame in the constellation to an active frame.

(It happens that webrender exposes these intermittents as permanent failures).
@glennw glennw force-pushed the glennw:pending-frames branch from b387633 to b670430 Dec 17, 2015
@glennw
Copy link
Member Author

glennw commented Dec 17, 2015

@jdm Try build succeeded, squashed down to 1 commit.

@Ms2ger
Copy link
Contributor

Ms2ger commented Dec 17, 2015

@bors-servo r=jdm

@bors-servo
Copy link
Contributor

bors-servo commented Dec 17, 2015

📌 Commit b670430 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Dec 17, 2015

Testing commit b670430 with merge c6ae32a...

bors-servo added a commit that referenced this pull request Dec 17, 2015
Add test to constellation to avoid writing reftest image if there are pending frames.

This changes several tests that contain <iframe></iframe> from FAIL to TIMEOUT. This is correct
since there is a bug that prevents these iframes from ever rendering.

~~~There are also a few previous FAILs that changed to OK. These may be intermittents or they
may genuinely be fixed by this change.~~~

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8612)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Dec 17, 2015

@bors-servo bors-servo merged commit b670430 into servo:master Dec 17, 2015
3 checks passed
3 checks passed
code-review/reviewable Review complete: all files reviewed, all discussions resolved
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@glennw glennw deleted the glennw:pending-frames branch Dec 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.