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

WebGL Rendering Context - Fixed unnecessary unwrap calls for every send #16050

Closed
wants to merge 1 commit into from

Conversation

@mrkajetanp
Copy link

mrkajetanp commented Mar 20, 2017

WebGL Rendering Context

  • replaced unwrap() calls every send to ipc_renderer with if .. is_err()
  • if error occurs, added a warning - "WebRenderer is unavailable!"
  • functions will immediately return after the warning

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #16021 (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because _____
    I think they don't require additional tests because they're just meant to reduce possible thread panics if WebRenderer is not available for some reason.

This change is Reviewable

Issue #16021
@highfive
Copy link

highfive commented Mar 20, 2017

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @metajack (or someone else) soon.

@highfive
Copy link

highfive commented Mar 20, 2017

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/webglrenderingcontext.rs
  • @fitzgen: components/script/dom/webglrenderingcontext.rs
  • @emilio: components/script/dom/webglrenderingcontext.rs
@highfive
Copy link

highfive commented Mar 20, 2017

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@jdm
Copy link
Member

jdm commented Mar 20, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Mar 20, 2017

Trying commit 577e4ad with merge ab93400...

bors-servo added a commit that referenced this pull request Mar 20, 2017
WebGL Rendering Context - Fixed unnecessary unwrap calls for every send

<!-- Please describe your changes on the following line: -->
* replaced unwrap() calls every send to ipc_renderer with if .. is_err()
* if error occurs, added a warning - "WebRenderer is unavailable!"
* functions will immediately return after the warning

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #16021  (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because _____
I think they don't require additional tests because they're just meant to reduce possible thread panics if WebRenderer is not available for some reason.

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- 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/16050)
<!-- Reviewable:end -->
@metajack
Copy link
Contributor

metajack commented Mar 20, 2017

The semantic change here is that previously these unwraps would panic if there was an error, but now we simply warn. Probably @glennw should weigh in on whether panics are appropriate or if we can relax them to warnings.

@jdm
Copy link
Member

jdm commented Mar 20, 2017

Don't we want Servo to be able to continue displaying the page even if something goes wrong in the WebGL rendering thread?

@metajack
Copy link
Contributor

metajack commented Mar 20, 2017

We do, but if the end result of this is that the renderer never recovers, it's not really much of an improvement. I haven't gone to look, but that part is already there, then the panics would indeed be unnecessary.

@bors-servo
Copy link
Contributor

bors-servo commented Mar 20, 2017

@mrkajetanp
Copy link
Author

mrkajetanp commented Mar 20, 2017

Well I guess if there are functions in which an error results in the renderer not being able to recover, we could panic!() instead of return in them.
I'm no expert but I think that even if that's the case for some functions, it doesn't apply to all of them.

@KiChjang
Copy link
Member

KiChjang commented Mar 28, 2017

@KiChjang
Copy link
Member

KiChjang commented Mar 28, 2017

@bors-servo r- try- clean

@nox nox closed this Mar 28, 2017
@nox nox reopened this Mar 28, 2017
@sviter
Copy link

sviter commented Oct 5, 2017

What is the status of this PR?

@jdm
Copy link
Member

jdm commented Oct 5, 2017

It sounds like this change is not desired until we support recreating a failed renderer, unfortunately.

@jdm jdm closed this Oct 5, 2017
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.

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