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

Better handling of iframes in the compositor tree #3197

Merged
merged 6 commits into from Sep 12, 2014
Merged

Conversation

@mrobinson
Copy link
Member

mrobinson commented Sep 2, 2014

This is a rework of Bryan Bell's original PR from #2943. It splits the single commit into independent pieces, preserves the existing "root layer + base layer" compositor tree layout, and includes miscellaneous other cleanup.

@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Sep 2, 2014

Critic review: https://critic.hoppipolla.co.uk/r/2495

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@mrobinson
Copy link
Member Author

mrobinson commented Sep 2, 2014

Note that I did not include Bryan's work-around for the failing test in my version, hoping that the reworked tests and code aren't flaky. If travis fails, I'll look into fixing it.

@mrobinson mrobinson force-pushed the mrobinson:iframes branch from 6476011 to 5330aa9 Sep 2, 2014
@jdm
Copy link
Member

jdm commented Sep 3, 2014

failures:
iframe/multiple_external.html == iframe/multiple_external_ref.html
@mrobinson
Copy link
Member Author

mrobinson commented Sep 3, 2014

Yeah, I'll have to poke at this some more. I tried replicating Bryan's original work-around (versus my reworked one) and this is still failing.

@mrobinson mrobinson force-pushed the mrobinson:iframes branch 2 times, most recently from ec6107f to a2c0cec Sep 3, 2014
@mrobinson
Copy link
Member Author

mrobinson commented Sep 3, 2014

Wow! I have to say I'm completely baffled by the continual failure of this reftest. In the latest round, I removed the content of the inner frames, which should make it resilient to iframe content clicking in later and what Bryan's reftest looked like. Still the test fails though.

@mrobinson mrobinson force-pushed the mrobinson:iframes branch 3 times, most recently from 26bbd51 to b04eff8 Sep 3, 2014
@mrobinson
Copy link
Member Author

mrobinson commented Sep 4, 2014

I modified the failing reference test to eliminate all chances of flakiness, but still allow verifying the fix for the old problem of calling SetLayerClipRect on a non-existent layer. I think the remaining failures are unrelated to this changes in this branch.

@jdm
Copy link
Member

jdm commented Sep 6, 2014

I've retriggered the wpt2 job a lot of times now, and I'm pretty sure it's consistently failing on Document-characterSet-normalization.html. Could you run that locally and make sure it passes for you?

@mrobinson
Copy link
Member Author

mrobinson commented Sep 6, 2014

When I run check-wpt locally, /html/dom/interfaces.html fails, but not characterSet-normalization.html. I'll keep investigating.

@jdm
Copy link
Member

jdm commented Sep 6, 2014

We're disabling the normalization test on master now, so you can ignore it.

@mrobinson mrobinson force-pushed the mrobinson:iframes branch from b04eff8 to 547b737 Sep 11, 2014
bjwbell added 2 commits Aug 28, 2014
Instead of storing a single ReadyState, store one per pipeline and
track the earliest one.
This can later be used to decide whether the entire pipeline is ready
for rendering.
@mrobinson mrobinson force-pushed the mrobinson:iframes branch from 547b737 to a7d0e0e Sep 12, 2014
bjwbell and others added 4 commits Aug 29, 2014
Instead of waiting to create the root layer, create them as soon as the
Compositor receives the frame tree. This allows the compositor to
create a layer tree skeleton of which to hang the base layers when they
are ready.
The constallation has accurate information about iframe layer origins,
but not their size.
Instead of producing image output as soon as the first pipeline is
ready, we wait to produce the output until all pipelines are in the
idle RenderState. This should remove a race condition when running
reference tests.
Add iframe tests to their own subdirectory and add another test case
that used to trigger a fatal error. The new test case uses the
"allow-scripts" sandbox attribute to work around a script task failure
caused by the child frame sharing the same script task as the parent.
@mrobinson mrobinson force-pushed the mrobinson:iframes branch from a7d0e0e to 6d0e103 Sep 12, 2014
mrobinson added a commit that referenced this pull request Sep 12, 2014
Better handling of iframes in the compositor tree
@mrobinson mrobinson merged commit 443bcc4 into servo:master Sep 12, 2014
1 check was pending
1 check was pending
continuous-integration/travis-ci The Travis CI build is in progress
Details
@mrobinson mrobinson deleted the mrobinson:iframes branch Sep 12, 2014
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

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