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

Remove Rc usage from the crate #188

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Remove Rc usage from the crate #188

wants to merge 1 commit into from

Conversation

nox
Copy link
Contributor

@nox nox commented Mar 8, 2019

Callers should use Rc wherever they want, so that adapters such as ErrorCheckingGl can do their job without multiple layers of virtual calls.


This change is Reviewable

Callers should use Rc<T> wherever they want, so that adapters such as ErrorCheckingGl can do their job without multiple layers of virtual calls.
@nox
Copy link
Contributor Author

nox commented Mar 8, 2019

Cc @asajeffrey @bholley

@bholley
Copy link
Contributor

bholley commented Mar 9, 2019

Looking at this patch, I'm not sure it's compatible with WebRender. That code needs to start using a trait object early on (to abstract over the difference between GL and GLES), and by the time we decide to apply the wrappers (e.g. ErrorReactingGl) we don't have the concrete type anymore.

ProfilingGl has a similar problem [1], which is sort of exacerbated by the fact that it can have four different inners (GL/GLES, with/without ErrorReactingGl wrapper).

[1] https://hg.mozilla.org/integration/mozilla-inbound/rev/1972b8836948

@kvark
Copy link
Member

kvark commented Mar 9, 2019

@bholley interesting point! Can't we solve this by simply:

impl<T: Gl> Gl for Rc<T> {...}

@nox
Copy link
Contributor Author

nox commented Mar 9, 2019

Yes.

@kvark
Copy link
Member

kvark commented Jul 10, 2019

@nox so we are basically ready to land this, just waiting for a moment to do the breaking change?

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.

3 participants