script: Reduce usage of Trusted in Node::insert.#37762
Conversation
|
cc @jschwe |
jdm
force-pushed
the
post-connect-optiona
branch
from
324d351 to
9c0d4bc
Compare
|
For the syntax, do we really need to pass idents twice (once as a field, once as an arg)? Can we do: task!(ScriptPrepare: |script: DomRoot<HTMLScriptElement>| {
script.prepare(CanGc::note());
}),As for usability I am only worried about haw to deal with no_trace types, if we even need them in tasks anyway. |
Signed-off-by: Josh Matthews <[email protected]>
jdm
force-pushed
the
post-connect-optiona
branch
from
9c0d4bc to
893536b
Compare
This was a good point! I've fixed the syntax to use this suggestion. |
| macro_rules! task { | ||
| ($name:ident: |$($field:ident: $field_type:ty$(,)*)*| $body:tt) => {{ | ||
| #[allow(non_camel_case_types)] | ||
| struct $name<F> { |
There was a problem hiding this comment.
How about impl js::gc::Rootable for the struct, and marking it must_root, so that you can store Dom in the struct?
Since those tasks are always stored somewhere, and not kept on the stack, it seems to make sense. The caller would have to root it first before storing it somewhere, but that seems ok.
There was a problem hiding this comment.
It's an interesting idea. It becomes much more complicated to interact with any captured values that are not Dom objects though, so I'm not sure how I feel about it.
There was a problem hiding this comment.
In that case I would suggest to not use a macro, and just do this by hand, like we do for promise handlers with structs implementing
The task macro is a nice shorthand, but I don't think it's worth the convenience at the cost of storing DomRoot on structs(I'm not sure what the cost is, but my understanding is that this is an anti-pattern).
There was a problem hiding this comment.
I find the task macro to be useful for preserving code readability, since it uses a closure that interacts with values directly before it. I'm going to go ahead and merge the current implementation, since the primary use case of this work is code that would not work with your suggestion (a Vec containing DOM objects).
| ($name:ident: |$($field:ident: $field_type:ty$(,)*)*| $body:tt) => {{ | ||
| #[allow(non_camel_case_types)] | ||
| struct $name<F> { | ||
| $($field: $field_type,)* |
There was a problem hiding this comment.
As per above, this could then be Dom::from_ref(&*$field_type).
| F: ::std::ops::FnOnce($($field_type,)*), | ||
| { | ||
| fn run_once(self) { | ||
| (self.task)($(self.$field,)*); |
There was a problem hiding this comment.
And here you could do self.$field.as_rooted().
4 participants
These changes introduce a new kind of task that uses a variation of the
task!syntax. Existingtask!usages create task structs that have aSendbound, which requires the use ofTrusted<T>to reference a DOM object T inside of the task closure. The new syntax replaces theSendbound with aJSTraceablebound, which requires explicit capture clauses with types. This looks like:The capture clauses must list every value that will be referenced from the closure's environment—these values are moved into fields in the generated task structure, which allows them to be traced by the GC as part of a generated JSTraceable implementation. Since the closure itself is not a
moveclosure, any attempts to reference values not explicitly captured will generate a borrow checker error since the closure requires the'staticlifetime.Testing: Existing WPT tests exercise these code paths. I also attempted to write incorrect tasks that capture references or use values not explicitly captured, and the compiler correctly errors out.
Fixes: part of #35517