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 the `wrap_fn` argument from `reflect_dom_object`. #8576

Closed
Ms2ger opened this issue Nov 18, 2015 · 10 comments
Closed

Remove the `wrap_fn` argument from `reflect_dom_object`. #8576

Ms2ger opened this issue Nov 18, 2015 · 10 comments

Comments

@Ms2ger
Copy link
Contributor

@Ms2ger Ms2ger commented Nov 18, 2015

It should be possible to derive it from the type. Maybe FooBinding::Wrap should be <Foo as ConcreteDOMObject>::wrap? (Name of the trait up for bikeshedding.)

@nox
Copy link
Member

@nox nox commented Dec 16, 2015

This obviously exclude global interfaces, which don't have the same wrap signature. Maybe we should also have a reflect_dom_global function with that other wrap?

@Ms2ger
Copy link
Contributor Author

@Ms2ger Ms2ger commented Dec 16, 2015

Orthogonal to this issue, but that sounds like something we could do, yes.

@jooh-kim
Copy link

@jooh-kim jooh-kim commented Mar 10, 2018

@jdm hi can i try this issue?

@jdm
Copy link
Member

@jdm jdm commented Mar 10, 2018

@kjhyung Of course! Ask questions about anything that's unclear!

@jdm jdm added the C-assigned label Mar 10, 2018
@jdm
Copy link
Member

@jdm jdm commented Mar 25, 2018

@kjhyung Have you made any progress on this task?

@jdm jdm removed the C-assigned label Sep 10, 2018
@lyuyuan
Copy link
Contributor

@lyuyuan lyuyuan commented Jan 14, 2020

@jdm May I have a try on this if no one is working on it now?

@jdm
Copy link
Member

@jdm jdm commented Jan 14, 2020

@lyuyuan Sure! I think the easiest way to proceed is to:

Does that make sense?

@lyuyuan
Copy link
Contributor

@lyuyuan lyuyuan commented Jan 15, 2020

@jdm Sounds good! Thanks for the tip!

@lyuyuan
Copy link
Contributor

@lyuyuan lyuyuan commented Jan 28, 2020

@jdm It seems that DomObject trait cannot contain associated constants (E0038) because it is FormControl's supertrait and FormControl is used in a trait object here.
If I understand the workaround correctly, I need to add a helper method in DomObject which returns the function pointer, WRAP. Does this sounds correct? Thanks!

@jdm
Copy link
Member

@jdm jdm commented Jan 28, 2020

I think we can get away with a different solution:

trait DomObjectWrap {
    const WRAP: ...
}

And edit https://github.com/servo/servo/blob/master/components/dom_struct/lib.rs#L42-L53 to implement the DomObjectWrap trait, and make reflect_dom_object take T: DomObject + DomObjectWrap.

bors-servo added a commit that referenced this issue Mar 5, 2020
Add trait DomObjectWrap to provide WRAP function

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./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)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Mar 6, 2020
Add trait DomObjectWrap to provide WRAP function

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./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)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Mar 19, 2020
Add trait DomObjectWrap to provide WRAP function

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [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)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Mar 21, 2020
Add trait DomObjectWrap to provide WRAP function

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [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)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants
You can’t perform that action at this time.