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 OpenEndedDictionary (Mozmap) in the Headers #13356

Merged
merged 1 commit into from Sep 23, 2016

Conversation

@jeenalee
Copy link
Contributor

jeenalee commented Sep 21, 2016

This PR will support OpenEndedDictionary (based on MozMap implementation from #13332) in the Headers API.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #11897 (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable

@highfive
Copy link

highfive commented Sep 21, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/request.rs, components/script/dom/bindings/mozmap.rs, components/script/dom/webidls/Headers.webidl, components/script/dom/headers.rs
@jeenalee
Copy link
Contributor Author

jeenalee commented Sep 21, 2016

r? @jdm

@highfive highfive assigned jdm and unassigned larsbergstrom Sep 21, 2016
Copy link
Member

jdm left a comment

I'm operating under the assumption that all of the tests now timing out require Promises. These changes look good!

@@ -14,7 +14,7 @@ use dom::bindings::codegen::Bindings::RequestBinding::RequestMethods;
use dom::bindings::codegen::Bindings::RequestBinding::RequestMode;
use dom::bindings::codegen::Bindings::RequestBinding::RequestRedirect;
use dom::bindings::codegen::Bindings::RequestBinding::RequestType;
use dom::bindings::codegen::UnionTypes::HeadersOrByteStringSequenceSequence;
use dom::bindings::codegen::UnionTypes::HeadersOrByteStringSequenceSequenceOrByteStringMozMap as HeadersInitType;

This comment has been minimized.

@jdm

jdm Sep 21, 2016

Member

nit: extra space before as.

&HeadersInitType::ByteStringSequenceSequence(ref b) =>
HeadersInitType::ByteStringSequenceSequence(b.clone()),
&HeadersInitType::ByteStringMozMap(ref m) =>
{

This comment has been minimized.

@jdm

jdm Sep 21, 2016

Member
&HeadersInitType::ByteStringMozMap(ref m) =>
    HeadersInitType::ByteStringMozMap(m.clone())
@jeenalee jeenalee force-pushed the jeenalee:mozmap-headers branch from e0051b0 to e5eea2a Sep 21, 2016
@@ -14,7 +14,7 @@ use dom::bindings::codegen::Bindings::RequestBinding::RequestMethods;
use dom::bindings::codegen::Bindings::RequestBinding::RequestMode;
use dom::bindings::codegen::Bindings::RequestBinding::RequestRedirect;
use dom::bindings::codegen::Bindings::RequestBinding::RequestType;
use dom::bindings::codegen::UnionTypes::HeadersOrByteStringSequenceSequence;
use dom::bindings::codegen::UnionTypes::HeadersOrByteStringSequenceSequenceOrByteStringMozMap as HeadersInitType;

This comment has been minimized.

@KiChjang

KiChjang Sep 22, 2016

Member

Hmm... dom::bindings::codegen::Bindings::RequestBinding should re-export this union type as HeadersInit. Have you tried importing that instead?

This comment has been minimized.

@jeenalee

jeenalee Sep 22, 2016

Author Contributor

You're right! Fixing it right now. Thanks!

@jdm
Copy link
Member

jdm commented Sep 22, 2016

I'm going to hold off merging this until after Promises, since there's a lot of overlap between the test result changes.

@bors-servo
Copy link
Contributor

bors-servo commented Sep 22, 2016

The latest upstream changes (presumably #12830) made this pull request unmergeable. Please resolve the merge conflicts.

@jdm
Copy link
Member

jdm commented Sep 22, 2016

If you rebase, I will merge :)

@jdm
Copy link
Member

jdm commented Sep 22, 2016

Oh, and go ahead and squash the commits into one.

@jdm jdm added S-needs-squash and removed S-needs-rebase labels Sep 22, 2016
@jdm
Copy link
Member

jdm commented Sep 22, 2016

@bors-servo: delegate+

@bors-servo
Copy link
Contributor

bors-servo commented Sep 22, 2016

✌️ @jeenalee can now approve this pull request

@jeenalee jeenalee force-pushed the jeenalee:mozmap-headers branch from e5eea2a to 7577c3f Sep 22, 2016
Expected wpt results are updated as well.
@jeenalee jeenalee force-pushed the jeenalee:mozmap-headers branch from 7577c3f to 3b75d22 Sep 22, 2016
@jeenalee
Copy link
Contributor Author

jeenalee commented Sep 22, 2016

@bors-servo: r=jdm

@bors-servo
Copy link
Contributor

bors-servo commented Sep 22, 2016

📌 Commit 3b75d22 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Sep 22, 2016

Testing commit 3b75d22 with merge 68e7ff3...

bors-servo added a commit that referenced this pull request Sep 22, 2016
Support OpenEndedDictionary (Mozmap) in the Headers

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

This PR will support OpenEndedDictionary (based on MozMap implementation from #13332) in the Headers API.

---
<!-- 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 #11897  (github issue number if applicable).

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

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

bors-servo commented Sep 23, 2016

💔 Test failed - linux-rel

@highfive
Copy link

highfive commented Sep 23, 2016

  ▶ Unexpected subtest result in /_mozilla/mozilla/promise.html:
  │ FAIL [expected PASS] Native promise from async callback can be resolved
  │   → assert_true: expected true got false
  │ 
  └ @http://web-platform.test:8000/_mozilla/mozilla/promise.html:58:11
@Ms2ger
Copy link
Contributor

Ms2ger commented Sep 23, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Sep 23, 2016

Testing commit 3b75d22 with merge 0ecaa77...

bors-servo added a commit that referenced this pull request Sep 23, 2016
Support OpenEndedDictionary (Mozmap) in the Headers

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

This PR will support OpenEndedDictionary (based on MozMap implementation from #13332) in the Headers API.

---
<!-- 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 #11897  (github issue number if applicable).

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- 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/13356)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Sep 23, 2016
Support OpenEndedDictionary (Mozmap) in the Headers

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

This PR will support OpenEndedDictionary (based on MozMap implementation from #13332) in the Headers API.

---
<!-- 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 #11897  (github issue number if applicable).

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

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

bors-servo commented Sep 23, 2016

Testing commit 3b75d22 with merge 06f7a64...

@bors-servo bors-servo mentioned this pull request Sep 23, 2016
4 of 5 tasks complete
@bors-servo
Copy link
Contributor

bors-servo commented Sep 23, 2016

@bors-servo bors-servo merged commit 3b75d22 into servo:master Sep 23, 2016
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
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.

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