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

Use BodyInit instead of SendParam, closes #9433 #9497

Closed
wants to merge 1 commit into from
Closed

Use BodyInit instead of SendParam, closes #9433 #9497

wants to merge 1 commit into from

Conversation

@fhahn
Copy link
Contributor

fhahn commented Feb 1, 2016

This PR fixes #9433 uses BodyInit instead of SendParam as suggested.

Review on Reviewable
@fhahn
Use BodyInit instead of SendParam, closes #9433
Loading status checks…
af95c17
@highfive
Copy link

highfive commented Feb 1, 2016

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @mbrubeck (or someone else) soon.
@highfive
Copy link

highfive commented Feb 1, 2016

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@KiChjang
Copy link
Member

KiChjang commented Feb 1, 2016

@fhahn Please claim the issue first before working on it. Duplicated efforts on the same issue would be a really nasty problem to have and I want to avoid it as much as possible.
@fhahn
Copy link
Contributor Author

fhahn commented Feb 1, 2016

Sure, sorry about that.
On Feb 2, 2016 00:35, "Keith Yeung" notifications@github.com wrote:

@fhahn https://github.com/fhahn Please claim the issue first before
working on it. Duplicated efforts on the same issue would be a really nasty
problem to have and I want to avoid it as much as possible.

—
Reply to this email directly or view it on GitHub
#9497 (comment).
@KiChjang
Copy link
Member

KiChjang commented Feb 1, 2016

@bors-servo r+

Code looks good to me, thanks for working on this!
@bors-servo
Copy link
Contributor

bors-servo commented Feb 1, 2016

📌 Commit af95c17 has been approved by KiChjang
@KiChjang KiChjang self-assigned this Feb 1, 2016
@KiChjang
Copy link
Member

KiChjang commented Feb 1, 2016

@bors-servo r-

Sorry, spotted something.
components/script/dom/xmlhttprequest.rs
use dom::bindings::codegen::Bindings::XMLHttpRequestBinding::XMLHttpRequestMethods;
use dom::bindings::codegen::Bindings::XMLHttpRequestBinding::XMLHttpRequestResponseType;
use dom::bindings::codegen::Bindings::XMLHttpRequestBinding::XMLHttpRequestResponseType::{Json, Text, _empty};
use dom::bindings::codegen::UnionTypes::BlobOrStringOrURLSearchParams;
use dom::bindings::codegen::UnionTypes::BlobOrStringOrURLSearchParams::{eBlob, eString, eURLSearchParams};

This comment has been minimized.

Sign in to view
@KiChjang

KiChjang Feb 1, 2016

Member

Is it possible to change this into dom::bindings::codegen::Bindings::XMLHttpRequestBinding::BodyInit::{eBlob, ...} instead?

This comment has been minimized.

Sign in to view
@nox

nox Feb 1, 2016

Member

I think it would be even better if we don't import them at all and we just qualify them with BodyInit, the name is shorter than before and that was kind of the point.

This comment has been minimized.

Sign in to view
@fhahn

fhahn Feb 2, 2016

Author Contributor

@KiChjang unfortunately it does not seem possible. BodyInit is only a type alias to BlobOrStringOrURLSearchParams, so this yields import errors.

This comment has been minimized.

Sign in to view
@KiChjang

KiChjang Feb 2, 2016

Member

Yeah, in which case, that match expression in the extract method down there would need BodyInit:: references.

This comment has been minimized.

Sign in to view
@KiChjang

KiChjang Feb 2, 2016

Member

Hmm... this would be annoying, we might actually need to fix our codegen to support this kind of syntax.

This comment has been minimized.

Sign in to view
@KiChjang

KiChjang Feb 2, 2016

Member

Filed rust-lang/rust#31355 for this.

This comment has been minimized.

Sign in to view
@fhahn

fhahn Feb 2, 2016

Author Contributor

@KiChjang yeah, using BodyInit:: does not work, even if BlobOrStringOrURLSearchParams is imported.

This comment has been minimized.

Sign in to view
@KiChjang

KiChjang Feb 2, 2016

Member

Filed #9511 instead, and is now blocked by it.
@bors-servo
Copy link
Contributor

bors-servo commented Feb 7, 2016

☔ The latest upstream changes (presumably #9543) made this pull request unmergeable. Please resolve the merge conflicts.
@KiChjang
Copy link
Member

KiChjang commented Feb 27, 2016

Is blocked on #9691
@jdm
Copy link
Member

jdm commented Mar 23, 2016

Now blocked on #10152. Almost there!
@jdm
Copy link
Member

jdm commented Mar 24, 2016

No longer blocked! @fhahn are you able to continue working on this now?
@KiChjang
Copy link
Member

KiChjang commented Apr 7, 2016

Closing due to inactivity.
@KiChjang KiChjang closed this Apr 7, 2016
@KiChjang KiChjang mentioned this pull request Apr 7, 2016
Closed
@fhahn fhahn deleted the fhahn:issue-9433-use-bodyinit branch May 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@KiChjang KiChjang

Labels
S-needs-code-changes S-needs-rebase
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Replace SendParam with BodyInit
6 participants
@fhahn @highfive @KiChjang @bors-servo @jdm @nox
You can’t perform that action at this time.