-
Notifications
You must be signed in to change notification settings - Fork 306
Propagate errors when initializing WebRender instead of panicking #768
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
Conversation
|
☔ The latest upstream changes (presumably #750) made this pull request unmergeable. Please resolve the merge conflicts. |
kvark
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @nical ! The change looks good, but I'd like a few things to be addressed.
| return Err(ShaderError::Link(error_log)); | ||
| } | ||
|
|
||
| Ok(()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similarly with shader compiling, we should still print out warnings on the successful builds too
webrender/src/device.rs
Outdated
| match (vs_id, fs_id) { | ||
| (Some(vs_id), None) => { | ||
| (Ok(vs_id), Err(e)) => { | ||
| println!("FAILED to load fs - falling back to previous!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think failing back to a previous shader is a good idea, especially if we get in a partial state where, for example, the FS is new and the VS is old.
It would be much nicer to do this idiomatically, as in - let vs_id = try!(Device::compile_shader(..)); without the need to do this match afterwards.
webrender/src/renderer.rs
Outdated
| Thread(std::io::Error), | ||
| } | ||
|
|
||
| impl std::convert::From<ShaderError> for InitError { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need the full path here: std::convert::From should be in prelude already
webrender/src/renderer.rs
Outdated
| let (data, marker, shader) = match &batch.data { | ||
| &PrimitiveBatchData::CacheImage(ref data) => { | ||
| let shader = self.ps_cache_image.get(&mut self.device, transform_kind); | ||
| let shader = self.ps_cache_image.get(&mut self.device, transform_kind).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could get away without uwnrap-ping in each match arm by doing it after the match
|
☔ The latest upstream changes (presumably #765) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Needs rebasing. |
|
@bors-servo r=kvark |
|
📌 Commit 82ca8ba has been approved by |
Propagate errors when initializing WebRender instead of panicking First step towards proper error handing in WebRender. This patch focuses on making the initialization of the Renderer fallible. This is important in order to let Gecko be able to catch potential errors and fallback to a different rendering backend. 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/768) <!-- Reviewable:end -->
|
Merging manually, as bors seems stuck, but travis / appveyor have passed. |
First step towards proper error handing in WebRender. This patch focuses on making the initialization of the Renderer fallible. This is important in order to let Gecko be able to catch potential errors and fallback to a different rendering backend.
r? @kvark
This change is