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

Support WebIDL `record<>` #24377

Merged
merged 1 commit into from Oct 15, 2019
Merged

Support WebIDL `record<>` #24377

merged 1 commit into from Oct 15, 2019

Conversation

@saschanaz
Copy link
Contributor

saschanaz commented Oct 5, 2019

Rebased @taki-zaro's work (#20318).


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #15012 and closes #20318. Possibly also closes #21463.
  • There are tests for these changes

This change is Reviewable

@highfive
Copy link

highfive commented Oct 5, 2019

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/testbinding.rs, components/script/dom/bindings/mod.rs, components/script/dom/bindings/mozmap.rs, components/script/dom/bindings/record.rs and 2 more
  • @KiChjang: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/testbinding.rs, components/script/dom/bindings/mod.rs, components/script/dom/bindings/mozmap.rs, components/script/dom/bindings/record.rs and 2 more
@highfive
Copy link

highfive commented Oct 5, 2019

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@saschanaz
Copy link
Contributor Author

saschanaz commented Oct 5, 2019

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

bors-servo commented Oct 5, 2019

Trying commit 32913ef with merge ec4ae48...

bors-servo added a commit that referenced this pull request Oct 5, 2019
Support WebIDL `record<>`

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

Rebased @taki-zaro's work (#20318).

---
<!-- 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 #15012 and closes #20318

<!-- 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 5, 2019

💔 Test failed - linux-rel-wpt

@CYBAI
Copy link
Collaborator

CYBAI commented Oct 5, 2019

Cool! Looks like more PASS tests 👀

{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "Basic operation with one property", 
    "test": "/fetch/api/headers/headers-record.html", 
    "line": 77071, 
    "action": "test_result", 
    "expected": "FAIL"
}
{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "Basic operation with one property and a proto", 
    "test": "/fetch/api/headers/headers-record.html", 
    "line": 77072, 
    "action": "test_result", 
    "expected": "FAIL"
}
{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "Correct operation ordering with two properties", 
    "test": "/fetch/api/headers/headers-record.html", 
    "line": 77073, 
    "action": "test_result", 
    "expected": "FAIL"
}
{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "Correct operation ordering with two properties one of which has an invalid value", 
    "test": "/fetch/api/headers/headers-record.html", 
    "line": 77075, 
    "action": "test_result", 
    "expected": "FAIL"
}
{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "Correct operation ordering with non-enumerable properties", 
    "test": "/fetch/api/headers/headers-record.html", 
    "line": 77076, 
    "action": "test_result", 
    "expected": "FAIL"
}
{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "Correct operation ordering with undefined descriptors", 
    "test": "/fetch/api/headers/headers-record.html", 
    "line": 77077, 
    "action": "test_result", 
    "expected": "FAIL"
}
{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "Operation with non-enumerable Symbol keys", 
    "test": "/fetch/api/headers/headers-record.html", 
    "line": 77080, 
    "action": "test_result", 
    "expected": "FAIL"
}
{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "URLSearchParams constructor, DOMException as argument", 
    "test": "/url/urlsearchparams-constructor.any.worker.html", 
    "line": 136213, 
    "action": "test_result", 
    "expected": "FAIL"
}
{
    "status": "PASS", 
    "group": "default", 
    "message": null, 
    "stack": null, 
    "subtest": "URLSearchParams constructor, DOMException as argument", 
    "test": "/url/urlsearchparams-constructor.any.html", 
    "line": 229104, 
    "action": "test_result", 
    "expected": "FAIL"
}
@highfive highfive removed the S-tests-failed label Oct 5, 2019
@saschanaz
Copy link
Contributor Author

saschanaz commented Oct 5, 2019

@bors-servo try=wpt

Two Headers tests are still failing, need to investigate. DevTools connection fails on both stable and nightly Firefox, is it #24092?

@bors-servo
Copy link
Contributor

bors-servo commented Oct 5, 2019

Trying commit 935a084 with merge d71a49c...

bors-servo added a commit that referenced this pull request Oct 5, 2019
Support WebIDL `record<>`

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

Rebased @taki-zaro's work (#20318).

---
<!-- 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 #15012 and closes #20318

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

<!-- 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/24377)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 5, 2019

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
State: approved= try=True

@saschanaz saschanaz force-pushed the saschanaz:record-support branch from 6644653 to 1c959db Oct 6, 2019
@saschanaz
Copy link
Contributor Author

saschanaz commented Oct 6, 2019

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

bors-servo commented Oct 6, 2019

Trying commit 6c96ad6 with merge b08fe84...

bors-servo added a commit that referenced this pull request Oct 6, 2019
Support WebIDL `record<>`

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

Rebased @taki-zaro's work (#20318).

---
<!-- 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 #15012 and closes #20318

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

<!-- 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/24377)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 6, 2019

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
State: approved= try=True

Copy link
Member

nox left a comment

Some changes and a couple of questions.

components/script/dom/bindings/codegen/CodegenRust.py Outdated Show resolved Hide resolved
components/script/dom/bindings/record.rs Outdated Show resolved Hide resolved
components/script/dom/bindings/record.rs Show resolved Hide resolved
components/script/dom/bindings/record.rs Outdated Show resolved Hide resolved
components/script/dom/testbinding.rs Outdated Show resolved Hide resolved
@saschanaz
Copy link
Contributor Author

saschanaz commented Oct 8, 2019

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

bors-servo commented Oct 8, 2019

Trying commit 04489ed with merge a754f7e...

bors-servo added a commit that referenced this pull request Oct 8, 2019
Support WebIDL `record<>`

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

Rebased @taki-zaro's work (#20318).

---
<!-- 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 #15012 and closes #20318. Possibly also closes #21463.

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

<!-- 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/24377)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 8, 2019

💔 Test failed - linux-rel-wpt

@saschanaz
Copy link
Contributor Author

saschanaz commented Oct 8, 2019

@bors-servo retry

@bors-servo
Copy link
Contributor

bors-servo commented Oct 8, 2019

Trying commit 04489ed with merge a4e3b90...

bors-servo added a commit that referenced this pull request Oct 8, 2019
Support WebIDL `record<>`

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

Rebased @taki-zaro's work (#20318).

---
<!-- 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 #15012 and closes #20318. Possibly also closes #21463.

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

<!-- 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/24377)
<!-- Reviewable:end -->
@saschanaz
Copy link
Contributor Author

saschanaz commented Oct 14, 2019

@nox, r?

@nox
Copy link
Member

nox commented Oct 15, 2019

Please squash all the commits together so I can start reviewing one last time.

@saschanaz
Copy link
Contributor Author

saschanaz commented Oct 15, 2019

Everything in one commit?

Edit: Done, feel free to ping if this wasn't what you wanted as I got a backup.

@saschanaz saschanaz force-pushed the saschanaz:record-support branch from b945205 to b697621 Oct 15, 2019
@nox
Copy link
Member

nox commented Oct 15, 2019

Looks good!

@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Oct 15, 2019

📌 Commit b697621 has been approved by nox

@bors-servo
Copy link
Contributor

bors-servo commented Oct 15, 2019

Testing commit b697621 with merge dce8f93...

bors-servo added a commit that referenced this pull request Oct 15, 2019
Support WebIDL `record<>`

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

Rebased @taki-zaro's work (#20318).

---
<!-- 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 #15012 and closes #20318. Possibly also closes #21463.

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

<!-- 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/24377)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 15, 2019

💔 Test failed - status-taskcluster

@saschanaz
Copy link
Contributor Author

saschanaz commented Oct 15, 2019

@bors-servo retry

@jdm
Copy link
Member

jdm commented Oct 15, 2019

@bors-servo
Copy link
Contributor

bors-servo commented Oct 15, 2019

💣 Failed to start rebuilding: Unknown error

@bors-servo
Copy link
Contributor

bors-servo commented Oct 15, 2019

Testing commit b697621 with merge c5d6bb6...

bors-servo added a commit that referenced this pull request Oct 15, 2019
Support WebIDL `record<>`

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

Rebased @taki-zaro's work (#20318).

---
<!-- 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 #15012 and closes #20318. Possibly also closes #21463.

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

<!-- 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/24377)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 15, 2019

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: nox
Pushing c5d6bb6 to master...

bors-servo added a commit that referenced this pull request Oct 15, 2019
Return false when GetPropertyKeys fails

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

As stated in #24377 (comment).

---
<!-- 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 #___ (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 bors-servo merged commit b697621 into servo:master Oct 15, 2019
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
Taskcluster (pull_request) TaskGroup: success
Details
homu Test successful
Details
bors-servo added a commit that referenced this pull request Oct 15, 2019
Return false when GetPropertyKeys fails

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

As stated in #24377 (comment).

---
<!-- 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 #___ (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 issues

Successfully merging this pull request may close these issues.

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