Closed Bug 1109751 Opened 8 years ago Closed 8 years ago

[Fetch] Implement Body.formData() method

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Version:
36 Branch
Platform:
x86_64
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla40
Tracking Flags:
Tracking Status
firefox39 --- fixed
firefox40 --- fixed

People

(Reporter: crimsteam, Assigned: nsm)

References

Details

Attachments

(2 files, 2 obsolete files)

Request and Response constructors should set mime type
3.86 KB, patch
baku
: review+
lizzard
: approval-mozilla-aurora+
Details | Diff | Splinter Review
Consume FormData support in Fetch API
32.01 KB, patch
baku
: review+
lizzard
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0
Build ID: 20141125180439

Steps to reproduce:

https://fetch.spec.whatwg.org/#dom-body-formdata

<script>

	var request1 = new Request("");

	alert(request1.formData) // undefined
	
</script>

Only this method is missing from Body interface.
https://fetch.spec.whatwg.org/#body
Thanks for filing. This is mainly blocked on having FormData on workers, and since forms are not the highest priority for shipping the first version (since the other extraction operators can be used with custom parsing to handle forms), this is not on the radar of any paid staff working on this. I'd be happy to review a patch!
Depends on: 739173
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reviewboard does not deal well with multiple bugs in a set of commits yet. The patch is at https://reviewboard.mozilla.org/r/3275
Attached patch FormData in Fetch API (obsolete) — — Splinter Review 
Folded:
Bug 1109751 - FormData parsing for Body.formData()
Attachment #8563726 - Flags: review?(amarchesini)
Assignee: nobody → nsm.nikhil
Status: NEW → ASSIGNED
Attachment #8563726 - Attachment is obsolete: true
Attachment #8563726 - Flags: review?(amarchesini)
Comment on attachment 8563726 [details] [diff] [review]
FormData in Fetch API

reverting bad bzexport.
Attachment #8563726 - Attachment is obsolete: false
Attachment #8563726 - Flags: review?(amarchesini)
Comment on attachment 8563730 [details] [diff] [review]
Request and Response constructors should set mime type

Review of attachment 8563730 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/fetch/Request.cpp
@@ +34,5 @@
>    , mOwner(aOwner)
>    , mRequest(aRequest)
>  {
> +  ErrorResult result;
> +  SetMimeType(result);

same here.

::: dom/fetch/Response.cpp
@@ +38,5 @@
>    , mOwner(aGlobal)
>    , mInternalResponse(aInternalResponse)
>  {
> +  ErrorResult result;
> +  SetMimeType(result);

Write some warning msg if the result.Failed().
The fact is that, if this operation fails somehow, we don't have any information.
Can we move this in the constructor and return error if this method fails? Or in FetchBody template?
Attachment #8563730 - Flags: review?(amarchesini) → review-
On further investigation, a Get() for "content-type" cannot fail, so I've made SetMimeType() not accept an error.
Attachment #8565499 - Flags: review?(amarchesini)
Attachment #8563726 - Attachment is obsolete: true
Attachment #8563726 - Flags: review?(amarchesini)
Attachment #8563726 - Attachment is obsolete: false
Attachment #8563726 - Flags: review?(amarchesini)
Attachment #8565499 - Flags: review?(amarchesini) → review+
Nikhil, note bug 1143857 comment 0.  Apparently this is causing problems on github in nightly.  Does this change the priority of things?
Flags: needinfo?(nsm.nikhil)
Andrea said he should be able to review my patches to add FormData support before 39 branches to Aurora. I can split up this bug so that we support sending FormData, which is easy, but not formData(). I'll put up patches this week. That way we can get this compat issue fixed, and wait for a review on the more complicated form parsing in formData() (afaik that is holding up the review) a little later.
Flags: needinfo?(nsm.nikhil)
We should probably just use bug 1143857 for the "sending formdata" bit....
Attachment #8563726 - Attachment is obsolete: true
Attachment #8563726 - Flags: review?(amarchesini)
Comment on attachment 8579031 [details] [diff] [review]
Consume FormData support in Fetch API

Review of attachment 8579031 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm. Sorry for the huge delay.

::: dom/fetch/Fetch.cpp
@@ +514,5 @@
>  }
> +
> +void
> +FillFormData(const nsString& aName, const nsString& aValue, void* aFormData)
> +{

MOZ_ASSERT(aFormData);

@@ +538,5 @@
> + * FIXME(nsm): Bug 1127552 - We should add telemetry to calls to formData() and
> + * friends to figure out if Fetch ends up copying big blobs to see if this is
> + * worth optimizing.
> + */
> +class MOZ_STACK_CLASS FormDataParser {

new line before {

@@ +562,5 @@
> +  // Reads over a boundary and sets start to the position after the end of the
> +  // boundary. Returns false if no boundary is found immediately.
> +  bool
> +  PushOverBoundary(const nsACString& boundaryString,
> +                   nsACString::const_iterator& start,

aStart and aBoundaryString

@@ +591,5 @@
> +  }
> +
> +  // Reads over a CRLF and positions start after it.
> +  bool
> +  PushOverLine(nsACString::const_iterator& start)

aStart

@@ +602,5 @@
> +    return false;
> +  }
> +
> +  bool
> +  FindCRLF(nsACString::const_iterator& start,

aStart

@@ +610,5 @@
> +    return FindInReadable(NS_LITERAL_CSTRING("\r\n"), start, end);
> +  }
> +
> +  bool
> +  ParseHeader(nsACString::const_iterator& start,

aStart

@@ +696,5 @@
> +  // part of the body. This will position the iterator at the beginning of the
> +  // boundary (after the CRLF).
> +  bool
> +  ParseBody(const nsACString& boundaryString,
> +            nsACString::const_iterator& start,

aBoundaryString and aStart

@@ +1518,5 @@
> +      // Take over ownership of the string.
> +      nsAutoCString data(reinterpret_cast<char*>(aResult), aResultLength);
> +      autoFree.Reset();
> +
> +      if (StringBeginsWith(mMimeType, NS_LITERAL_CSTRING("multipart/form-data"))) {

Why StringBeginsWith ? What about multipart/form-datafoobar ?
Attachment #8579031 - Flags: review?(amarchesini) → review+
(In reply to Andrea Marchesini (:baku) from comment #14)
> 
> @@ +1518,5 @@
> > +      // Take over ownership of the string.
> > +      nsAutoCString data(reinterpret_cast<char*>(aResult), aResultLength);
> > +      autoFree.Reset();
> > +
> > +      if (StringBeginsWith(mMimeType, NS_LITERAL_CSTRING("multipart/form-data"))) {
> 
> Why StringBeginsWith ? What about multipart/form-datafoobar ?

Added fix and test for this in an update to the mime type patch. Fixed other issues in original patch.
Flags: needinfo?(wkocher) → needinfo?(nsm.nikhil)
seems like asan is happy after i explicitly adopted the string in the formdata case.
Flags: needinfo?(nsm.nikhil)
https://hg.mozilla.org/mozilla-central/rev/e2daaf1eadf6
https://hg.mozilla.org/mozilla-central/rev/1bbcde14b9bb
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment on attachment 8579031 [details] [diff] [review]
Consume FormData support in Fetch API

Approval Request Comment
[Feature/regressing bug #]: dom-fetch-api
[User impact if declined]: We ship FormData support for sending data in Aurora. It would be good to uplift this so that extracting FormData is also supported
[Describe test coverage new/current, TreeHerder]: The patch adds tests in dom/tests/mochitest/fetch
[Risks and why]: Minimal. There aren't any compatibility issues since this is a new spec.
[String/UUID change made/needed]: None
Attachment #8579031 - Flags: approval-mozilla-aurora?
Comment on attachment 8565499 [details] [diff] [review]
Request and Response constructors should set mime type

Approval Request Comment
[Feature/regressing bug #]: please see comment 25. The same applies for this patch.
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:
Attachment #8565499 - Flags: approval-mozilla-aurora?
Nikhil, for the messages in your patches, where would they be exposed?  I'm not sure if they need localization or not. 

Axel, is this something that would trigger your localization tools/process?
Flags: needinfo?(nsm.nikhil)
Flags: needinfo?(l10n)
The BAD_FORMDATA message is on the '.message' property of a javascript error.
Flags: needinfo?(nsm.nikhil)
It seems that that message isn't localized. No idea if that's a bug, though.
Flags: needinfo?(l10n)
Comment on attachment 8565499 [details] [diff] [review]
Request and Response constructors should set mime type

Approving for uplift to aurora since it sounds like we need to support this feature and this includes some test coverage.
Attachment #8565499 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8579031 [details] [diff] [review]
Consume FormData support in Fetch API

Approving for uplift for aurora.
Attachment #8579031 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Nikhil Marathe from comment #23)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/1bbcde14b9bb

Although I see no reference to it in this bug or any attachments, Mercurial blames the above changeset for the pattern NS_NAMED_LITERAL_CSTRING(..., NS_LITERAL_CSTRING("...")); which appears twice.

The preferred syntax is simply NS_NAMED_LITERAL_CSTRING(..., "...");

I think this only compiles because I forgot to delete the copy constructor in bug 514173.

I will therefore file a bug to do that and also fix this usage at the same time.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.