Skip to content

Add SupportedPropertyNames to Document (also fix iframe getting)#25548

Merged
bors-servo merged 2 commits intoservo:masterfrom
pshaughn:docnamedgetter
Feb 14, 2020
Merged

Add SupportedPropertyNames to Document (also fix iframe getting)#25548
bors-servo merged 2 commits intoservo:masterfrom
pshaughn:docnamedgetter

Conversation

@pshaughn
Copy link
Copy Markdown
Contributor

@pshaughn pshaughn commented Jan 17, 2020 •
edited
Loading

Existing test of named-getting an iframe now succeeds. I added a new test for Object.getOwnPropertyNames(document) based on my understanding of the spec; that test could use a second opinion.

UPDATE: This was trying to do too many things in one PR as originally submitted. It is now using #25572 as a base, and I suggest reviewing that PR before this one to avoid duplicating review effort.

  • There are tests for these changes

@highfive
Copy link
Copy Markdown

highfive commented Jan 17, 2020

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/document.rs, components/script/dom/shadowroot.rs, components/script/dom/element.rs, components/script/dom/raredata.rs, components/script/dom/documentorshadowroot.rs
  • @KiChjang: components/script/dom/document.rs, components/script/dom/shadowroot.rs, components/script/dom/element.rs, components/script/dom/raredata.rs, components/script/dom/documentorshadowroot.rs

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jan 17, 2020
@highfive
Copy link
Copy Markdown

highfive commented Jan 17, 2020

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

@servo-wpt-sync servo-wpt-sync mentioned this pull request Jan 17, 2020
Merged
@servo-wpt-sync
Copy link
Copy Markdown
Collaborator

servo-wpt-sync commented Jan 17, 2020

Opened new PR for upstreamable changes.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#21247.

This was referenced Jan 20, 2020
Open
Closed
bors-servo pushed a commit that referenced this pull request Jan 21, 2020
Auto merge of #25562 - pshaughn:window-getter, r=<try>
57d0b84 
Attempt at window named getter, sharing name_map infrastructure with document named getter

I think I have the window named getter working well enough to look at CI results. This has #25548 as a base but also pulls in most of #21869.
bors-servo pushed a commit that referenced this pull request Jan 21, 2020
Auto merge of #25562 - pshaughn:window-getter, r=<try>
a936850 
Attempt at window named getter, sharing name_map infrastructure with document named getter

I think I have the window named getter working well enough to look at CI results. This has #25548 as a base but also pulls in most of #21869.
bors-servo pushed a commit that referenced this pull request Jan 21, 2020
Auto merge of #25562 - pshaughn:window-getter, r=<try>
759ad3e 
Attempt at window named getter, sharing name_map infrastructure with document named getter

I think I have the window named getter working well enough to look at CI results. This has #25548 as a base but also pulls in most of #21869.
@pshaughn pshaughn mentioned this pull request Jan 21, 2020
Closed
@pshaughn
Copy link
Copy Markdown
Contributor Author

pshaughn commented Jan 21, 2020

This is broken right now and needs #25570 fixed as a foundation so element names don't sneak out of the atom cache.

@servo-wpt-sync
Copy link
Copy Markdown
Collaborator

servo-wpt-sync commented Jan 22, 2020

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#21247.

@pshaughn
Copy link
Copy Markdown
Contributor Author

pshaughn commented Jan 22, 2020

Now using #25572 as a base, which should prevent crashes. I also took into account review comments by @Manishearth from #25562 (which I plan on updating to use this as a base, if this seems solid enough.)
@bors-servo try

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Jan 22, 2020

⌛ Trying commit 7a2456a with merge 2cbc26e...

bors-servo pushed a commit that referenced this pull request Jan 22, 2020
Auto merge of #25548 - pshaughn:docnamedgetter, r=<try>
2cbc26e 
Add SupportedPropertyNames to Document (also fix iframe getting)

Existing test of named-getting an iframe now succeeds. I added a new test for Object.getOwnPropertyNames(document) based on my understanding of the spec; that test could use a second opinion.

UPDATE: This was trying to do too many things in one PR as originally submitted. It is now using #25572 as a base, and I suggest reviewing that PR before this one to avoid duplicating review effort.

---
<!-- 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 #7273 for all implemented named getters, fix #25146, and fix the iframe case only of #25145.

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

pshaughn commented Jan 22, 2020

(forgot to account for comment #25562 (comment), so as of this push the new supported property names test is still broken out into a bunch of really small cases)

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Jan 22, 2020

☀️ Test successful - status-taskcluster
State: approved= try=True

jdm
jdm approved these changes Feb 12, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@jdm jdm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! I found this to be quite readable, which is better than the attempt that I made at it originally!

@jdm jdm added S-needs-rebase There are merge conflict errors. S-blocked-on-external Something, somewhere else, needs to happen before this PR can be merged. and removed S-awaiting-review There is new code that needs to be reviewed. labels Feb 12, 2020
@jdm jdm assigned jdm and unassigned asajeffrey Feb 12, 2020
@jdm
Copy link
Copy Markdown
Member

jdm commented Feb 12, 2020

This can merge after a rebase and #25572 is merged.

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Feb 13, 2020
@servo-wpt-sync
Copy link
Copy Markdown
Collaborator

servo-wpt-sync commented Feb 13, 2020

Transplanted upstreamable changes to existing PR.

Completed upstream sync of web-platform-test changes at web-platform-tests/wpt#21247.

@jdm
Copy link
Copy Markdown
Member

jdm commented Feb 13, 2020

@bors-servo r+

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Feb 13, 2020

📌 Commit e48eac6 has been approved by jdm

@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. labels Feb 13, 2020
bors-servo pushed a commit that referenced this pull request Feb 13, 2020
Auto merge of #25562 - pshaughn:window-getter, r=<try>
4249af7 
Attempt at window named getter, sharing name_map infrastructure with document named getter

I think I have the window named getter working well enough to look at CI results. This has #25548 as a base but also pulls in most of #21869.
bors-servo pushed a commit that referenced this pull request Feb 13, 2020
Auto merge of #25562 - pshaughn:window-getter, r=<try>
007955d 
Attempt at window named getter, sharing name_map infrastructure with document named getter

I think I have the window named getter working well enough to look at CI results. This has #25548 as a base but also pulls in most of #21869.
bors-servo pushed a commit that referenced this pull request Feb 13, 2020
Auto merge of #25548 - pshaughn:docnamedgetter, r=jdm
f59f110 
Add SupportedPropertyNames to Document (also fix iframe getting)

Existing test of named-getting an iframe now succeeds. I added a new test for Object.getOwnPropertyNames(document) based on my understanding of the spec; that test could use a second opinion.

UPDATE: This was trying to do too many things in one PR as originally submitted. It is now using #25572 as a base, and I suggest reviewing that PR before this one to avoid duplicating review effort.

---
<!-- 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 #7273 for all implemented named getters, fix #25146, and fix the iframe case only of #25145.

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

bors-servo commented Feb 13, 2020

⌛ Testing commit e48eac6 with merge f59f110...

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Feb 13, 2020

💔 Test failed - status-taskcluster

@highfive highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Feb 13, 2020
@jdm
Copy link
Copy Markdown
Member

jdm commented Feb 13, 2020

@bors-servo retry

@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Feb 14, 2020

⌛ Testing commit e48eac6 with merge f020536...

bors-servo pushed a commit that referenced this pull request Feb 14, 2020
Auto merge of #25548 - pshaughn:docnamedgetter, r=jdm
f020536 
Add SupportedPropertyNames to Document (also fix iframe getting)

Existing test of named-getting an iframe now succeeds. I added a new test for Object.getOwnPropertyNames(document) based on my understanding of the spec; that test could use a second opinion.

UPDATE: This was trying to do too many things in one PR as originally submitted. It is now using #25572 as a base, and I suggest reviewing that PR before this one to avoid duplicating review effort.

---
<!-- 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 #7273 for all implemented named getters, fix #25146, and fix the iframe case only of #25145.

<!-- 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. -->
@highfive highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Feb 14, 2020
@bors-servo
Copy link
Copy Markdown
Contributor

bors-servo commented Feb 14, 2020

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing f020536 to master...

@bors-servo bors-servo merged commit e48eac6 into servo:master Feb 14, 2020
@highfive highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Feb 14, 2020
@bors-servo bors-servo mentioned this pull request Feb 14, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jdm jdm jdm approved these changes

Assignees

@jdm jdm

Labels

S-blocked-on-external Something, somewhere else, needs to happen before this PR can be merged.

Projects

None yet

Milestone

No milestone

6 participants

@pshaughn @highfive @servo-wpt-sync @bors-servo @jdm @asajeffrey