Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upImplement HTMLSelectElement.add() and indexed setter #25449
Conversation
highfive
commented
Jan 7, 2020
|
Heads up! This PR modifies the following files:
|
|
@bors-servo r+ |
|
|
Implement HTMLSelectElement.add() and indexed setter HTMLSelectElement.add and its indexed setter just needed to forward to matching methods in HTMLOptionsCollection, which they now do. It was also necessary to change codegen slightly; it had accidentally assumed that if an indexed setter existed, a named getter or setter also would. I expect this and #25446 will combine to pass more tests than either alone does. --- <!-- 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 #25003 <!-- 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. -->
|
|
feel free to tweak that test (or remove it) if it's wrong |
|
That test is both right and interesting. It's making sure that, if script calls a method that takes a union type using an argument that's incorrect for that union, codegen code throws a TypeError at the script. This was working when the method didn't have the [Throws] extended attribute, and now it isn't working. |
|
I stand corrected; the TypeError is still getting thrown, and some other line is throwing a NotFoundError for reasons as yet unclear. |
…as relying on add to be a stub
|
The test only worked because add() wasn't actually doing anything; I wasn't sure which of two behaviors was the one really being tested for so now it tries for both. |
| @@ -14053,7 +14051,7 @@ | |||
| ], | |||
| "css/border_black_groove.html": [ | |||
| "49e1647a6f71e320770225ad537b4fd4020bd700", | |||
| "reftest_node" | |||
This comment has been minimized.
This comment has been minimized.
pshaughn
Jan 7, 2020
Author
Member
I don't know what this means; I just ran update-manifest as usual and it changed this!
This comment has been minimized.
This comment has been minimized.
|
@bors-servo r+ |
|
|
Implement HTMLSelectElement.add() and indexed setter HTMLSelectElement.add and its indexed setter just needed to forward to matching methods in HTMLOptionsCollection, which they now do. It was also necessary to change codegen slightly; it had accidentally assumed that if an indexed setter existed, a named getter or setter also would. I expect this and #25446 will combine to pass more tests than either alone does. --- <!-- 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 #25003 <!-- 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. -->
|
|
pshaughn commentedJan 7, 2020
HTMLSelectElement.add and its indexed setter just needed to forward to matching methods in HTMLOptionsCollection, which they now do. It was also necessary to change codegen slightly; it had accidentally assumed that if an indexed setter existed, a named getter or setter also would.
I expect this and #25446 will combine to pass more tests than either alone does.
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors