Skip to content

/ servo

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

Support IDL stringifier attributes #24544

Merged
merged 2 commits into from Oct 29, 2019
Merged

Support IDL stringifier attributes #24544

merged 2 commits into from Oct 29, 2019

Conversation

@saschanaz
Copy link
Contributor

saschanaz commented Oct 25, 2019 •
edited

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #7590
  • There are tests for these changes
@highfive
Copy link

highfive commented Oct 25, 2019

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/webidls/WorkerLocation.webidl, components/script/dom/workerlocation.rs, components/script/dom/webidls/Location.webidl, components/script/dom/url.rs and 9 more
  • @KiChjang: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/webidls/WorkerLocation.webidl, components/script/dom/workerlocation.rs, components/script/dom/webidls/Location.webidl, components/script/dom/url.rs and 9 more
@saschanaz
Copy link
Contributor Author

saschanaz commented Oct 25, 2019

@bors-servo try=wpt
bors-servo added a commit that referenced this pull request Oct 25, 2019
@bors-servo
Auto merge of #24544 - saschanaz:stringifier-attr, r=<try>
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
e161a3b 
Support IDL stringifier attributes

<!-- 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
- [ ] `./mach test-tidy` does not report any errors
- [x] These changes fix #7590

<!-- Either: -->
- [x] There are tests for these changes

<!-- 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
Copy link
Contributor

bors-servo commented Oct 25, 2019

⌛ Trying commit 9d5d6fd with merge e161a3b...
@saschanaz
saschanaz reviewed Oct 25, 2019
View changes
components/script/dom/webidls/DOMTokenList.webidl Outdated
@@ -21,9 +21,8 @@ interface DOMTokenList {
[CEReactions, Throws]
void replace(DOMString token, DOMString newToken);

[CEReactions, Pure]
attribute DOMString value;
[CEReactions]

This comment has been minimized.

Sign in to view
@saschanaz

saschanaz Oct 25, 2019

Author Contributor

Removed Pure because WebIDL.py currently does not allow it on stringifier attribute (just because gecko didn't have it on any of its stringifier attributes).

The support can be added, but not sure why servo has Pure but gecko does not. @nox, could you give me some idea?

This comment has been minimized.

Sign in to view
@nox

nox Oct 25, 2019

Member

https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings

[Pure]

This is an alias for [Affects=Nothing, DependsOn=DOMState]. Attributes/methods flagged in this way promise that they will keep returning the same value as long as nothing that has [Affects=Everything] executes.

This comment has been minimized.

Sign in to view
@saschanaz

saschanaz Oct 25, 2019

Author Contributor

Thanks for the docs, but I mean why DOMTokenList in Gecko does not have Pure.

This comment has been minimized.

Sign in to view
@saschanaz

saschanaz Oct 25, 2019

Author Contributor

In other words, should we upstream the changes in #7601 ?

This comment has been minimized.

Sign in to view
@nox

nox Oct 25, 2019

Member

No clue, I don't know if that conflicts with how stringifiers are handled through SpiderMonkey. Or it could be a case of no one adding support for [Pure] on them in the Gecko side.

This comment has been minimized.

Sign in to view
@saschanaz

saschanaz Oct 25, 2019

Author Contributor

There is no blocker to add [Pure] support on stringifier attributes, it's just that no one was bothered to add one on DOMTokenList or to do what #7601 did.

@bzbarsky, thoughts?

This comment has been minimized.

Sign in to view
@jdm

jdm Oct 25, 2019

Member

I'm not actually confident that Pure has any meaning in Servo's implementation. It may have been copied from upstream webidl in Gecko.

This comment has been minimized.

Sign in to view
@nox

nox Oct 25, 2019

Member

I'm not actually confident that Pure has any meaning in Servo's implementation. It may have been copied from upstream webidl in Gecko.

WebIDL.py translates [Pure] and [Constant] to specific [Affects]/[DependsOn] combinations, and you can actually find references to those in CodegenRust.py.

This comment has been minimized.

Sign in to view
@nox

nox Oct 25, 2019

Member

(Submitted too fast.)

And in #7601 I went out of my way to make use of them more extensively so those weren't there by coincidence.

This comment has been minimized.

Sign in to view
@bzbarsky

bzbarsky Oct 25, 2019

Contributor

Fwiw, I think we should add support for [Pure] in the parser here and probably add it in the Gecko IDL.
@bors-servo
Copy link
Contributor

bors-servo commented Oct 25, 2019

💔 Test failed - linux-rel-wpt
@nox
nox reviewed Oct 25, 2019
View changes
Copy link
Member

nox left a comment

Where do the WebIDL parser and tests changes come from? Upstream? If not, you are not supposed to change those files.
@saschanaz
Copy link
Contributor Author

saschanaz commented Oct 25, 2019

Where do the WebIDL parser and tests changes come from? Upstream? If not, you are not supposed to change those files.

From https://phabricator.services.mozilla.com/D48355 😀
@nox
Copy link
Member

nox commented Oct 25, 2019

Cool. Then please make a first commit with the vendoring of those new changes (with the update.sh script) and then a second commit with the rest of the changes.
@saschanaz saschanaz force-pushed the saschanaz:stringifier-attr branch from 9d5d6fd to fa41436 Oct 25, 2019
@saschanaz saschanaz force-pushed the saschanaz:stringifier-attr branch from fa41436 to e1eacc0 Oct 25, 2019
@saschanaz
Copy link
Contributor Author

saschanaz commented Oct 25, 2019

@bors-servo try=wpt
@bors-servo
Copy link
Contributor

bors-servo commented Oct 25, 2019

⌛ Trying commit e1eacc0 with merge a14f9c4...
bors-servo added a commit that referenced this pull request Oct 25, 2019
@bors-servo
Auto merge of #24544 - saschanaz:stringifier-attr, r=<try>
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
a14f9c4 
Support IDL stringifier attributes

<!-- 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
- [x] These changes fix #7590

<!-- Either: -->
- [x] There are tests for these changes

<!-- 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
Copy link
Contributor

bors-servo commented Oct 25, 2019

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
State: approved= try=True
saschanaz added 2 commits Oct 25, 2019
@saschanaz
Update WebIDL.py
97c01fc
@saschanaz
Support stringifier attributes
Loading status checks…
691af0e
@saschanaz saschanaz force-pushed the saschanaz:stringifier-attr branch from e1eacc0 to 691af0e Oct 29, 2019
@saschanaz
Copy link
Contributor Author

saschanaz commented Oct 29, 2019

@bors-servo try=wpt
@bors-servo
Copy link
Contributor

bors-servo commented Oct 29, 2019

⌛ Trying commit 691af0e with merge 9ab0229...
bors-servo added a commit that referenced this pull request Oct 29, 2019
@bors-servo
Auto merge of #24544 - saschanaz:stringifier-attr, r=<try>
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
9ab0229 
Support IDL stringifier attributes

<!-- 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
- [x] These changes fix #7590

<!-- Either: -->
- [x] There are tests for these changes

<!-- 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. -->
@jdm
jdm approved these changes Oct 29, 2019
View changes
@bors-servo
Copy link
Contributor

bors-servo commented Oct 29, 2019

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
State: approved= try=True
@jdm
Copy link
Member

jdm commented Oct 29, 2019

@bors-servo r=nox,jdm
@bors-servo
Copy link
Contributor

bors-servo commented Oct 29, 2019

📌 Commit 691af0e has been approved by nox,jdm
@bors-servo
Copy link
Contributor

bors-servo commented Oct 29, 2019

⌛ Testing commit 691af0e with merge 5ba4d6b...
bors-servo added a commit that referenced this pull request Oct 29, 2019
@bors-servo
Auto merge of #24544 - saschanaz:stringifier-attr, r=nox,jdm
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
5ba4d6b 
Support IDL stringifier attributes

<!-- 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
- [x] These changes fix #7590

<!-- Either: -->
- [x] There are tests for these changes

<!-- 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
Copy link
Contributor

bors-servo commented Oct 29, 2019

💔 Test failed - status-taskcluster
@jdm
Copy link
Member

jdm commented Oct 29, 2019

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Oct 29, 2019

💣 Failed to start rebuilding: Unknown error
bors-servo added a commit that referenced this pull request Oct 29, 2019
@bors-servo
Auto merge of #24544 - saschanaz:stringifier-attr, r=nox,jdm
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
aa46cbd 
Support IDL stringifier attributes

<!-- 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
- [x] These changes fix #7590

<!-- Either: -->
- [x] There are tests for these changes

<!-- 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
Copy link
Contributor

bors-servo commented Oct 29, 2019

⌛ Testing commit 691af0e with merge aa46cbd...
@bors-servo
Copy link
Contributor

bors-servo commented Oct 29, 2019

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: nox,jdm
Pushing aa46cbd to master...
@bors-servo bors-servo merged commit 691af0e into servo:master Oct 29, 2019
2 checks passed
2 checks passed
Taskcluster (pull_request) TaskGroup: success
Details
homu Test successful
Details
@saschanaz saschanaz deleted the saschanaz:stringifier-attr branch Nov 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nox nox

@bzbarsky bzbarsky

@jdm jdm

Assignees

@nox nox

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Support for WebIDL stringifier attributes
7 participants
@saschanaz @highfive @bors-servo @nox @jdm @bzbarsky @Manishearth
You can’t perform that action at this time.