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 refs from WebRenderContext constructor API #153

Merged
merged 1 commit into from
Apr 5, 2020

Conversation

elrnv
Copy link
Contributor

@elrnv elrnv commented Mar 25, 2020

Specifically, the lifetime of WebRenderContext is changed to not depend on lifetimes of CanvasRenderingContext2d and Window objects, since these lifetimes don't actually enforce the existence of web resources anyways.

The mut is also removed from CanvasRenderingContext2d since its API has no mut calls and this generally makes the WebRenderContext difficult to use in libraries like druid.

For compatibility the WebRenderContext maintains a phantom lifetime placeholder for compatibility as well as for anticipating a correct lifetime there.

Specifically, the lifetime of WebRenderContext is changed to not depend on lifetimes of CanvasRenderingContext2d and Window objects, since these lifetimes don't actually enforce the existence of web resources anyways.

The mut is also removed from CanvasRenderingContext2d since its API has
no mut calls and this generally makes the WebRenderContext difficult to use
in libraries like druid.

For compatibility the WebRenderContext maintains a phantom lifetime
placeholder for compatibility as well as for anticipating a correct
lifetime there.
@cmyr
Copy link
Member

cmyr commented Mar 26, 2020

I'm really not sure who 'owns' this code, I've never looked at the piet-web stuff. Are you using this anywhere @elrnv?

@elrnv
Copy link
Contributor Author

elrnv commented Mar 26, 2020

I was looking for a UI library to do some prototyping preferably with web support, since it makes it easier to share the results with others. I saw that druid doesn't yet have complete support for wasm so I thought I could contribute there. This PR makes it possible to integrate piet with wasm in druid (#759). Hope it helps :)

@cmyr
Copy link
Member

cmyr commented Mar 26, 2020

So I think the motivation for having these signatures might be piet-common. At the moment I certainly can't get the tests to compile (I can't actually compile on mac period but that's a separate problem, i'll open an issue for that.)

So I guess: can you run tests in piet-common? I think it doesn't quite matter how closely these APIs match, as long as they are used correctly from the particular backends; it does matter when we want to be able to treat the backends interchangeably.

@xStrom
Copy link
Member

xStrom commented Mar 26, 2020

The piet-common tests do seem to compile for me with these changes. I also opened #156 so that our CI here would compile tests with wasm32.

@elrnv
Copy link
Contributor Author

elrnv commented Mar 26, 2020

Can confirm the compile test in piet-common works for me too.

If I remember correctly (from reading through the code and issues), the goal of having a lifetime on the Piet context (and PietText I guess) is to ensure that the context doesn't outlive the "factory" that it relies on. But in this case the lifetime refers only to the web-sys object pointer, which may in theory outlive the resource (window/canvas/context) it points to.

I'm not sure how to tie the lifetime to the availability of the context in wasm. Getting the window initially is done through web_sys::window() function which returns an Option so the lifetime is effectively checked at runtime. We could potentially restructure the code to call web_sys::window() every time we need the window (followed by getting the canvas and context if those are needed), but this is awkward IMO.

I'm not sure how to test the situation where the window/canvas/context would not be available. I tried deleting the canvas from the inspector, but it did not cause any errors, as if the underlying canvas/context were not garbage collected.

@cmyr cmyr merged commit 04830d8 into linebender:master Apr 5, 2020
@elrnv elrnv deleted the web-fix branch April 9, 2020 03:29
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