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

Bindings generation ignores Exposed annotations on members and partial interfaces #23332

Closed
jdm opened this issue May 6, 2019 · 9 comments
Closed

Comments

@jdm
Copy link
Member

@jdm jdm commented May 6, 2019

Despite #12414 being closed, exposed annotations do on members do not actually affect whether those members are exposed in the generated bindings. master...jdm:exposed-member contains an automated test that demonstrates this.

@highfive
Copy link

@highfive highfive commented May 27, 2019

@jdm
Copy link
Member Author

@jdm jdm commented May 27, 2019

This requires:

  • adding a Exposed(Globals) variant to the Condition enum
  • making Condition::is_satisfied call is_exposed_in for the new variant
  • updating the code generator's MemberCondition to accept a new argument for exposed conditions and returning Condition::Exposed when appropriate
  • updating getControllingCondition to pass a list of exposed globals when appropriate

Code: components/script/dom/bindings/guard.rs, components/script/dom/bindings/codegen/CodegenRust.py

@sreeise
Copy link
Contributor

@sreeise sreeise commented May 27, 2019

@highfive: assign me

I can work on this :)

@highfive highfive added the C-assigned label May 27, 2019
@highfive
Copy link

@highfive highfive commented May 27, 2019

Hey @sreeise! Thanks for your interest in working on this issue. It's now assigned to you!

@jdm
Copy link
Member Author

@jdm jdm commented May 27, 2019

Please let me know if anything is unclear!

@sreeise
Copy link
Contributor

@sreeise sreeise commented May 29, 2019

@jdm Hey, so I started on this in the commit here, but it doesn't compile yet so I didn't bother to make a pr.

The main issue currently is where the Exposed annotation could either be a list or it could be a string. When a match is found, in Guard.rs, and the is_exposed_in method is called it expects a single global to be passed as an argument which works only if the Exposed attribute in any given webidl has one member, i.e., Exposed=Window as opposed to Exposed=(Window, Worker). Because there is a possibility that the attribute is set to a list, compilation will fail because is_exposed_in does not expect a list.

For example, when the code is generated for a list the arguments don't get parsed correctly:


1349 |     Guard::new(Condition::Exposed(['Window', 'Worker']), sMethods_specs[1]),
     |                                              ^^^^^^^^
help: if you meant to write a `str` literal, use double quotes
     |
1349 |     Guard::new(Condition::Exposed(['Window', "Worker"]), sMethods_specs[1]),

The compiler complains about using double quotes for a string literal which really isn't the problem as is_exposed_in only accepts one InterfaceObjectMap::Globals argument anyway (I am assuming that globals is not an &str so I could be wrong).

I am honestly not sure where in CodgenRust.py to write a fix for this or if I am possibly missing something that should be changed. I havn't worked in the codegen section before so I figured I'd ask about this before attempting to find a solution. Thanks for your help :).

@jdm
Copy link
Member Author

@jdm jdm commented May 29, 2019

The Globals type is actually a bitset. I recommend using the following code as a model:

bits = " | ".join(sorted(
"InterfaceObjectMap::Globals::" + camel_to_upper_snake(i) for i in iface.exposureSet
))
.

@jdm
Copy link
Member Author

@jdm jdm commented May 29, 2019

More specifically, returning Condition::Exposed(%s) with the Globals bitset value from MemberCondition seems like the right choice to me.

@jdm
Copy link
Member Author

@jdm jdm commented May 29, 2019

You can see the documentation for the Globals type at https://doc.servo.org/script/dom/bindings/codegen/InterfaceObjectMap/struct.Globals.html if that helps.

bors-servo added a commit that referenced this issue Jun 3, 2019
Make bindings aware of exposed members/partial interfaces

<!-- 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 #23332

<!-- 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. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23500)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jun 4, 2019
Make bindings aware of exposed members/partial interfaces

<!-- 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 #23332

<!-- 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. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23500)
<!-- Reviewable:end -->
@jdm jdm mentioned this issue Jun 11, 2019
3 of 3 tasks complete
bors-servo added a commit that referenced this issue Jun 30, 2019
Make bindings aware of exposed members/partial interfaces

<!-- 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 #23332

<!-- 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. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23500)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jul 3, 2019
Make bindings aware of exposed members/partial interfaces

<!-- 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 #23332

<!-- 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. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23500)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jul 10, 2019
Make bindings aware of exposed members/partial interfaces

<!-- 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 #23332

<!-- 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. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23500)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jul 11, 2019
Make bindings aware of exposed members/partial interfaces

<!-- 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 #23332

<!-- 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. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23500)
<!-- Reviewable:end -->
@jdm jdm added the C-has open PR label Jul 12, 2019
bors-servo added a commit that referenced this issue Jul 14, 2019
Make bindings aware of exposed members/partial interfaces

<!-- 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 #23332

<!-- 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. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23500)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jul 14, 2019
Make bindings aware of exposed members/partial interfaces

<!-- 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 #23332

<!-- 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. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23500)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jul 14, 2019
Make bindings aware of exposed members/partial interfaces

<!-- 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 #23332

<!-- 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. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23500)
<!-- Reviewable:end -->
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.

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