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

Update CustomElementConstructor type and exception #23202

Closed
jdm opened this issue Apr 15, 2019 · 3 comments · Fixed by #23530
Closed

Update CustomElementConstructor type and exception #23202

jdm opened this issue Apr 15, 2019 · 3 comments · Fixed by #23530
Labels
A-content/dom Interacting with the DOM from web content I-spec-update

Comments

@jdm
Copy link
Member

jdm commented Apr 15, 2019

The IDL for CustomElementConstructor was changed in whatwg/html#4525. We should update that as well as the exception used by this code.

To test these changes, we will need the tests in web-platform-tests/wpt#16328.

@jdm jdm added A-content/dom Interacting with the DOM from web content I-spec-update labels Apr 15, 2019
@ferjm ferjm changed the title Update CusomElementConstructor type and exception Update CustomElementConstructor type and exception Apr 16, 2019
@gatoWololo
Copy link

Cool. I think I understand this. I can do it.

@gatoWololo
Copy link

We should update that as well as the exception used by this code.

I have found 2 places where we need to change from Error::InvalidState to Error::Type. The one you pointed out above. and

Some(ConstructionStackEntry::AlreadyConstructedMarker) => Err(Error::InvalidState),

To test these changes, we will need the tests in web-platform-tests/wpt#16328.

The tests are already in Servo here and here, and they are the latest version using TypeError.

After changing to Error::Type and running:
./mach test ./tests/wpt/web-platform-tests/custom-elements/upgrading/upgrading-parser-created-element.html
./mach test ./tests/wpt/web-platform-tests/custom-elements/upgrading/Node-cloneNode.html

I can confirm all the [expected FAIL] tests related to TypeError, now pass :)

My only question: I'm not sure what String to pass to Error::Type(String)?
Currently, I have something like:

        if !same {
            return Err(Error::Type("Returned element is not SameValue as the upgraded element".to_string()));

Is this what's expected? Or is there somewhere specific to get the correct string from?

@jdm
Copy link
Member Author

jdm commented Jun 6, 2019

Strings are implementation-defined, so that one seems fine.

gatoWololo pushed a commit to gatoWololo/servo that referenced this issue Jun 7, 2019
gatoWololo pushed a commit to gatoWololo/servo that referenced this issue Jun 7, 2019
bors-servo pushed a commit that referenced this issue Jun 7, 2019
…r_type, r=cybai,jdm

Use TypeError instead of InvalidState for exception.

<!-- Please describe your changes on the following line: -->
Use TypeError instead of InvalidState as per whatwg/html#4525 for making CustomElementConstructor exceptions more consistent.

<!-- 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 #23202  (GitHub issue number if applicable)

<!-- Either: -->
- [X] There are tests for these changes: Tests that expected failure now expect passing with correct result.

<!-- 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/23530)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Jun 7, 2019
…r_type, r=cybai,jdm

Use TypeError instead of InvalidState for exception.

<!-- Please describe your changes on the following line: -->
Use TypeError instead of InvalidState as per whatwg/html#4525 for making CustomElementConstructor exceptions more consistent.

<!-- 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 #23202  (GitHub issue number if applicable)

<!-- Either: -->
- [X] There are tests for these changes: Tests that expected failure now expect passing with correct result.

<!-- 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/23530)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Jun 7, 2019
…r_type, r=cybai,jdm

Use TypeError instead of InvalidState for exception.

<!-- Please describe your changes on the following line: -->
Use TypeError instead of InvalidState as per whatwg/html#4525 for making CustomElementConstructor exceptions more consistent.

<!-- 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 #23202  (GitHub issue number if applicable)

<!-- Either: -->
- [X] There are tests for these changes: Tests that expected failure now expect passing with correct result.

<!-- 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/23530)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Jun 8, 2019
…r_type, r=cybai,jdm

Use TypeError instead of InvalidState for exception.

<!-- Please describe your changes on the following line: -->
Use TypeError instead of InvalidState as per whatwg/html#4525 for making CustomElementConstructor exceptions more consistent.

<!-- 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 #23202  (GitHub issue number if applicable)

<!-- Either: -->
- [X] There are tests for these changes: Tests that expected failure now expect passing with correct result.

<!-- 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/23530)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Jun 8, 2019
…r_type, r=cybai,jdm

Use TypeError instead of InvalidState for exception.

<!-- Please describe your changes on the following line: -->
Use TypeError instead of InvalidState as per whatwg/html#4525 for making CustomElementConstructor exceptions more consistent.

<!-- 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 #23202  (GitHub issue number if applicable)

<!-- Either: -->
- [X] There are tests for these changes: Tests that expected failure now expect passing with correct result.

<!-- 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/23530)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Jun 8, 2019
…r_type, r=cybai,jdm

Use TypeError instead of InvalidState for exception.

<!-- Please describe your changes on the following line: -->
Use TypeError instead of InvalidState as per whatwg/html#4525 for making CustomElementConstructor exceptions more consistent.

<!-- 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 #23202  (GitHub issue number if applicable)

<!-- Either: -->
- [X] There are tests for these changes: Tests that expected failure now expect passing with correct result.

<!-- 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/23530)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this issue Jun 8, 2019
…r_type, r=cybai,jdm

Use TypeError instead of InvalidState for exception.

<!-- Please describe your changes on the following line: -->
Use TypeError instead of InvalidState as per whatwg/html#4525 for making CustomElementConstructor exceptions more consistent.

<!-- 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 #23202  (GitHub issue number if applicable)

<!-- Either: -->
- [X] There are tests for these changes: Tests that expected failure now expect passing with correct result.

<!-- 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/23530)
<!-- Reviewable:end -->
oneturkmen pushed a commit to oneturkmen/servo that referenced this issue Jun 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-content/dom Interacting with the DOM from web content I-spec-update
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants