Skip to content

/ servo

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

Make webgl behave better with session history #23000

Merged
merged 2 commits into from Mar 8, 2019
Merged

Make webgl behave better with session history #23000

merged 2 commits into from Mar 8, 2019

Conversation

@jdm
Copy link
Member

jdm commented Mar 8, 2019 •
edited by SimonSapin

This prevents the compositor from animating pages that are not actually visible, so pages using webgl do not needlessly impact the performance of the rest of the browser. Additionally, this fixes a problem that was alluded to in this code, causing Servo to delete arbitrary resources when a GC occurred in content that used three.js.

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #22987 and fix #22977 and fix #20934 and fix #20953 and fix #20930 and fix #20950 and fix #20924
  • There are tests for these changes

This change is Reviewable
@jdm
Mark pipelines not visible when updating the active pipeline for a br…
0e30666 
…owsing context.
@highfive
Copy link

highfive commented Mar 8, 2019

Heads up! This PR modifies the following files:

  • @asajeffrey: components/constellation/constellation.rs
  • @cbrewster: components/constellation/constellation.rs
  • @paulrouget: components/constellation/constellation.rs
@jdm
Copy link
Member Author

jdm commented Mar 8, 2019

r? @nox
@highfive highfive assigned nox and unassigned asajeffrey Mar 8, 2019
@jdm jdm mentioned this pull request Mar 8, 2019
Closed
@asajeffrey
asajeffrey approved these changes Mar 8, 2019
View changes
Copy link
Member

asajeffrey left a comment

OMG please yes can we have this yesterday?
components/constellation/constellation.rs
if let Some(new_pipeline) = self.pipelines.get(&new_pipeline_id) {
new_pipeline.notify_visibility(true);
}

This comment has been minimized.

Sign in to view
Copy link
@asajeffrey

asajeffrey Mar 8, 2019

Member

I could have sworn we were already doing this. Am I going mad, and just dreaming up code? @cbrewster, can you remember?

This comment has been minimized.

Sign in to view
Copy link
@jdm

jdm Mar 8, 2019

Author Member

See #22986 - the visibility stuff is totally unused at the moment.

This comment has been minimized.

Sign in to view
Copy link
@cbrewster

cbrewster Mar 8, 2019

Member

It's been a while. I think we only updated activity, but not visibility.
@jdm jdm force-pushed the jdm:invisible-webgl branch from 9091ed3 to 467958d Mar 8, 2019
@jdm
Copy link
Member Author

jdm commented Mar 8, 2019

The automated test relies on the behaviour changes from #22999, so this PR will need to wait until that one is merged.
@nox
Copy link
Member

nox commented Mar 8, 2019

@bors-servo r+

Nice.
@bors-servo
Copy link
Contributor

bors-servo commented Mar 8, 2019

📌 Commit 467958d has been approved by nox
@bors-servo
Copy link
Contributor

bors-servo commented Mar 8, 2019

⌛ Testing commit 467958d with merge bb7ea39...
bors-servo added a commit that referenced this pull request Mar 8, 2019
@bors-servo
Auto merge of #23000 - jdm:invisible-webgl, r=nox
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
bb7ea39 
Make webgl behave better with session history

This prevents the compositor from animating pages that are not actually visible, so pages using webgl do not needlessly impact the performance of the rest of the browser. Additionally, this fixes a problem that was alluded to in [this code](https://github.com/servo/rust-offscreen-rendering-context/blob/b5228c098b889a9806a5f93582903e192b3939ef/src/draw_buffer.rs#L282-L285), causing Servo to delete arbitrary resources when a GC occurred in content that used three.js.

- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #22987 and fix #22977 and fix #20934 and fix #20953 and fix #20930 and fix #20950 and fix #20924
- [x] There are tests for these changes

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

bors-servo commented Mar 8, 2019

💔 Test failed - linux-rel-wpt
@CYBAI
Copy link
Collaborator

CYBAI commented Mar 8, 2019 

{
    "status": "FAIL", 
    "group": "default", 
    "message": "/_mozilla/mozilla/webgl/clearcolor_green.html 481577d73ed0b1f93ad922c251e4e029ec2b8660\n/_mozilla/mozilla/webgl/clearcolor_ref.html b28acfb10eb9c8254d2c7994e1a35f5c0aa70199\nTesting 481577d73ed0b1f93ad922c251e4e029ec2b8660 == b28acfb10eb9c8254d2c7994e1a35f5c0aa70199", 
    "stack": null, 
    "subtest": null, 
    "test": "/_mozilla/mozilla/webgl/clearcolor_green.html", 
    "line": 42656, 
    "action": "test_result", 
    "expected": "PASS"
}
{
    "status": "FAIL", 
    "group": "default", 
    "message": "/_mozilla/mozilla/webgl/clearcolor_blue.html bb883ed4bef46b50c14777ea035a95ea54ee4307\n/_mozilla/mozilla/webgl/clearcolor_ref.html b28acfb10eb9c8254d2c7994e1a35f5c0aa70199\nTesting bb883ed4bef46b50c14777ea035a95ea54ee4307 == b28acfb10eb9c8254d2c7994e1a35f5c0aa70199", 
    "stack": null, 
    "subtest": null, 
    "test": "/_mozilla/mozilla/webgl/clearcolor_blue.html", 
    "line": 42969, 
    "action": "test_result", 
    "expected": "PASS"
}
@jdm
Ensure that removed contexts are made active before removal.
Loading status checks…
056edbb
@jdm jdm force-pushed the jdm:invisible-webgl branch from 467958d to 056edbb Mar 8, 2019
@jdm
Copy link
Member Author

jdm commented Mar 8, 2019

@bors-servo r=nox
@bors-servo
Copy link
Contributor

bors-servo commented Mar 8, 2019

📌 Commit 056edbb has been approved by nox
@bors-servo
Copy link
Contributor

bors-servo commented Mar 8, 2019

⌛ Testing commit 056edbb with merge 34a5a82...
bors-servo added a commit that referenced this pull request Mar 8, 2019
@bors-servo
Auto merge of #23000 - jdm:invisible-webgl, r=nox
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
34a5a82 
Make webgl behave better with session history

This prevents the compositor from animating pages that are not actually visible, so pages using webgl do not needlessly impact the performance of the rest of the browser. Additionally, this fixes a problem that was alluded to in [this code](https://github.com/servo/rust-offscreen-rendering-context/blob/b5228c098b889a9806a5f93582903e192b3939ef/src/draw_buffer.rs#L282-L285), causing Servo to delete arbitrary resources when a GC occurred in content that used three.js.

- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #22987 and fix #22977 and fix #20934 and fix #20953 and fix #20930 and fix #20950 and fix #20924
- [x] There are tests for these changes

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

bors-servo commented Mar 8, 2019

☀️ Test successful - android-mac, arm32, arm64, linux-rel-css, linux-rel-wpt, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, magicleap, status-taskcluster
Approved by: nox
Pushing 34a5a82 to master...
@bors-servo bors-servo merged commit 056edbb into servo:master Mar 8, 2019
3 of 4 checks passed
3 of 4 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
Taskcluster (pull_request) TaskGroup: success
Details
Travis CI - Pull Request Build Passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cbrewster cbrewster

@asajeffrey asajeffrey

Assignees

@nox nox

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

three.js obj2 meshspray loader example has webgl error
three.js md2 loader example has webgl error
three.js geometry text example has webgl error
three.js geometry colors lookuptable example has WebGL error
Going back in history from three.js example panics
Inactive pages in session history with webgl content keep running
Going forwards in history to a webgl page panics
7 participants
@jdm @highfive @nox @bors-servo @CYBAI @asajeffrey @cbrewster
You can’t perform that action at this time.