Add trait DomObjectWrap to provide WRAP function #25624

lyuyuan · 2020-01-27T19:21:35Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #8576 (GitHub issue number if applicable)

There are tests for these changes OR
These changes do not require tests because ___

highfive · 2020-01-27T19:21:44Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @paulrouget (or someone else) soon.

highfive · 2020-01-27T19:21:45Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/webgl_extensions/ext/angleinstancedarrays.rs, components/script/dom/bindings/reflector.rs
@KiChjang: components/script/dom/webgl_extensions/ext/angleinstancedarrays.rs, components/script/dom/bindings/reflector.rs

highfive · 2020-01-27T19:21:48Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify script code, but no tests are modified. Please consider adding a test!

lyuyuan · 2020-01-31T22:33:43Z

@jdm I have tried adding the DomObjectWrap trait implementation in the derive macro. But there are some interface do not provide the Wrap function. And WindowProxy doesn't have a generated binding. The build errors can be found here.

jdm · 2020-02-01T01:16:01Z

Ok, there's an interesting mixture here:

some interfaces (eg. GlobalScope, CSS, CharacterData) are marked [Abstract] in the WebIDL, meaning there is no way to create a reflector that has that particular concrete type (but interfaces that inherit from those interfaces are fine). This is why there's no Wrap method present.
other interfaces (eg. Headers, FormData) come from WebIDL files that contain multiple types like an interface with an iterator. This means the exported names are in the form {$name}Wrap instead (ie. FormDataWrap).
some interfaces (eg. Promise, WindowProxy) have the [NoInterfaceObject] marking in the WebIDL, so there is no way to create a reflector for those types, so there is no Wrap method present.

I think a reasonable choice here is:

change the code generation to always export the ${name}Wrap form for all interfaces, and use that form in the dom_struct plugin
define a invalid_wrap_function_do_not_use function in components/script/dom/bindings/reflector.rs that panics
for interfaces that do not generate a Wrap method, change the code generation to add a pub use crate::dom::bindings::reflector::invalid_wrap_function_do_not_use as Wrap

lyuyuan · 2020-02-20T01:05:04Z

Hi @jdm

change the code generation to always export the ${name}Wrap form for all interfaces, and use that form in the dom_struct plugin

This seems to break the code in htmlconstructor, as it makes all HTML element bindings reexport the GetConstructorObject method in the form of ${name}GetConstructorObject.

I am modifying the get_constructor macro to adapt this change. Does it sound right?

jdm · 2020-02-20T03:03:00Z

That sounds reasonable for now.

bors-servo · 2020-02-21T15:00:45Z

☔ The latest upstream changes (presumably #25784) made this pull request unmergeable. Please resolve the merge conflicts.

jdm · 2020-02-21T15:05:46Z

This looks like a nice improvement so far! Is this PR still WIP, or is the next step to review the code?

lyuyuan · 2020-02-21T19:18:34Z

Hi @jdm I am still working on some build errors caused by my change. I will switch it ready-for-review once I fix them. Thanks!

lyuyuan · 2020-03-02T02:53:32Z

Hi @jdm , sorry to bother you again. How to get Wrap function implemented in IterableIteratorBinding? I have added this in CodegenRust.py, but the iterable interface still misses it.
Edit: I figured it out. IterableIterator has zero descriptor, so it won't hit the code that generates Wrap.

bors-servo · 2020-03-02T14:28:45Z

☔ The latest upstream changes (presumably #25781) made this pull request unmergeable. Please resolve the merge conflicts.

jdm · 2020-03-19T16:01:22Z

components/script/dom/bindings/iterable.rs

@@ -119,6 +117,11 @@ impl<T: DomObject + JSTraceable + Iterable> IterableIterator<T> {
    }
 }

+impl<T: DomObject + DomObjectWrap + JSTraceable + Iterable> DomObjectWrap for IterableIterator<T> {
+    const WRAP: unsafe fn(JSContext, &GlobalScope, Box<Self>) -> Root<Dom<Self>> =


What is the code that requires a DomObjectWrap implementation for IterableIterator? It's hard for me to figure out if this WRAP will be called anywhere - it looks like it will be used by IterableIterator::new, which doesn't seem like it will work.

You are right. WPT crashed when executing IterableIterator::new 😢.

IterableIterator::new calls reflect_dom_object<T, U> which requires T implements DomObjectWrap. This requires IterableIterator implements the trait.

jdm · 2020-03-19T16:01:22Z

components/script/dom/bindings/codegen/CodegenRust.py

@@ -6286,6 +6309,8 @@ def reexportedName(name):
                cgThings.append(CGWrapGlobalMethod(descriptor, properties))
            else:
                cgThings.append(CGWrapMethod(descriptor))
+                if not descriptor.interface.isIteratorInterface():


Why can we not generate a DomObjectWrap implementation for iterator interfaces?

Sorry for making this mistake. For interfaces that provides Wrap function for both itself and IterableIterator (FormData, Header...), I introduced another trait DomObjectIteratorWrap which provides a const ITER_WRAP that points to the iterator Wrap function. Then, IterableIterator<T>::WRAP could point to T::ITER_WRAP, therefore, implements the DomObjectWrap trait.

jdm · 2020-03-19T16:01:40Z

Overall I'm really excited about these changes!
@bors-servo try=wpt

bors-servo · 2020-03-19T16:01:49Z

⌛ Trying commit 24c90ac with merge 554f14c...


        Auto merge of #25624 - lyuyuan:issue-8576, r=<try>

Add trait DomObjectWrap to provide WRAP function  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #8576 (GitHub issue number if applicable)  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___

bors-servo · 2020-03-19T16:29:09Z

💔 Test failed - status-taskcluster

jdm · 2020-03-19T16:44:05Z

Indeed:

  ▶ CRASH [expected OK] /fetch/api/redirect/redirect-location.any.worker.html
  │ 
  │ 
  │ 
  │ 
  │ Invalid wrap function, do not use (thread WebWorker for http://web-platform.test:8000/fetch/api/redirect/redirect-location.any.worker.js, at components/script/dom/bindings/reflector.rs:121)
  │ stack backtrace:
  │    0: servo::backtrace::print
  │    1: servo::main::{{closure}}
  │    2: std::panicking::rust_panic_with_hook
  │              at src/libstd/panicking.rs:513
  │    3: std::panicking::begin_panic
  │    4: script::dom::bindings::iterable::IterableIterator<T>::new
  │    5: <std::panic::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once
  │    6: script::dom::bindings::codegen::Bindings::HeadersBinding::HeadersBinding::entries
  │    7: CallJitMethodOp
  │    8: script::dom::bindings::utils::generic_call

  ▶ CRASH [expected OK] /WebIDL/ecmascript-binding/default-iterator-object.html
  │ 
  │ 
  │ 
  │ 
  │ Invalid wrap function, do not use (thread ScriptThread PipelineId { namespace_id: PipelineNamespaceId(1), index: PipelineIndex(1) }, at components/script/dom/bindings/reflector.rs:121)
  │ stack backtrace:
  │    0: servo::backtrace::print
  │    1: servo::main::{{closure}}
  │    2: std::panicking::rust_panic_with_hook
  │              at src/libstd/panicking.rs:513
  │    3: std::panicking::begin_panic
  │    4: script::dom::bindings::iterable::IterableIterator<T>::new
  │    5: <std::panic::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once
  │    6: script::dom::bindings::codegen::Bindings::URLSearchParamsBinding::URLSearchParamsBinding::entries
  │    7: CallJitMethodOp
  │    8: script::dom::bindings::utils::generic_call


        Add trait DomObjectWrap to provide WRAP function

jdm · 2020-03-21T13:39:42Z

Great solution!
@bors-servo r+

bors-servo · 2020-03-21T13:39:43Z

📌 Commit 3ea6d87 has been approved by jdm

bors-servo · 2020-03-21T13:39:48Z

⌛ Testing commit 3ea6d87 with merge 0a272c2...


        Auto merge of #25624 - lyuyuan:issue-8576, r=jdm

Add trait DomObjectWrap to provide WRAP function  ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #8576 (GitHub issue number if applicable)  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___

bors-servo · 2020-03-21T14:24:32Z

💔 Test failed - status-taskcluster

jdm · 2020-03-21T15:06:23Z

@bors-servo retry
#25513

bors-servo · 2020-03-21T15:06:28Z

⌛ Testing commit 3ea6d87 with merge 67b4336...

bors-servo · 2020-03-21T15:16:58Z

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing 67b4336 to master...

highfive assigned paulrouget Jan 27, 2020

highfive added the S-awaiting-review label Jan 27, 2020

lyuyuan force-pushed the lyuyuan:issue-8576 branch 4 times, most recently from 140775b to 205e619 Jan 28, 2020

lyuyuan force-pushed the lyuyuan:issue-8576 branch from 205e619 to ef1661c Feb 10, 2020

lyuyuan force-pushed the lyuyuan:issue-8576 branch from ef1661c to 0f3bc97 Feb 21, 2020

lyuyuan changed the title ~~WIP: Add WRAP function pointer to DomObject trait~~ WIP: Add trait DomObjectWrap to provide WRAP function Feb 21, 2020

highfive added the S-needs-rebase label Feb 21, 2020

jdm assigned jdm and unassigned paulrouget Feb 21, 2020

jdm added S-needs-code-changes and removed S-awaiting-review labels Feb 21, 2020

lyuyuan force-pushed the lyuyuan:issue-8576 branch from 0f3bc97 to 3c320c1 Mar 1, 2020

highfive added S-awaiting-review and removed S-needs-code-changes labels Mar 1, 2020

lyuyuan force-pushed the lyuyuan:issue-8576 branch from 3c320c1 to 86251b9 Mar 2, 2020

jdm reviewed Mar 19, 2020

View changes

highfive added the S-tests-failed label Mar 19, 2020

Add trait DomObjectWrap to provide WRAP function

Unverified

This user has not uploaded their public key yet.

GPG key ID: 63A93E6A451DD166 Learn about signing commits

Loading status checks…

3ea6d87

lyuyuan force-pushed the lyuyuan:issue-8576 branch from 24c90ac to 3ea6d87 Mar 21, 2020

highfive removed the S-tests-failed label Mar 21, 2020

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Mar 21, 2020

highfive added S-tests-failed and removed S-awaiting-merge labels Mar 21, 2020

highfive added S-awaiting-merge and removed S-tests-failed labels Mar 21, 2020

bors-servo merged commit 67b4336 into servo:master Mar 21, 2020
2 checks passed

2 checks passed

Community-TC (pull_request) TaskGroup: success
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Mar 21, 2020

bors-servo mentioned this pull request Mar 21, 2020

Implement readablestream support #25873

Merged

0 of 5 tasks complete

lyuyuan deleted the lyuyuan:issue-8576 branch Mar 21, 2020

servo / servo

Add trait DomObjectWrap to provide WRAP function #25624

Add trait DomObjectWrap to provide WRAP function #25624

lyuyuan commented Jan 27, 2020 •

edited

highfive commented Jan 27, 2020

highfive commented Jan 27, 2020

highfive commented Jan 27, 2020

lyuyuan commented Jan 31, 2020

jdm commented Feb 1, 2020

lyuyuan commented Feb 20, 2020 •

edited

jdm commented Feb 20, 2020

bors-servo commented Feb 21, 2020

jdm commented Feb 21, 2020

lyuyuan commented Feb 21, 2020

lyuyuan commented Mar 2, 2020 •

edited

bors-servo commented Mar 2, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Mar 19, 2020

bors-servo commented Mar 19, 2020

bors-servo commented Mar 19, 2020

jdm commented Mar 19, 2020 •

edited

jdm commented Mar 21, 2020

bors-servo commented Mar 21, 2020

bors-servo commented Mar 21, 2020

bors-servo commented Mar 21, 2020

jdm commented Mar 21, 2020

bors-servo commented Mar 21, 2020

bors-servo commented Mar 21, 2020

servo / servo

Join GitHub today

Add trait DomObjectWrap to provide WRAP function #25624

Add trait DomObjectWrap to provide WRAP function #25624

Conversation

lyuyuan commented Jan 27, 2020 • edited

highfive commented Jan 27, 2020

highfive commented Jan 27, 2020

highfive commented Jan 27, 2020

lyuyuan commented Jan 31, 2020

jdm commented Feb 1, 2020

lyuyuan commented Feb 20, 2020 • edited

jdm commented Feb 20, 2020

bors-servo commented Feb 21, 2020

jdm commented Feb 21, 2020

lyuyuan commented Feb 21, 2020

lyuyuan commented Mar 2, 2020 • edited

bors-servo commented Mar 2, 2020

This comment has been minimized.

jdm Mar 19, 2020

This comment has been minimized.

lyuyuan Mar 19, 2020

This comment has been minimized.

lyuyuan Mar 21, 2020 • edited

This comment has been minimized.

jdm Mar 19, 2020

This comment has been minimized.

lyuyuan Mar 21, 2020

jdm commented Mar 19, 2020

bors-servo commented Mar 19, 2020

bors-servo commented Mar 19, 2020

jdm commented Mar 19, 2020 • edited

jdm commented Mar 21, 2020

bors-servo commented Mar 21, 2020

bors-servo commented Mar 21, 2020

bors-servo commented Mar 21, 2020

jdm commented Mar 21, 2020

bors-servo commented Mar 21, 2020

bors-servo commented Mar 21, 2020

lyuyuan commented Jan 27, 2020 •

edited

lyuyuan commented Feb 20, 2020 •

edited

lyuyuan commented Mar 2, 2020 •

edited

lyuyuan Mar 21, 2020 •

edited

jdm commented Mar 19, 2020 •

edited