layout: Make the compositor rather than layout determine the position of each iframe. #7423
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
highfive
added
the
S-awaiting-review
|
r? @glennw |
pcwalton
added
A-gfx/rendering
I-wrong
| if let Some(ref frame_size) = frame_size { | ||
| let frame_size = frame_size.to_untyped(); | ||
| root_layer.bounds.borrow_mut().size = Size2D::from_untyped(&frame_size); | ||
| /*root_layer.bounds.borrow_mut().origin = |
|
@pcwalton Just a few minor nits, looks good otherwise. |
pcwalton
force-pushed
the
iframe-stacking-context-position
branch
from
d4d87ed
to
791ec95
Compare
|
@bors-servo: r=glennw |
|
📌 Commit 791ec95 has been approved by |
highfive
added
S-awaiting-merge
bors-servo
pushed a commit
that referenced
this pull request
Aug 28, 2015
…ennw layout: Make the compositor rather than layout determine the position of each iframe. The old code that attempted to do this during layout wasn't able to work for multiple reasons: it couldn't know where the iframe was going to be on the page (because of nested iframes), and at the time it was building the display list for a fragment it couldn't know where that fragment was going to be in page coordinates. This patch rewrites that code so that only the size of an iframe is determined during layout, and the position is determined by the compositor. Layout layerizes iframes and marks the iframe layers with the appropriate subpage ID so that the compositor can place them correctly. Closes #7377. <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7423) <!-- Reviewable:end -->
|
💔 Test failed - mac2 |
highfive
added
S-tests-failed
pcwalton
force-pushed
the
iframe-stacking-context-position
branch
from
791ec95
to
a6737e4
Compare
highfive
added
S-awaiting-review
|
@bors-servo: r=glennw |
|
📌 Commit a6737e4 has been approved by |
highfive
removed
the
S-awaiting-review
highfive
added
the
S-awaiting-merge
pcwalton
force-pushed
the
iframe-stacking-context-position
branch
from
002e120
to
c72d0c2
Compare
highfive
added
S-awaiting-review
each iframe. The old code that attempted to do this during layout wasn't able to work for multiple reasons: it couldn't know where the iframe was going to be on the page (because of nested iframes), and at the time it was building the display list for a fragment it couldn't know where that fragment was going to be in page coordinates. This patch rewrites that code so that both the sizes and positions of iframes are determined by the compositor. Layout layerizes all iframes and marks the iframe layers with the appropriate pipeline and subpage IDs so that the compositor can place them correctly. This approach is similar in spirit to Gecko's `RefLayer` infrastructure. The logic that determines when it is time to take the screenshot for reftests has been significantly revamped to deal with this change in delegation of responsibility. Additionally, this code removes the infrastructure that sends layout data back to the layout task to be destroyed, since it is now all thread-safe and can be destroyed on the script task. The failing tests now fail because of a pre-existing bug related to intrinsic heights and borders on inline replaced elements. They happened to pass before because we never rendered the iframes at all, which meant they never had a chance to draw the red border the tests expect to not render! Closes servo#7377.
|
OK, I believe I've fixed those tests. The failures came from a deadlock in the way the compositor blocked on the constellation, which could happen simultaneously with the constellation waiting on the compositor. Fixing this by introducing a new message pair ( I also effectively redid #7771 with an alternate approach in order to fix the rebase conflict. r? @glennw |
|
Reviewed 3 of 21 files at r2, 12 of 12 files at r3. Comments from the review on Reviewable.io |
|
@bors-servo r+ |
|
📌 Commit c72d0c2 has been approved by |
highfive
added
S-awaiting-merge
bors-servo
pushed a commit
that referenced
this pull request
Sep 30, 2015
…ennw layout: Make the compositor rather than layout determine the position of each iframe. The old code that attempted to do this during layout wasn't able to work for multiple reasons: it couldn't know where the iframe was going to be on the page (because of nested iframes), and at the time it was building the display list for a fragment it couldn't know where that fragment was going to be in page coordinates. This patch rewrites that code so that only the size of an iframe is determined during layout, and the position is determined by the compositor. Layout layerizes iframes and marks the iframe layers with the appropriate subpage ID so that the compositor can place them correctly. Closes #7377. <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7423) <!-- Reviewable:end -->
|
💔 Test failed - mac-rel-wpt |
highfive
added
S-tests-failed
|
@bors-servo retry |
bors-servo
pushed a commit
that referenced
this pull request
Sep 30, 2015
…ennw layout: Make the compositor rather than layout determine the position of each iframe. The old code that attempted to do this during layout wasn't able to work for multiple reasons: it couldn't know where the iframe was going to be on the page (because of nested iframes), and at the time it was building the display list for a fragment it couldn't know where that fragment was going to be in page coordinates. This patch rewrites that code so that only the size of an iframe is determined during layout, and the position is determined by the compositor. Layout layerizes iframes and marks the iframe layers with the appropriate subpage ID so that the compositor can place them correctly. Closes #7377. <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7423) <!-- Reviewable:end -->
highfive
added
S-awaiting-merge
|
☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt |
This was referenced Sep 30, 2015
Closed
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The old code that attempted to do this during layout wasn't able to work
for multiple reasons: it couldn't know where the iframe was going to be
on the page (because of nested iframes), and at the time it was building
the display list for a fragment it couldn't know where that fragment was
going to be in page coordinates.
This patch rewrites that code so that only the size of an iframe is
determined during layout, and the position is determined by the
compositor. Layout layerizes iframes and marks the iframe layers with
the appropriate subpage ID so that the compositor can place them
correctly.
Closes #7377.