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

Allow explicit indication of which missing pipelines are acceptable #2762

Merged
merged 1 commit into from May 16, 2018

Conversation

staktrace
Copy link
Contributor

@staktrace staktrace commented May 16, 2018

In some cases Gecko includes iframes for pipelines that are in other
content processes, and the order in which the display lists reach the
compositor is not deterministic. So it would be better if we could just
have WebRender ignore those pipeline references. In all other cases we
can assert.

This is desirable for https://bugzilla.mozilla.org/show_bug.cgi?id=1454042

r? @kvark


This change is Reviewable

&mut self,
info: &LayoutPrimitiveInfo,
pipeline_id: PipelineId,
ignore_missing_pipeline: bool)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: formatting ())

@kvark
Copy link
Member

kvark commented May 16, 2018

Did you consider instead to have Gecko sending a provisional pipeline in case it needs to update it later? I don't quite like the fact that WR will just skip some elements, whatever they are, since it would make visual surprises for us, non-deterministically. If Gecko doesn't know when it will get the contents, it can at least figure out what the temporary contents should be for the mean time.

@staktrace
Copy link
Contributor Author

I'm not sure how Gecko could send a provisional pipeline, it would require the UI process to block on the content process in order to know whether it needs to send one, and that is not allowed because it can lead to chrome hangs which are bad. Also the temporary contents would just be an empty display list so in practice the results should be the same.

In some cases Gecko includes iframes for pipelines that are in other
content processes, and the order in which the display lists reach the
compositor is not deterministic. So it would be better if we could just
have WebRender ignore those pipeline references. In all other cases we
can assert.
@staktrace
Copy link
Contributor Author

Patch updated to fix style nit

@kvark
Copy link
Member

kvark commented May 16, 2018

Also the temporary contents would just be an empty display list so in practice the results should be the same.

My point here is that it's Gecko who wants to mess things up by not providing the pipeline upfront for scene building, so it's the entity that should take responsibility for missing content, not the special handling in WR.

@kvark
Copy link
Member

kvark commented May 16, 2018

Let's get this in, and continue discussing later.
@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 5801963 has been approved by kvark

@bors-servo
Copy link
Contributor

⌛ Testing commit 5801963 with merge 21d115b...

bors-servo pushed a commit that referenced this pull request May 16, 2018
Allow explicit indication of which missing pipelines are acceptable

In some cases Gecko includes iframes for pipelines that are in other
content processes, and the order in which the display lists reach the
compositor is not deterministic. So it would be better if we could just
have WebRender ignore those pipeline references. In all other cases we
can assert.

This is desirable for https://bugzilla.mozilla.org/show_bug.cgi?id=1454042

r? @kvark

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2762)
<!-- Reviewable:end -->
@Darkspirit
Copy link
Sponsor

#TechnicalDebt

@bors-servo
Copy link
Contributor

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: kvark
Pushing 21d115b to master...

@bors-servo bors-servo merged commit 5801963 into servo:master May 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants