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

Add generic to frozen array #25468

Merged
merged 2 commits into from Jan 12, 2020
Merged

Add generic to frozen array #25468

merged 2 commits into from Jan 12, 2020

Conversation

@gterzian
Copy link
Member

gterzian commented Jan 9, 2020

#25467 (comment)

Depends on #25467

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #___ (GitHub issue number if applicable)
  • There are tests for these changes OR
  • These changes do not require tests because ___
@highfive
Copy link

highfive commented Jan 9, 2020

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/messageevent.rs, components/script/dom/extendablemessageevent.rs, components/script/dom/bindings/utils.rs
  • @KiChjang: components/script/dom/messageevent.rs, components/script/dom/extendablemessageevent.rs, components/script/dom/bindings/utils.rs
@highfive
Copy link

highfive commented Jan 9, 2020

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify script code, but no tests are modified. Please consider adding a test!
@gterzian
Copy link
Member Author

gterzian commented Jan 9, 2020

@Manishearth r?
@Manishearth
Manishearth reviewed Jan 9, 2020
View changes
components/script/dom/messageevent.rs
@@ -219,7 +238,13 @@ impl MessageEventMethods for MessageEvent {

/// <https://html.spec.whatwg.org/multipage/#dom-messageevent-ports>
fn Ports(&self, cx: JSContext) -> JSVal {
message_ports_to_frozen_array(self.ports.borrow().as_slice(), cx)
let ports: Vec<DomRoot<MessagePort>> = self

This comment has been minimized.

Sign in to view
@Manishearth

Manishearth Jan 9, 2020

Member

It's actually worth caching this in a MutNullableDom<Heap> or something. The spec doesn't say it's SameObject, but it probably should, and even if it doesn't we can still do it.

This comment has been minimized.

Sign in to view
@Manishearth

Manishearth Jan 9, 2020

Member

Doesn't have to be this PR

This comment has been minimized.

Sign in to view
@gterzian

gterzian Jan 9, 2020

Author Member

Ok, I will look into it.
@Manishearth
Manishearth approved these changes Jan 9, 2020
View changes
@gterzian gterzian force-pushed the gterzian:add_generic_to_frozen_array branch from f02bfca to d04c76d Jan 11, 2020
@gterzian
add a generic utility to convert a slice into a frozen array
e768ca1
@gterzian gterzian force-pushed the gterzian:add_generic_to_frozen_array branch from d04c76d to 0f5742c Jan 11, 2020
@servo-wpt-sync
Copy link
Collaborator

servo-wpt-sync commented Jan 11, 2020

Error syncing changes upstream. Logs saved in error-snapshot-1578753799580.
@gterzian gterzian force-pushed the gterzian:add_generic_to_frozen_array branch from 0f5742c to 33f82a6 Jan 11, 2020
@gterzian
Copy link
Member Author

gterzian commented Jan 11, 2020

@Manishearth r?
@Manishearth
Copy link
Member

Manishearth commented Jan 12, 2020

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Jan 12, 2020

📌 Commit 33f82a6 has been approved by Manishearth
@highfive highfive assigned Manishearth and unassigned paulrouget Jan 12, 2020
@bors-servo
Copy link
Contributor

bors-servo commented Jan 12, 2020

⌛ Testing commit 33f82a6 with merge 1eeecf0...
bors-servo added a commit that referenced this pull request Jan 12, 2020
@bors-servo
Auto merge of #25468 - gterzian:add_generic_to_frozen_array, r=Manish…
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…
1eeecf0 
…earth

Add generic to frozen array

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

#25467 (comment)

Depends on #25467

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./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
Copy link
Contributor

bors-servo commented Jan 12, 2020

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

Manishearth commented Jan 12, 2020 

1 unexpected results that are NOT known-intermittents:
  ▶ Unexpected subtest result in /html/webappapis/scripting/events/messageevent-constructor.https.html:
  └ PASS [expected FAIL] ports attribute should be a FrozenArray
@gterzian
Copy link
Member Author

gterzian commented Jan 12, 2020 •
edited

assert_true(ev.ports === ev.ports, "ev.ports should return the same object"), so that pass is due to the caching...
@gterzian
cache frozen array of ports
Loading status checks…
0339601
@gterzian gterzian force-pushed the gterzian:add_generic_to_frozen_array branch from 33f82a6 to 0339601 Jan 12, 2020
@gterzian
Copy link
Member Author

gterzian commented Jan 12, 2020

@bors-servo r=Manishearth
@bors-servo
Copy link
Contributor

bors-servo commented Jan 12, 2020

📌 Commit 0339601 has been approved by Manishearth
@bors-servo
Copy link
Contributor

bors-servo commented Jan 12, 2020

⌛ Testing commit 0339601 with merge 2a36c00...
bors-servo added a commit that referenced this pull request Jan 12, 2020
@bors-servo
Auto merge of #25468 - gterzian:add_generic_to_frozen_array, r=Manish…
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…
2a36c00 
…earth

Add generic to frozen array

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

#25467 (comment)

Depends on #25467

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./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
Copy link
Contributor

bors-servo commented Jan 12, 2020

💔 Test failed - status-taskcluster
@gterzian
Copy link
Member Author

gterzian commented Jan 12, 2020

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

bors-servo commented Jan 12, 2020

⌛ Testing commit 0339601 with merge a9b4e31...
bors-servo added a commit that referenced this pull request Jan 12, 2020
@bors-servo
Auto merge of #25468 - gterzian:add_generic_to_frozen_array, r=Manish…
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…
a9b4e31 
…earth

Add generic to frozen array

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

#25467 (comment)

Depends on #25467

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./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
Copy link
Contributor

bors-servo commented Jan 12, 2020

💔 Test failed - status-taskcluster
@gterzian
Copy link
Member Author

gterzian commented Jan 12, 2020

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

bors-servo commented Jan 12, 2020

⌛ Testing commit 0339601 with merge dbba9ea...
bors-servo added a commit that referenced this pull request Jan 12, 2020
@bors-servo
Auto merge of #25468 - gterzian:add_generic_to_frozen_array, r=Manish…
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…
dbba9ea 
…earth

Add generic to frozen array

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

#25467 (comment)

Depends on #25467

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [ ] `./mach build -d` does not report any errors
- [ ] `./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
Copy link
Contributor

bors-servo commented Jan 12, 2020

☀️ Test successful - status-taskcluster
Approved by: Manishearth
Pushing dbba9ea to master...
@bors-servo bors-servo merged commit 0339601 into servo:master Jan 12, 2020
2 checks passed
2 checks passed
Community-TC (pull_request) TaskGroup: success
Details
homu Test successful
Details
@gterzian gterzian deleted the gterzian:add_generic_to_frozen_array branch Jan 12, 2020
@bors-servo bors-servo mentioned this pull request Jan 12, 2020
Merged
4 of 5 tasks complete
@pshaughn pshaughn mentioned this pull request Jan 27, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Manishearth Manishearth

Assignees

@Manishearth Manishearth

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@gterzian @highfive @servo-wpt-sync @Manishearth @bors-servo @paulrouget
You can’t perform that action at this time.