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

Refactor FormData code to match updated spec #9049

Merged
merged 1 commit into from Jan 1, 2016
Merged

Refactor FormData code to match updated spec #9049

merged 1 commit into from Jan 1, 2016

Conversation

@KiChjang
Copy link
Member

KiChjang commented Dec 22, 2015

Review on Reviewable
@KiChjang
Copy link
Member Author

KiChjang commented Dec 22, 2015

Haven't written tests for formdata.set() yet.
@KiChjang KiChjang force-pushed the KiChjang:form-data-refactor branch from 44d58f9 to e2d550e Dec 22, 2015
@KiChjang
Copy link
Member Author

KiChjang commented Dec 22, 2015

Tests for formdata.set() has been committed, I think this is ready to be reviewed.
@KiChjang KiChjang mentioned this pull request Dec 23, 2015
Closed
@KiChjang KiChjang force-pushed the KiChjang:form-data-refactor branch from fa752b4 to 51e7bbe Dec 24, 2015
@eefriedman eefriedman self-assigned this Dec 24, 2015
@eefriedman
Copy link
Contributor

eefriedman commented Dec 24, 2015

Review status: 0 of 14 files reviewed at latest revision, 3 unresolved discussions.

components/script/dom/formdata.rs, line 129 [r2] (raw file):
is_none() then unwrap() is an anti-pattern. Use match instead.

components/script/dom/webidls/FormData.webidl, line 22 [r2] (raw file):
Hmm... implementing iterable is going to be kind of awkward with the hashtable representation. Not something that needs to be fixed in this patch.

tests/wpt/metadata/XMLHttpRequest/formdata-set.htm.ini, line 5 [r2] (raw file):
Do you know why these tests are failing? They don't obviously depend on any unimplemented functions.

Comments from the review on Reviewable.io
@KiChjang
Copy link
Member Author

KiChjang commented Dec 24, 2015

Review status: 0 of 14 files reviewed at latest revision, 1 unresolved discussion.

tests/wpt/metadata/XMLHttpRequest/formdata-set.htm.ini, line 5 [r2] (raw file):
testFormDataSetEmptyBlob actually does depend on the constructor for File objects, which we don't support yet.

As for "Passing a String object to FormData.set should work", it depends on the implementation of XHR responseText and onload.

Comments from the review on Reviewable.io
@eefriedman
Copy link
Contributor

eefriedman commented Dec 29, 2015

r=me after squash.
@bors-servo
Copy link
Contributor

bors-servo commented Dec 29, 2015

☔ The latest upstream changes (presumably #8109) made this pull request unmergeable. Please resolve the merge conflicts.
@KiChjang KiChjang force-pushed the KiChjang:form-data-refactor branch from cfaf32a to b511ec6 Jan 1, 2016
@KiChjang
Copy link
Member Author

KiChjang commented Jan 1, 2016

r? @eefriedman
@KiChjang KiChjang force-pushed the KiChjang:form-data-refactor branch from b511ec6 to 7001583 Jan 1, 2016
@KiChjang
Refactor FormData code to match updated spec
Loading status checks…
7001583 
Use Atoms instead of Strings as keys
@jdm
Copy link
Member

jdm commented Jan 1, 2016

@bors-servo: r=eefriedman
@bors-servo
Copy link
Contributor

bors-servo commented Jan 1, 2016

📌 Commit 7001583 has been approved by eefriedman
@bors-servo
Copy link
Contributor

bors-servo commented Jan 1, 2016

⌛ Testing commit 7001583 with merge ade750d...
bors-servo added a commit that referenced this pull request Jan 1, 2016
@bors-servo
Auto merge of #9049 - KiChjang:form-data-refactor, r=eefriedman
ade750d 
Refactor FormData code to match updated spec

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9049)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 1, 2016

💔 Test failed - mac-rel-wpt
@jdm
Copy link
Member

jdm commented Jan 1, 2016

@bors-servo: retry
@bors-servo
Copy link
Contributor

bors-servo commented Jan 1, 2016

⌛ Testing commit 7001583 with merge 7a5522a...
bors-servo added a commit that referenced this pull request Jan 1, 2016
@bors-servo
Auto merge of #9049 - KiChjang:form-data-refactor, r=eefriedman
Loading status checks…
7a5522a 
Refactor FormData code to match updated spec

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9049)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 1, 2016

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt
@bors-servo bors-servo merged commit 7001583 into servo:master Jan 1, 2016
2 of 3 checks passed
2 of 3 checks passed
code-review/reviewable Review in progress: 0 of 14 files reviewed, 1 unresolved discussion
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@bors-servo bors-servo mentioned this pull request Jan 1, 2016
Merged
@KiChjang KiChjang deleted the KiChjang:form-data-refactor branch Mar 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@eefriedman eefriedman

Labels
S-awaiting-merge
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@KiChjang @eefriedman @bors-servo @jdm @highfive
You can’t perform that action at this time.