add file support to form data by psdh · Pull Request #9198 · servo/servo

psdh · 2016-01-08T12:04:51Z

nox · 2016-01-08T13:46:49Z

Thanks for your contribution!

-S-awaiting-review +S-needs-code-changes

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

components/script/dom/htmlformelement.rs, line 212 [r1] (raw file):
Nit: missing space before brace.

components/script/dom/htmlformelement.rs, line 213 [r1] (raw file):
Nit: remove braces.

components/script/dom/htmlformelement.rs, line 214 [r1] (raw file):
What do you mean that it should be found in UrlEncoded?

And nit: remove braces.

components/script/dom/htmlformelement.rs, line 334 [r1] (raw file):
String implements From<DOMString>, so you don't need to allocate a new string here. And why isn't the StringData constructor holding onto a DOMString?

components/script/dom/htmlformelement.rs, line 450 [r1] (raw file):
This should probably be DOMString.

components/script/dom/htmlformelement.rs, line 452 [r1] (raw file):
There is a type BlobOrString in UnionTypes, any reason you didn't use that?

components/script/dom/htmlinputelement.rs, line 475 [r1] (raw file):
No need to allocate a new string here.

components/script/dom/htmlselectelement.rs, line 96 [r1] (raw file):
No need to allocate a new string here.

Comments from the review on Reviewable.io

Manishearth · 2016-01-08T15:58:43Z

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

components/script/dom/htmlformelement.rs, line 214 [r1] (raw file):
The spec says to put the filename here, right?

Comments from the review on Reviewable.io

psdh · 2016-01-08T17:14:12Z

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

components/script/dom/htmlformelement.rs, line 214 [r1] (raw file):
as far URLEncoded is concerned, all data.value should be of type StringOrBlob::StringData. None of them should correspond to the second (StringOrBlob::BlobData) pattern match. Haven't touched the spec in this PR.

components/script/dom/htmlformelement.rs, line 334 [r1] (raw file):
correcting this

components/script/dom/htmlformelement.rs, line 452 [r1] (raw file):
removing this enum.

Comments from the review on Reviewable.io

Manishearth · 2016-01-08T20:48:52Z

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

components/script/dom/htmlformelement.rs, line 214 [r1] (raw file):
https://html.spec.whatwg.org/multipage/forms.html#application/x-www-form-urlencoded-encoding-algorithm step 4.2

We'll be using an encoding-agnostic way to generate the Vec of BlobOrStrings, and then running this encoding specific bit of code over it.

Comments from the review on Reviewable.io

psdh · 2016-01-19T16:59:03Z

Review status: 0 of 5 files reviewed at latest revision, 4 unresolved discussions.

components/script/dom/formdata.rs, line 165 [r3] (raw file):
the spec asks to change value of the entry and thus I have used mut form_data. I was wondering if we could just change the value in the return value and do away with the mutable reference?

Comments from the review on Reviewable.io

bors-servo · 2016-02-07T02:25:26Z

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

bors-servo · 2016-02-15T04:47:10Z

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

Ms2ger · 2016-02-26T08:56:34Z

@psdh It seems like the build fails, as well as ./mach test-tidy. Would you mind looking what's up?

bors-servo · 2016-02-27T00:02:13Z

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

nox · 2016-02-29T13:12:13Z

This needs a rebase and can't be built.

psdh · 2016-02-29T20:22:49Z

tidy fails due to #9806

KiChjang · 2016-03-01T20:29:50Z

components/script/dom/htmlformelement.rs

+                generate_multipart_data(&mut form_data, boundary)
            }
-            _ => "".to_owned() // TODO: Add serializers for the other encoding types
+            FormEncType::TextPlainEncoded => "".to_owned() // TODO: Add serializers for the other encoding types


This TODO comment can be removed, I believe.

KiChjang · 2016-03-01T20:42:46Z

components/script/dom/htmlformelement.rs

+            }
+            FormEncType::FormDataEncoded => {
+                let boundary = generate_boundary();
+                let mime = "multipart/formdata; boundary=".to_owned() + &boundary;


Aside from the typo (it's "multipart/form-data"), I hear from @seanmonstar that this is bad practice to use strings instead of actual Mime types that hyper provides.

What I mean is that we can use these enums, instead of hardcoding a string.

Also look at this macro.

KiChjang · 2016-03-02T21:52:06Z

-S-awaiting-review +S-needs-code-changes

Reviewed 1 of 3 files at r3, 1 of 6 files at r4, 2 of 5 files at r5, 3 of 4 files at r6.
Review status: 7 of 8 files reviewed at latest revision, 19 unresolved discussions, some commit checks failed.

components/script/dom/formdata.rs, line 165 [r3] (raw file):
I'd rather not, this sounds like it involves creating another Vec, which isn't very memory-efficient.

components/script/dom/formdata.rs, line 152 [r6] (raw file):
Should this (and generate_boundary) be located here at all? It looks like there aren't any code in formdata.rs that's calling these two functions.

components/script/dom/formdata.rs, line 156 [r6] (raw file):
From my spec reading, yes, it should take an Option.

components/script/dom/formdata.rs, line 171 [r6] (raw file):
Use hyper::header::ContentDisposition and hyper::header::DispositionParam here.

components/script/dom/formdata.rs, line 184 [r6] (raw file):
Use hyper:header::ContentType here.

components/script/dom/htmlformelement.rs, line 291 [r6] (raw file):
Perhaps it makes more sense to create a value_string() fn for FormDatum?

components/script/dom/htmlformelement.rs, line 555 [r6] (raw file):
Why the change from Blob to File?

components/script/dom/htmlformelement.rs, line 561 [r6] (raw file):
This really should be atomized I think, perhaps add a TODO comment about this?

Comments from the review on Reviewable.io

KiChjang · 2016-03-02T21:53:05Z

With the usage of Hyper, I don't think we need to be waiting on the tidy update in order to land this.

bors-servo · 2016-03-17T19:58:37Z

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

jdm · 2016-03-17T20:10:56Z

@psdh Are you still planning to finish this work?

psdh · 2016-03-18T01:34:05Z

@jdm Yes. I have exams now but will have time in a couple of days.

jdm · 2016-03-18T02:58:09Z

Great!

Manishearth · 2016-03-27T21:27:52Z

Any updates?

jdm · 2016-04-13T19:52:55Z

Closing due to inactivity. Feel free to reopen it if you start working on it again!

psdh · 2016-04-14T04:53:03Z

Oh, I think I completely missed Manish's comment. Anyway, I don't think I would be able to pick this in at least a month's time (lots of course projects this time around). Sorry for the inconvenience.

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jan 8, 2016

nox self-assigned this Jan 8, 2016

nox added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jan 8, 2016

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Jan 8, 2016

psdh force-pushed the addfileTODO branch from d964efb to 1a78131 Compare January 19, 2016 16:50

highfive added the S-needs-rebase There are merge conflict errors. label Feb 7, 2016

psdh force-pushed the addfileTODO branch from 1a78131 to 1c87851 Compare February 11, 2016 13:04

highfive added the S-needs-rebase There are merge conflict errors. label Feb 15, 2016

psdh force-pushed the addfileTODO branch from 1c87851 to df959ee Compare February 22, 2016 13:33

KiChjang removed the S-needs-rebase There are merge conflict errors. label Feb 26, 2016

highfive added the S-needs-rebase There are merge conflict errors. label Feb 27, 2016

nox added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Feb 29, 2016

psdh force-pushed the addfileTODO branch from df959ee to ea86119 Compare February 29, 2016 20:11

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Feb 29, 2016

psdh force-pushed the addfileTODO branch from ea86119 to 0a76d0c Compare February 29, 2016 20:12

generate multipart for forms

b90a603

psdh force-pushed the addfileTODO branch from 2de66ee to b90a603 Compare March 1, 2016 10:56

nox added S-blocked-on-external Something, somewhere else, needs to happen before this PR can be merged. and removed S-needs-rebase There are merge conflict errors. labels Mar 1, 2016

KiChjang reviewed Mar 1, 2016
View reviewed changes

KiChjang assigned KiChjang and unassigned nox Mar 1, 2016

KiChjang reviewed Mar 1, 2016
View reviewed changes

KiChjang added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Mar 1, 2016

KiChjang mentioned this pull request Mar 1, 2016

form + HTTP post doesn't work (or at least doesn't redirect) #9829

Closed

use mime

50fdf33

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Mar 2, 2016

KiChjang added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Mar 2, 2016

KiChjang removed the S-blocked-on-external Something, somewhere else, needs to happen before this PR can be merged. label Mar 2, 2016

highfive added the S-needs-rebase There are merge conflict errors. label Mar 17, 2016

jdm mentioned this pull request Mar 23, 2016

Add support for multipart/form-data #7553

Closed

jdm closed this Apr 13, 2016

Uh oh!

Comments

Conversation

psdh commented Jan 8, 2016

Uh oh!

nox commented Jan 8, 2016

Uh oh!

Manishearth commented Jan 8, 2016

Uh oh!

psdh commented Jan 8, 2016

Uh oh!

Manishearth commented Jan 8, 2016

Uh oh!

psdh commented Jan 19, 2016

Uh oh!

bors-servo commented Feb 7, 2016

Uh oh!

bors-servo commented Feb 15, 2016

Uh oh!

Ms2ger commented Feb 26, 2016

Uh oh!

bors-servo commented Feb 27, 2016

Uh oh!

nox commented Feb 29, 2016

Uh oh!

psdh commented Feb 29, 2016

Uh oh!

KiChjang Mar 1, 2016

Choose a reason for hiding this comment

Uh oh!

KiChjang Mar 1, 2016

Choose a reason for hiding this comment

Uh oh!

KiChjang Mar 1, 2016

Choose a reason for hiding this comment

Uh oh!

KiChjang Mar 1, 2016

Choose a reason for hiding this comment

Uh oh!

KiChjang commented Mar 2, 2016

Uh oh!

KiChjang commented Mar 2, 2016

Uh oh!

bors-servo commented Mar 17, 2016

Uh oh!

jdm commented Mar 17, 2016

Uh oh!

psdh commented Mar 18, 2016

Uh oh!

jdm commented Mar 18, 2016

Uh oh!

Manishearth commented Mar 27, 2016

Uh oh!

jdm commented Apr 13, 2016

Uh oh!

psdh commented Apr 14, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants