-
Notifications
You must be signed in to change notification settings - Fork 51
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 Windows and Web handles !Send
and !Sync
#154
base: master
Are you sure you want to change the base?
Conversation
Making these handles un-Send would be a breaking change, so we might not want to do this. |
Yet, at least ;) |
@@ -33,6 +34,7 @@ pub struct WebWindowHandle { | |||
/// | |||
/// [data attributes]: https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/data-* | |||
pub id: u32, | |||
_marker: PhantomData<*const ()>, |
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'm not sure what to think of this, the ID here is not a pointer, but it might be misleading for consumers who think that they are on the main thread if they got this variant.
Is there any previous discussion on this?
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 was also wondering if there are any plans to remove this type completely, now that we have the WebCanvasWindowHandle
and WebOffscreenCanvasWindowHandle
types.
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.
plans to remove this type completely
Moved to #157.
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'm not sure what to think of this, the ID here is not a pointer, but it might be misleading for consumers who think that they are on the main thread if they got this variant.
Well, that's the same as what I've documented on e.g. AppKitWindowHandle
, but I'm beginning to see that that may not be the best approach (hence why this PR isn't merged yet).
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.
After some more thinking I'm now in favor of making this !Send
.
!Send
and !Sync
, and document main thread safety!Send
and !Sync
Follow-up to #152 to consider documenting our decision on main thread safety, and making some handles
!Send
and!Sync
.