Issue 2292763002: [Fetch API] Implement Request.formData and Response.formData.

e_hakkinen

eero.hakkinen@intel.com changed reviewers: + horo@chromium.org

4 years, 3 months ago (2016-08-29 22:08:03 UTC) #1

e_hakkinen

PTAL. This adds implementations for both multipart and urlencoded form data parsers. I can split ...

4 years, 3 months ago (2016-08-29 22:08:04 UTC) #2

horo

https://codereview.chromium.org/2292763002/diff/1/third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/response-content.js File third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/response-content.js (right): https://codereview.chromium.org/2292763002/diff/1/third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/response-content.js#newcode122 third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/response-content.js:122: formData.get('sample blob').size); Please check the content of the blob ...

4 years, 3 months ago (2016-08-30 05:12:56 UTC) #4

yhirano

https://codereview.chromium.org/2292763002/diff/1/third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp File third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp (right): https://codereview.chromium.org/2292763002/diff/1/third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp#newcode7 third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:7: #include "core/fetch/MultipartImageResourceParser.h" On 2016/08/30 05:12:55, horo wrote: > I ...

4 years, 3 months ago (2016-08-30 05:51:14 UTC) #5

horo

https://codereview.chromium.org/2292763002/diff/1/third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp File third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp (right): https://codereview.chromium.org/2292763002/diff/1/third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp#newcode7 third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:7: #include "core/fetch/MultipartImageResourceParser.h" On 2016/08/30 05:51:14, yhirano wrote: > On ...

4 years, 3 months ago (2016-08-30 06:24:36 UTC) #6

yhirano

On 2016/08/30 06:24:36, horo wrote: > https://codereview.chromium.org/2292763002/diff/1/third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp > File third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp (right): > > https://codereview.chromium.org/2292763002/diff/1/third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp#newcode7 > ...

4 years, 3 months ago (2016-08-30 06:50:47 UTC) #7

e_hakkinen

https://codereview.chromium.org/2292763002/diff/1/third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/response-content.js File third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/response-content.js (right): https://codereview.chromium.org/2292763002/diff/1/third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/response-content.js#newcode122 third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/response-content.js:122: formData.get('sample blob').size); On 2016/08/30 05:12:55, horo wrote: > Please ...

4 years, 3 months ago (2016-08-30 07:59:09 UTC) #8

yhirano

not LGTM. I don't think it's a good idea to use MultipartImageResourceParser as-is for this ...

4 years, 3 months ago (2016-08-31 11:25:38 UTC) #9

e_hakkinen

On 2016/08/31 11:25:38, yhirano wrote: > I don't think it's a good idea to use ...

4 years, 3 months ago (2016-08-31 13:41:21 UTC) #10

e_hakkinen

On 2016/08/31 13:41:21, e_hakkinen wrote: > On 2016/08/31 11:25:38, yhirano wrote: > > I don't ...

4 years, 3 months ago (2016-09-02 09:57:39 UTC) #11

yhirano

https://codereview.chromium.org/2292763002/diff/80001/third_party/WebKit/Source/modules/fetch/Body.cpp File third_party/WebKit/Source/modules/fetch/Body.cpp (right): https://codereview.chromium.org/2292763002/diff/80001/third_party/WebKit/Source/modules/fetch/Body.cpp#newcode189 third_party/WebKit/Source/modules/fetch/Body.cpp:189: ParsedContentType parsedTypeWithParameters(contentType()); The spec[1] uses body's "MIME type", why ...

4 years, 3 months ago (2016-09-07 06:00:16 UTC) #12

e_hakkinen

https://codereview.chromium.org/2292763002/diff/80001/third_party/WebKit/Source/modules/fetch/Body.cpp File third_party/WebKit/Source/modules/fetch/Body.cpp (right): https://codereview.chromium.org/2292763002/diff/80001/third_party/WebKit/Source/modules/fetch/Body.cpp#newcode189 third_party/WebKit/Source/modules/fetch/Body.cpp:189: ParsedContentType parsedTypeWithParameters(contentType()); On 2016/09/07 06:00:16, yhirano wrote: > The ...

4 years, 3 months ago (2016-09-08 00:06:29 UTC) #13

yhirano

Sorry for the late response. Though I left some code comments, I have questions about ...

4 years, 3 months ago (2016-09-12 02:20:50 UTC) #14

Sorry for the late response.

Though I left some code comments, I have questions about the basic design. If
you update it, you don't need to respond with the code comments.

1. Why is this parser streaming?
   This parser is hard to understand partially because it is a
   streaming parser (i.e., it dispatches data to the client as soon
   as it's available). As what we need is supporting formData()
   function which returns a promise resolved when ALL data is
   available, we don't need the feature. I would appreciate if
   you could simplify the code.
   Even if you want to write a streaming parser, buffering a small
   amount of data for delimiter, CRLF, "--", etc would be good for
   readability than managing adhoc seen* variables / conditions.
2. How to handle content-transfer-encoding in each part?
   Each part MAY contain a content-transfer-encoding header. It 
   seems this parser doesn't handle it, but as the browser process
   and net/ handles it, I think it should be handled in the parser.
   At least, the parser should reject the input when it sees the
   header.

https://codereview.chromium.org/2292763002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/Body.cpp (right):

https://codereview.chromium.org/2292763002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/Body.cpp:189: ParsedContentType
parsedTypeWithParameters(contentType());
On 2016/09/08 00:06:29, e_hakkinen wrote:
> On 2016/09/07 06:00:16, yhirano wrote:
> > The spec[1] uses body's "MIME type", why don't you use Body::mimeType?
> 
> Because the return value of Body::mimeType does not contain parameters like
> boundary if the response comes from content. So using Body::mimeType would
work
> for Responses created by Response constructor but not for Responses created by
> fetches. 

Then I think the existing Body::mimeType should be fixed.
Note that ResourceRequest::mimeType and 'content-type' header may diverge
because of the mime sniffer. I'm trying to disable it for Fetch API[1]. When
it's done we can use the 'content-type' header for Body::mimeType (possibly in a
separate CL?).

1: https://codereview.chromium.org/2323623002/

https://codereview.chromium.org/2292763002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp (right):

https://codereview.chromium.org/2292763002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:169: class
FetchDataLoaderAsFormData final : public FetchDataLoader, public
BytesConsumer::Client, private MultipartParser::Client {
On 2016/09/08 00:06:29, e_hakkinen wrote:
> On 2016/09/07 06:00:16, yhirano wrote:
> > Why do you use private inheritance?
> 
> Because public inheritance is not needed. I'll use public one if it is
> preferred.
> Done.

Thanks, I think public is preferred
(https://google.github.io/styleguide/cppguide.html#Inheritance).

https://codereview.chromium.org/2292763002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/MultipartParser.cpp (right):

https://codereview.chromium.org/2292763002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.cpp:19: }
 // namespace

https://codereview.chromium.org/2292763002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.cpp:71: if (headerBytes
!= m_seenHeaderBytes.data())
if (m_seenHeaderBytes.isEmpty()) is easier to understand.

https://codereview.chromium.org/2292763002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.cpp:78:
m_seenHeaderBytes.shrink(0);
clear()

https://codereview.chromium.org/2292763002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.cpp:103: m_state =
bytes[index] != '-' ? ParsingDelimiterSuffix : ParsingCloseDelimiterSuffix;
This is not correct. When parsing the first "dash-boundary transport-padding
CRLF" in multipart-body,  we need reject the input if we see '-' here.

https://codereview.chromium.org/2292763002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.cpp:195: if
(possibleDelimiterBytes > 0u) {
Is it OK to execute these statements unconditionally?

https://codereview.chromium.org/2292763002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.cpp:198: }
DCHECK_LE(*index, size);

https://codereview.chromium.org/2292763002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.cpp:208: *index +=
countNonDelimiterBytes(bytes + *index, size - *index);
DCHECK_LE(*index, size);

https://codereview.chromium.org/2292763002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.cpp:216: if (expected ==
'\r')
Hmm... I don't like this special case. The function looks like an general
parsing (e.g., taking a pattern to match) but it starts consuming LWSPs if it
sees a certain character.

https://codereview.chromium.org/2292763002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.cpp:224:
++m_seenDelimiterLength;
I don't understand - what you are seeing here is not the delimiter, but a
"delimiter suffix". Why are you updating this variable?

https://codereview.chromium.org/2292763002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/MultipartParser.h (right):

https://codereview.chromium.org/2292763002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.h:23: virtual void
partDataInMultipartReceived(const char* bytes, size_t) = 0;
It looks you include the delimiter to the "part data". Please write so here (I
don't understand why the delimiter should be included , but it is a separate
topic...).

https://codereview.chromium.org/2292763002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.h:23: virtual void
partDataInMultipartReceived(const char* bytes, size_t) = 0;
Can |size| be zero? I don't think it problematic but it looks you are making
efforts to avoid that.

e_hakkinen

On 2016/09/12 02:20:50, yhirano (slow) wrote: > Sorry for the late response. No problem. > ...

4 years, 3 months ago (2016-09-16 13:41:37 UTC) #15

On 2016/09/12 02:20:50, yhirano (slow) wrote:
> Sorry for the late response.

No problem.

> Though I left some code comments, I have questions about the basic design. If
> you update it, you don't need to respond with the code comments.
> 
> 1. Why is this parser streaming?
>    This parser is hard to understand partially because it is a
>    streaming parser (i.e., it dispatches data to the client as soon
>    as it's available). As what we need is supporting formData()
>    function which returns a promise resolved when ALL data is
>    available, we don't need the feature. I would appreciate if
>    you could simplify the code.
>    Even if you want to write a streaming parser, buffering a small
>    amount of data for delimiter, CRLF, "--", etc would be good for
>    readability than managing adhoc seen* variables / conditions.

Multipart parts can big data files. So IMHO it makes sense to store data right
away to blobs instead of first buffering all data in one huge char buffer and
only then in blobs.

I replaces seen* variables with a buffer container. Hopefully that helps
readability. I also added more comments.

> 2. How to handle content-transfer-encoding in each part?
>    Each part MAY contain a content-transfer-encoding header. It 
>    seems this parser doesn't handle it, but as the browser process
>    and net/ handles it, I think it should be handled in the parser.
>    At least, the parser should reject the input when it sees the
>    header.

Form parts should not be transfer encoded. So there should be no need to handle
encodings. But it is a good idea to reject the input if body octets are encoded.
I added a check for that.

https://codereview.chromium.org/2292763002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/MultipartParser.cpp (right):

https://codereview.chromium.org/2292763002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.cpp:19: }
On 2016/09/12 02:20:49, yhirano (slow) wrote:
>  // namespace

Done.

https://codereview.chromium.org/2292763002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.cpp:71: if (headerBytes
!= m_seenHeaderBytes.data())
On 2016/09/12 02:20:49, yhirano (slow) wrote:
> if (m_seenHeaderBytes.isEmpty()) is easier to understand.

Done.

https://codereview.chromium.org/2292763002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.cpp:78:
m_seenHeaderBytes.shrink(0);
On 2016/09/12 02:20:49, yhirano (slow) wrote:
> clear()

Done.

https://codereview.chromium.org/2292763002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.cpp:103: m_state =
bytes[index] != '-' ? ParsingDelimiterSuffix : ParsingCloseDelimiterSuffix;
On 2016/09/12 02:20:49, yhirano (slow) wrote:
> This is not correct.

No, it is correct.

> When parsing the first "dash-boundary transport-padding
> CRLF" in multipart-body,  we need reject the input if we see '-' here.

That's true. That is why next state after a preamble and a delimiter is
ParsingDelimiterSuffix (see the line 44) and not
ParsingDelimiterOrCloseDelimiterSuffix as we need to reject the input if we see
a close delimiter suffix.

So, the parsing state can be ParsingDelimiterOrCloseDelimiterSuffix only after
part octets and a delimiter (see the line 92) as that is the only place in the
input where both a delimiter suffix and a close delimiter suffix are allowed.

https://codereview.chromium.org/2292763002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.cpp:216: if (expected ==
'\r')
On 2016/09/12 02:20:49, yhirano (slow) wrote:
> Hmm... I don't like this special case. The function looks like an general
> parsing (e.g., taking a pattern to match) but it starts consuming LWSPs if it
> sees a certain character.

Removed.

https://codereview.chromium.org/2292763002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.cpp:224:
++m_seenDelimiterLength;
On 2016/09/12 02:20:49, yhirano (slow) wrote:
> I don't understand - what you are seeing here is not the delimiter, but a
> "delimiter suffix". Why are you updating this variable?

Sorry for reusing m_seenDelimiterLength for m_seenDelimiterSuffixLength.
Reimplemented.

https://codereview.chromium.org/2292763002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/MultipartParser.h (right):

https://codereview.chromium.org/2292763002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.h:23: virtual void
partDataInMultipartReceived(const char* bytes, size_t) = 0;
On 2016/09/12 02:20:49, yhirano (slow) wrote:
> It looks you include the delimiter to the "part data".

I do not include the delimiter to the "part data". That would not make sense.

However, it is possible that the "part data" contain partial delimiters such as
newlines ("\r\n"), newlines followed by dashes ("\r\n--") or longer partial
delimiters ("\r\n--boundar") but never a full delimiter ("\r\n--boundary").

https://codereview.chromium.org/2292763002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.h:23: virtual void
partDataInMultipartReceived(const char* bytes, size_t) = 0;
On 2016/09/12 02:20:49, yhirano (slow) wrote:
> Can |size| be zero? I don't think it problematic but it looks you are making
> efforts to avoid that.

No, |size| cannot be zero. To me, it sounds contradictory to claim that data has
been received then no data (zero bytes) is received.

yhirano

https://codereview.chromium.org/2292763002/diff/120001/third_party/WebKit/Source/modules/fetch/MultipartParser.cpp File third_party/WebKit/Source/modules/fetch/MultipartParser.cpp (right): https://codereview.chromium.org/2292763002/diff/120001/third_party/WebKit/Source/modules/fetch/MultipartParser.cpp#newcode103 third_party/WebKit/Source/modules/fetch/MultipartParser.cpp:103: m_state = bytes[index] != '-' ? ParsingDelimiterSuffix : ParsingCloseDelimiterSuffix; ...

4 years, 3 months ago (2016-09-20 09:49:53 UTC) #16

https://codereview.chromium.org/2292763002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/MultipartParser.cpp (right):

https://codereview.chromium.org/2292763002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.cpp:103: m_state =
bytes[index] != '-' ? ParsingDelimiterSuffix : ParsingCloseDelimiterSuffix;
On 2016/09/16 13:41:36, e_hakkinen wrote:
> On 2016/09/12 02:20:49, yhirano (slow) wrote:
> > This is not correct.
> 
> No, it is correct.
> 
> > When parsing the first "dash-boundary transport-padding
> > CRLF" in multipart-body,  we need reject the input if we see '-' here.
> 
> That's true. That is why next state after a preamble and a delimiter is
> ParsingDelimiterSuffix (see the line 44) and not
> ParsingDelimiterOrCloseDelimiterSuffix as we need to reject the input if we
see
> a close delimiter suffix.
> 
> So, the parsing state can be ParsingDelimiterOrCloseDelimiterSuffix only after
> part octets and a delimiter (see the line 92) as that is the only place in the
> input where both a delimiter suffix and a close delimiter suffix are allowed.

Thank you, I understand.

https://codereview.chromium.org/2292763002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/MultipartParser.h (right):

https://codereview.chromium.org/2292763002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.h:23: virtual void
partDataInMultipartReceived(const char* bytes, size_t) = 0;
On 2016/09/16 13:41:37, e_hakkinen wrote:
> On 2016/09/12 02:20:49, yhirano (slow) wrote:
> > Can |size| be zero? I don't think it problematic but it looks you are making
> > efforts to avoid that.
> 
> No, |size| cannot be zero. To me, it sounds contradictory to claim that data
has
> been received then no data (zero bytes) is received.

Then please write so. Unless you express such things explicitly any user cannot
rely on that.

https://codereview.chromium.org/2292763002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.h:23: virtual void
partDataInMultipartReceived(const char* bytes, size_t) = 0;
On 2016/09/16 13:41:37, e_hakkinen wrote:
> On 2016/09/12 02:20:49, yhirano (slow) wrote:
> > It looks you include the delimiter to the "part data".
> 
> I do not include the delimiter to the "part data". That would not make sense.
> 
> However, it is possible that the "part data" contain partial delimiters such
as
> newlines ("\r\n"), newlines followed by dashes ("\r\n--") or longer partial
> delimiters ("\r\n--boundar") but never a full delimiter ("\r\n--boundary").

Sorry, I misunderstood. Thanks for the explanation.

https://codereview.chromium.org/2292763002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/MultipartParser.cpp (right):

https://codereview.chromium.org/2292763002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.cpp:93: m_state =
Failed;
If bytes == bytesEnd this function returns true. I guess that's not you want.

https://codereview.chromium.org/2292763002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.cpp:94: } else if
(m_bufferedBytes.size() == kDelimiterSuffixSize) {
How about having isComplete method rather than comparing pattern's length
manually?

https://codereview.chromium.org/2292763002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.cpp:143: if
(m_bufferedBytes.size() == 2u && parseTransportPadding(&bytes, bytesEnd))
What do you think about the following pattern?

while (...) {
  if (m_bufferedBytes.size() == 2u)
    parseTransportPadding(&bytes, bytesEnd);
  if (bytes == bytesEnd)
    break;
  ...
}

That way you don't have to return a boolean from parseTransportPadding.

https://codereview.chromium.org/2292763002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.cpp:235:
m_bufferedBytes.clear();
Can you move this statement (and the similar ones) out of this function so that
the caller can calculate the region currently held by |*data| from
|*bytesPointer| and |m_bufferedBytes| uniformally? That way we can remove |data|
parameter.

https://codereview.chromium.org/2292763002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.cpp:258: // also in the
range [bytesBegin, bytesEnd[. There is no need to reset
]

https://codereview.chromium.org/2292763002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.cpp:318: if
(!encoding.isNull()) {
Why do you need this condition?

https://codereview.chromium.org/2292763002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/MultipartParser.h (right):

https://codereview.chromium.org/2292763002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.h:38: class Buffer {
This class is used in a two ways:

1. It is a matcher for a fixed pattern (I use the term "fixed" because all
patterns are fixed once a MultipartParser instance is created). 
2. It is a temporary buffer containing a pointer and a size.

They are different concepts and I think they should be separated. Can you rename
this class to something like Matcher? I think we can kill the second usage as
commented in parseDataAndDelimiter.

https://codereview.chromium.org/2292763002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.h:58: Buffer
closeDelimiterSuffixBuffer(size_t = 0u) const;
The parameter is never specified.

https://codereview.chromium.org/2292763002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.h:60: Buffer
delimiterSuffixBuffer(size_t = 0u) const;
The parameter is never specified.

https://codereview.chromium.org/2292763002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.h:62: bool
parseDataAndDelimiter(const char** bytesPointer, const char* bytesEnd, Buffer*
data);
These three functions are named in the same manner and it looks their return
values have the same meaning but they don't.

parseDataAndDelimiter returns true when it sees the complete delimiter. It
doesn't modify |m_state|. It clears |m_buferredBytes|. parseHeaderFields returns
true when it sees a complete header. It modifies |m_state| when failed. The
caller needs to clear |m_bufferedBytes|. parseTransportPadding returns false
when it sees an error. It doesn't modify |m_state|.

The inconsistency is very hard to understand. Can you make them consistent (more
preferable), or write more comments (less preferable)?

e_hakkinen

https://codereview.chromium.org/2292763002/diff/160001/third_party/WebKit/Source/modules/fetch/MultipartParser.cpp File third_party/WebKit/Source/modules/fetch/MultipartParser.cpp (right): https://codereview.chromium.org/2292763002/diff/160001/third_party/WebKit/Source/modules/fetch/MultipartParser.cpp#newcode93 third_party/WebKit/Source/modules/fetch/MultipartParser.cpp:93: m_state = Failed; On 2016/09/20 09:49:53, yhirano (slow) wrote: ...

4 years, 3 months ago (2016-09-20 21:59:29 UTC) #17

https://codereview.chromium.org/2292763002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/MultipartParser.cpp (right):

https://codereview.chromium.org/2292763002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.cpp:93: m_state =
Failed;
On 2016/09/20 09:49:53, yhirano (slow) wrote:
> If bytes == bytesEnd this function returns true. I guess that's not you want.

True. Fixed.

https://codereview.chromium.org/2292763002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.cpp:94: } else if
(m_bufferedBytes.size() == kDelimiterSuffixSize) {
On 2016/09/20 09:49:53, yhirano (slow) wrote:
> How about having isComplete method rather than comparing pattern's length
> manually?

Done.

https://codereview.chromium.org/2292763002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.cpp:143: if
(m_bufferedBytes.size() == 2u && parseTransportPadding(&bytes, bytesEnd))
On 2016/09/20 09:49:53, yhirano (slow) wrote:
> What do you think about the following pattern?

I am equally fine with that. Done.

https://codereview.chromium.org/2292763002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.cpp:235:
m_bufferedBytes.clear();
On 2016/09/20 09:49:53, yhirano (slow) wrote:
> Can you move this statement (and the similar ones) out of this function so
that
> the caller can calculate the region currently held by |*data| from
> |*bytesPointer| and |m_bufferedBytes| uniformally? That way we can remove
|data|
> parameter.

Done.

https://codereview.chromium.org/2292763002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.cpp:258: // also in the
range [bytesBegin, bytesEnd[. There is no need to reset
On 2016/09/20 09:49:53, yhirano (slow) wrote:
> ]

*bytesEnd does not belong to the range.

https://codereview.chromium.org/2292763002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.cpp:318: if
(!encoding.isNull()) {
On 2016/09/20 09:49:53, yhirano (slow) wrote:
> Why do you need this condition?

It is needed in order to handle the common case in which there are no
Content-Transfer-Encoding header fields.

https://codereview.chromium.org/2292763002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/MultipartParser.h (right):

https://codereview.chromium.org/2292763002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.h:38: class Buffer {
On 2016/09/20 09:49:53, yhirano (slow) wrote:
> This class is used in a two ways:
> 
> 1. It is a matcher for a fixed pattern (I use the term "fixed" because all
> patterns are fixed once a MultipartParser instance is created). 
> 2. It is a temporary buffer containing a pointer and a size.
> 
> They are different concepts and I think they should be separated. Can you
rename
> this class to something like Matcher? I think we can kill the second usage as
> commented in parseDataAndDelimiter.

Done.

https://codereview.chromium.org/2292763002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.h:58: Buffer
closeDelimiterSuffixBuffer(size_t = 0u) const;
On 2016/09/20 09:49:53, yhirano (slow) wrote:
> The parameter is never specified.

Done.

https://codereview.chromium.org/2292763002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.h:60: Buffer
delimiterSuffixBuffer(size_t = 0u) const;
On 2016/09/20 09:49:53, yhirano (slow) wrote:
> The parameter is never specified.

Done.

https://codereview.chromium.org/2292763002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.h:62: bool
parseDataAndDelimiter(const char** bytesPointer, const char* bytesEnd, Buffer*
data);
On 2016/09/20 09:49:53, yhirano (slow) wrote:
> These three functions are named in the same manner and it looks their return
> values have the same meaning but they don't.

They return true only when they have parsed what they are supposed to parse be
that data and a complete delimiter or a complete header or transport padding.

> parseDataAndDelimiter returns true when it sees the complete delimiter. It
> doesn't modify |m_state|. It clears |m_buferredBytes|. parseHeaderFields
returns
> true when it sees a complete header. It modifies |m_state| when failed. The
> caller needs to clear |m_bufferedBytes|. parseTransportPadding returns false
> when it sees an error. It doesn't modify |m_state|.
> 
> The inconsistency is very hard to understand. Can you make them consistent
(more
> preferable), or write more comments (less preferable)?

I preferred to consolidate parsing to these auxiliary functions.
I have now split these functions further, removed most bool returns and split
out some functionality. The latter two resulted in more code duplication. Let's
see if it makes code easier to understand.

yhirano

https://codereview.chromium.org/2292763002/diff/180001/third_party/WebKit/Source/modules/fetch/Body.h File third_party/WebKit/Source/modules/fetch/Body.h (right): https://codereview.chromium.org/2292763002/diff/180001/third_party/WebKit/Source/modules/fetch/Body.h#newcode64 third_party/WebKit/Source/modules/fetch/Body.h:64: virtual String mimeType() const = 0; I still think ...

4 years, 3 months ago (2016-09-21 09:02:58 UTC) #18

e_hakkinen

https://codereview.chromium.org/2292763002/diff/180001/third_party/WebKit/Source/modules/fetch/Body.h File third_party/WebKit/Source/modules/fetch/Body.h (right): https://codereview.chromium.org/2292763002/diff/180001/third_party/WebKit/Source/modules/fetch/Body.h#newcode64 third_party/WebKit/Source/modules/fetch/Body.h:64: virtual String mimeType() const = 0; On 2016/09/21 09:02:57, ...

4 years, 3 months ago (2016-09-22 22:27:17 UTC) #19

yhirano

https://codereview.chromium.org/2292763002/diff/200001/third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/body-mixin.js File third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/body-mixin.js (right): https://codereview.chromium.org/2292763002/diff/200001/third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/body-mixin.js#newcode112 third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/body-mixin.js:112: test.unreached_func('formData() must fail'), Please use unreached_fulfillment. https://codereview.chromium.org/2292763002/diff/200001/third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp File third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp ...

4 years, 2 months ago (2016-09-26 10:59:34 UTC) #20

horo

https://codereview.chromium.org/2292763002/diff/200001/third_party/WebKit/Source/modules/fetch/Body.cpp File third_party/WebKit/Source/modules/fetch/Body.cpp (right): https://codereview.chromium.org/2292763002/diff/200001/third_party/WebKit/Source/modules/fetch/Body.cpp#newcode189 third_party/WebKit/Source/modules/fetch/Body.cpp:189: ParsedContentType parsedTypeWithParameters(contentType()); nit: const https://codereview.chromium.org/2292763002/diff/200001/third_party/WebKit/Source/modules/fetch/Body.cpp#newcode190 third_party/WebKit/Source/modules/fetch/Body.cpp:190: String parsedType = ...

4 years, 2 months ago (2016-09-27 10:45:50 UTC) #21

e_hakkinen

https://codereview.chromium.org/2292763002/diff/200001/third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/body-mixin.js File third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/body-mixin.js (right): https://codereview.chromium.org/2292763002/diff/200001/third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/body-mixin.js#newcode112 third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/body-mixin.js:112: test.unreached_func('formData() must fail'), On 2016/09/26 10:59:33, yhirano wrote: > ...

4 years, 2 months ago (2016-09-28 15:15:05 UTC) #22

yhirano

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

4 years, 2 months ago (2016-09-29 07:08:07 UTC) #23

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2292763002/240001

4 years, 2 months ago (2016-09-29 07:08:25 UTC) #24

yhirano

https://codereview.chromium.org/2292763002/diff/200001/third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp File third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp (right): https://codereview.chromium.org/2292763002/diff/200001/third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp#newcode292 third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:292: } else { On 2016/09/28 15:15:04, e_hakkinen wrote: > ...

4 years, 2 months ago (2016-09-29 08:30:09 UTC) #25

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 2 months ago (2016-09-29 08:54:28 UTC) #26

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/301873)

4 years, 2 months ago (2016-09-29 08:54:29 UTC) #27

e_hakkinen

The CQ bit was checked by eero.hakkinen@intel.com to run a CQ dry run

4 years, 2 months ago (2016-09-29 21:22:31 UTC) #28

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2292763002/260001

4 years, 2 months ago (2016-09-29 21:22:59 UTC) #29

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 2 months ago (2016-09-29 23:43:53 UTC) #30

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 2 months ago (2016-09-29 23:43:54 UTC) #31

e_hakkinen

On 2016/09/29 08:30:09, yhirano wrote: > On 2016/09/28 15:15:04, e_hakkinen wrote: > > On 2016/09/26 ...

4 years, 2 months ago (2016-10-06 14:50:20 UTC) #32

On 2016/09/29 08:30:09, yhirano wrote:
> On 2016/09/28 15:15:04, e_hakkinen wrote:
> > On 2016/09/26 10:59:34, yhirano wrote:
> > > You're assuming that each part which doesn't have a filename parameter is
a
> > > string. Is the assumption valid?
> > 
> > I am not sure as https://fetch.spec.whatwg.org/#concept-body-package-data is
> > missing a detailed parsing specification.
> > 
> > While the parsing is not fully specified, it might make sense to align with
> > encoding. The https://fetch.spec.whatwg.org/#concept-bodyinit-extract
> algorithm
> > refers to
> >
>
https://html.spec.whatwg.org/multipage/forms.html#multipart/form-data-encodin...
> > which never creates string parts with a Content-Disposition filename
parameter
> > nor with a Content-Type header field.
> > 
> > So it might make sense to treat each part which have a Content-Disposition
> > filename parameter or a Content-Type header field as a blob and to treat
each
> > part which have neither as a string.
> > 
> > Changed accordingly.
> 
> If the spec is unclear, I think it's a good idea to ask a question at
> https://github.com/whatwg/fetch/issues . We should avoid interoperability
issues
> as much as possible.

I did not get a clear answer.

However, as the only current implementation of this part of the spec (Firefox)
and common server-side form-data procedures do the same thing:
 * They assume that each part which doesn't have a filename parameter is
   a string and ignore a possible Content-Type header field completely
   including its charset parameter.
 * They assume that each part which does have a filename parameter is a file
   or a blob (regardless of the presence of the Content-Type header field).
 * They also do not change decoding encoding based on a part which have a name
   parameter equal to "_charset_".

In the context of Fetch Standard Body mixin formData() method, this means that
bytes to characters conversions are always done using UTF-8 encoding as
required by https://encoding.spec.whatwg.org/ (string part bytes are always
treated as being UTF-8 bytes and blob part bytes are not decoded but stored
in Blobs in raw bytes).

For above reasons, I reverted the above change. There should not thus be any
interoperability issues any more.

e_hakkinen

The CQ bit was checked by eero.hakkinen@intel.com to run a CQ dry run

4 years, 2 months ago (2016-10-06 14:50:35 UTC) #33

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2292763002/280001

4 years, 2 months ago (2016-10-06 14:51:01 UTC) #34

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 2 months ago (2016-10-06 14:53:39 UTC) #35

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/81747) ios-simulator on ...

4 years, 2 months ago (2016-10-06 14:53:40 UTC) #36

e_hakkinen

The CQ bit was checked by eero.hakkinen@intel.com to run a CQ dry run

4 years, 2 months ago (2016-10-06 23:52:56 UTC) #37

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2292763002/300001

4 years, 2 months ago (2016-10-06 23:53:25 UTC) #38

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 2 months ago (2016-10-07 03:10:31 UTC) #39

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 2 months ago (2016-10-07 03:10:32 UTC) #40

yhirano

On 2016/10/06 14:50:20, e_hakkinen wrote: > On 2016/09/29 08:30:09, yhirano wrote: > > On 2016/09/28 ...

4 years, 2 months ago (2016-10-11 07:47:42 UTC) #41

On 2016/10/06 14:50:20, e_hakkinen wrote:
> On 2016/09/29 08:30:09, yhirano wrote:
> > On 2016/09/28 15:15:04, e_hakkinen wrote:
> > > On 2016/09/26 10:59:34, yhirano wrote:
> > > > You're assuming that each part which doesn't have a filename parameter
is
> a
> > > > string. Is the assumption valid?
> > > 
> > > I am not sure as https://fetch.spec.whatwg.org/#concept-body-package-data
is
> > > missing a detailed parsing specification.
> > > 
> > > While the parsing is not fully specified, it might make sense to align
with
> > > encoding. The https://fetch.spec.whatwg.org/#concept-bodyinit-extract
> > algorithm
> > > refers to
> > >
> >
>
https://html.spec.whatwg.org/multipage/forms.html#multipart/form-data-encodin...
> > > which never creates string parts with a Content-Disposition filename
> parameter
> > > nor with a Content-Type header field.
> > > 
> > > So it might make sense to treat each part which have a Content-Disposition
> > > filename parameter or a Content-Type header field as a blob and to treat
> each
> > > part which have neither as a string.
> > > 
> > > Changed accordingly.
> > 
> > If the spec is unclear, I think it's a good idea to ask a question at
> > https://github.com/whatwg/fetch/issues . We should avoid interoperability
> issues
> > as much as possible.
> 
> I did not get a clear answer.
> 
> However, as the only current implementation of this part of the spec (Firefox)
> and common server-side form-data procedures do the same thing:
>  * They assume that each part which doesn't have a filename parameter is
>    a string and ignore a possible Content-Type header field completely
>    including its charset parameter.
>  * They assume that each part which does have a filename parameter is a file
>    or a blob (regardless of the presence of the Content-Type header field).
>  * They also do not change decoding encoding based on a part which have a name
>    parameter equal to "_charset_".
> 
> In the context of Fetch Standard Body mixin formData() method, this means that
> bytes to characters conversions are always done using UTF-8 encoding as
> required by https://encoding.spec.whatwg.org/ (string part bytes are always
> treated as being UTF-8 bytes and blob part bytes are not decoded but stored
> in Blobs in raw bytes).
> 
> For above reasons, I reverted the above change. There should not thus be any
> interoperability issues any more.

Thank you very much for your effort (and sorry for the late response).
I saw your discussion at https://github.com/whatwg/fetch/issues/392. I think
it's reasonable to determine each part's type from the filename parameter. But I
couldn't find how we should interpret "utf-8 as encoding" from annevk@'s
response. How about the following?

 - Determine each part's type (string or file) based on the filename parameter.
 - Decode the bytes if part's type is string and charset is explicitly specified
and the encoding is supported.
 - Fail decoding if part's type is string and charset is not specified or the
encoding is specified but not supported.

e_hakkinen

On 2016/10/11 07:47:42, yhirano wrote: > Thank you very much for your effort (and sorry ...

4 years, 2 months ago (2016-10-11 10:21:18 UTC) #42

On 2016/10/11 07:47:42, yhirano wrote:
> Thank you very much for your effort (and sorry for the late response).
> I saw your discussion at https://github.com/whatwg/fetch/issues/392. I think
> it's reasonable to determine each part's type from the filename parameter.

I agree.

> But I couldn't find how we should interpret "utf-8 as encoding" from
> annevk@'s response.

Not really from annevk@'s response but from the Fetch Standard and from
existing implementations.

> How about the following?
> 
>  - Determine each part's type (string or file) based on the filename
parameter.

I agree.

>  - Decode the bytes if part's type is string and charset is explicitly
specified
> and the encoding is supported.

This will create an interoperability issue as Firefox does always UTF-8 decoding
for string parts regardless of the presence of a Content-Type header field or
the presence or the value of a charset parameter.
What Firefox does is the same as all the server side form processing procedures
I checked do.
So it seems that a Content-Type header field and its charset parameter are taken
into account only for file parts but never for string parts.

>  - Fail decoding if part's type is string and charset is not specified or the
> encoding is specified but not supported.

This goes against the Fetch Standard.
According to https://fetch.spec.whatwg.org/#concept-body-package-data FormData
step 1 and RFC 2388 section 3 and RFC 7578 section 5.1.2, UTF-8 encoded strings
MAY be represented directly encoded.
In addition according to
https://html.spec.whatwg.org/multipage/forms.html#multipart/form-data-encodin...
step 4, the parts of the generated multipart/form-data resource that correspond
to non-file fields must not have a `Content-Type` header specified.

Thus, if part's type is a string, it SHOULD NOT have a Content-Type header field
and if it does not have one, it cannot have a charset parameter thus its charset
is not specified but it MUST still be decoded using UTF-8 encoding and MUST NOT
fail (unless the UTF-8 decoding of bytes fails).

yhirano

Sorry for the late response. (snip) >> - Decode the bytes if part's type is ...

4 years, 2 months ago (2016-10-20 07:42:34 UTC) #44

yhirano

Sorry for a typo: > I would be very appreciated. It would be very appreciated.

4 years, 2 months ago (2016-10-20 07:44:19 UTC) #45

e_hakkinen

On 2016/10/20 07:42:34, yhirano wrote: > (snip) > >> - Decode the bytes if part's ...

4 years, 2 months ago (2016-10-20 09:07:58 UTC) #46

On 2016/10/20 07:42:34, yhirano wrote:
> (snip)
> >>  - Decode the bytes if part's type is string and charset is explicitly
specified
> >> and the encoding is supported.
> 
> > This will create an interoperability issue as Firefox does always UTF-8
decoding
> > for string parts regardless of the presence of a Content-Type header field
or
> > the presence or the value of a charset parameter.
> > What Firefox does is the same as all the server side form processing
procedures
> > I checked do.
> > So it seems that a Content-Type header field and its charset parameter are
taken
> > into account only for file parts but never for string parts.
> 
> I don't think we should do something just because Firefox does that.

It is not just Firefox but also all server side implementation which I checked.

So while I agree that we should not something just because Firefox does that, I
do think that we should be something else than everybody else does.

> If you could clarify the fetch spec, I would be very appreciated.

I can try to do that.

> >>  - Fail decoding if part's type is string and charset is not specified or
the
> >> encoding is specified but not supported.
> 
> > This goes against the Fetch Standard.
> > According to https://fetch.spec.whatwg.org/#concept-body-package-data
FormData
> > step 1 and RFC 2388 section 3 and RFC 7578 section 5.1.2, UTF-8 encoded
strings
> > MAY be represented directly encoded.
> I'm not following this. Can you explain a bit more?

Sorry for being unclear. I was trying to say that it is not possible to "[f]ail
decoding if part's type is string and charset is not specified" because string
parts typically do not have a Content-Type nor a charset specified (as they
should not have a Content-Type according to specs).

> Regarding RFC7578, it says 
> 
>  In the case where the form data is text, the charset parameter for
>  the "text/plain" Content-Type MAY be used to indicate the character
>  encoding used in that part.
> 
> > In addition according to
> >
>
https://html.spec.whatwg.org/multipage/forms.html#multipart/form-data-encodin...
> > step 4, the parts of the generated multipart/form-data resource that
> correspond
> > to non-file fields must not have a `Content-Type` header specified.
> That is the spec for encoding, and I think it's perfectly fine that the
browser
> can accept input that would never be generated by the browser itself.

I was just continuing reasoning why string parts without a Content-Type header
field must be accepted. I agree that it's perfectly fine that the browser can
accept input that would never be generated by the browser itself.

To summarize treatment of different kind of Content-Disposition and Content-Type
combinations, do we agree about the following?

The below multipart part should be decoded into a Blob. Such a part will result
from both the <input type="file"/> element and from formData.append("blob",
blob).
 --boundary
 Content-Disposition: form-data; name=blob; filename=blob
 Content-Type: application/octet-stream

 ... binary data ...
 --boundary--

Also the below multipart part should be decoded into a Blob although a browser
does not (to my knowledge) generate such a part.
 --boundary
 Content-Disposition: form-data; name=blob; filename=blob

 ... binary data ...
 --boundary--

The below multipart part should be decoded into a USVString. Such a part will
result from both the <input type="text"/> element and from
formData.append("string", string).
 --boundary
 Content-Disposition: form-data; name=string

 ... UTF-8 data ...
 --boundary--

The treatment of the below multipart part is unspecified.
 --boundary
 Content-Disposition: form-data; name=string
 Content-Type: text/x-ignored; charset=ignored

 ... UTF-8 data ...
 --boundary--

e_hakkinen

On 2016/10/20 09:07:58, e_hakkinen wrote: > To summarize treatment of different kind of Content-Disposition and ...

4 years, 2 months ago (2016-10-20 09:22:34 UTC) #47

yhirano

Chrome sends a multipart/form-data consisting of parts encoded in Shift-JIS when it submits a form ...

4 years, 1 month ago (2016-10-24 10:26:18 UTC) #48

e_hakkinen

Description was changed from ========== [Fetch API] Implement Request.formData and Response.formData. This contains form data ...

4 years ago (2016-11-25 14:24:28 UTC) #49

e_hakkinen

The CQ bit was checked by eero.hakkinen@intel.com to run a CQ dry run

4 years ago (2016-12-17 23:06:55 UTC) #50

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2292763002/320001

4 years ago (2016-12-17 23:07:00 UTC) #51

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years ago (2016-12-18 00:32:32 UTC) #52

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/356670)

4 years ago (2016-12-18 00:32:34 UTC) #53

e_hakkinen

The CQ bit was checked by eero.hakkinen@intel.com to run a CQ dry run

3 years, 11 months ago (2017-01-09 15:20:28 UTC) #58

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2292763002/420001

3 years, 11 months ago (2017-01-09 15:20:47 UTC) #59

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 11 months ago (2017-01-09 17:08:09 UTC) #60

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 11 months ago (2017-01-09 17:08:10 UTC) #61

e_hakkinen

Now the spec (https://fetch.spec.whatwg.org/#concept-body-package-data) has been updated so that it explicitly says that text data ...

3 years, 11 months ago (2017-01-09 17:42:58 UTC) #62

yhirano

Thank you very much for your efforts both on the spec and the implementation! You ...

3 years, 11 months ago (2017-01-11 08:54:03 UTC) #63

Thank you very much for your efforts both on the spec and the implementation!

You need to send an intent-to-ship to blink-dev@chromium.org before shipping a
feature. See https://www.chromium.org/blink/launching-features.

> I had to reimplement parts of the blink::parseMultipartHeadersFromBody
function
> (by adding blink::parseMultipartFormHeadersFromBody) because
> parseMultipartHeadersFromBody is not thread-safe (resource loading happens in
> the main thread whereas workers run on other threads)

I think ResourceResponse can be used on workers.

> and it expects header
> fields to be Latin-1 encoded (but oddly expects content type to be UTF-8
encoded) whereas fetch multipart/form-data header fields are UTF-8 encoded.

The HTTP spec says

   field-content = field-vchar [ 1*( SP / HTAB ) field-vchar ]
   field-name = token
   field-value = *( field-content / obs-fold )
   field-vchar = VCHAR / obs-text

and VCHAR is any visible USASCII character, so it it's Latin-1, right?

> Additionally parseMultipartHeadersFromBody does not parse
> Content-Transfer-Encoding header fields but that would have been easy to fix.

I'm now not sure if we should case about content-transfer-encoding given that we
effectively ignore content-type. Even if it's needed, it can be fixed easily as
you said.

So I think it's possible to have one function rather than two functions. What do
you think?

https://codereview.chromium.org/2292763002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp (right):

https://codereview.chromium.org/2292763002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:192: const CString
multipartBoundaryCString = m_multipartBoundary.utf8();
Is this variable needed? Isn't it enough to run
|multipartBoundaryVector.append(multipartBoundary.utf8().data(),
multipartBoundary.utf8().length())|?

https://codereview.chromium.org/2292763002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:233: // Fall
throught.
Can you write m_client->didFetchDataLoadFailed() here explicitly?

https://codereview.chromium.org/2292763002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:260:
m_multipartParser->cancel();
Shouldn't be this cancel() (i.e., this->cancel())? Ditto below.

https://codereview.chromium.org/2292763002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/MultipartParser.cpp (right):

https://codereview.chromium.org/2292763002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.cpp:123: if
(octetsNeedDecoding(headerFields))
Is this needed? With the current spec we end up ignoring the encoding in any
case, right?

yhirano

I made some mistakes, sorry. >> I had to reimplement parts of the blink::parseMultipartHeadersFromBody function ...

3 years, 11 months ago (2017-01-11 09:46:57 UTC) #64

e_hakkinen

https://codereview.chromium.org/2292763002/diff/420001/third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp File third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp (right): https://codereview.chromium.org/2292763002/diff/420001/third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp#newcode192 third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:192: const CString multipartBoundaryCString = m_multipartBoundary.utf8(); On 2017/01/11 08:54:03, yhirano ...

3 years, 7 months ago (2017-05-02 23:38:56 UTC) #66

https://codereview.chromium.org/2292763002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp (right):

https://codereview.chromium.org/2292763002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:192: const CString
multipartBoundaryCString = m_multipartBoundary.utf8();
On 2017/01/11 08:54:03, yhirano (away May 3 - May 5) wrote:
> Is this variable needed? Isn't it enough to run
> |multipartBoundaryVector.append(multipartBoundary.utf8().data(),
> multipartBoundary.utf8().length())|?

|Utf8()| is not a getter but a conversion function thus calling it twice would
result in the conversion to be done twice. So I think that it does make sense to
cache the conversion result.

https://codereview.chromium.org/2292763002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:233: // Fall
throught.
On 2017/01/11 08:54:03, yhirano (away May 3 - May 5) wrote:
> Can you write m_client->didFetchDataLoadFailed() here explicitly?

Done.

https://codereview.chromium.org/2292763002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:260:
m_multipartParser->cancel();
On 2017/01/11 08:54:03, yhirano (away May 3 - May 5) wrote:
> Shouldn't be this cancel() (i.e., this->cancel())? Ditto below.

No, it should not. This member function is called while |OnStateChange()| does a
two-phase read during which |consumer_->Cancel()| (called by |Cancel()|) must
not be called. Ditto below.

https://codereview.chromium.org/2292763002/diff/420001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/MultipartParser.cpp (right):

https://codereview.chromium.org/2292763002/diff/420001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/MultipartParser.cpp:123: if
(octetsNeedDecoding(headerFields))
On 2017/01/11 08:54:03, yhirano (away May 3 - May 5) wrote:
> Is this needed? With the current spec we end up ignoring the encoding in any
> case, right?

Yes, let's remove this.

yhirano

Sorry for the slow response. I've lost most of the context so some comments may ...

3 years, 7 months ago (2017-05-10 05:59:36 UTC) #67

horo

Please run the LayoutTets under - external/wpt/fetch/api/ - virtual/service-worker-navigation-preload-disabled/http/tests/serviceworker/webexposed/ - virtual/service-worker-navigation-preload-disabled/webexposed/ and update -expected.txt files. ...

3 years, 7 months ago (2017-05-11 04:44:16 UTC) #68

e_hakkinen

On 2017/01/11 08:54:03, yhirano wrote: > You need to send an intent-to-ship to mailto:blink-dev@chromium.org before ...

3 years, 7 months ago (2017-05-11 11:01:51 UTC) #69

On 2017/01/11 08:54:03, yhirano wrote:
> You need to send an intent-to-ship to mailto:blink-dev@chromium.org before
shipping a
> feature. See https://www.chromium.org/blink/launching-features.

Done and passed with lgtm3:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DxYtgt49OjA

https://codereview.chromium.org/2292763002/diff/450001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/Body.cpp (right):

https://codereview.chromium.org/2292763002/diff/450001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/Body.cpp:218:
resolver->Reject(V8ThrowException::CreateTypeError(script_state->GetIsolate(),
On 2017/05/11 04:44:16, horo wrote:
> Please update external/wpt/fetch/api/response/response-consume-empty.html and
> external/wpt/fetch/api/response/response-consume-empty.html of LayoutTest.

Done.

> According to the spec, formData() should throw a TypeError when the body is
> empty.

Yes but only if content type is multipart/form-data.
In that case MultipartParser::Finish called by
FetchDataLoaderAsFormData::OnStateChange will fail and therefore OnStateChange
will call BodyConsumerBase::DidFetchDataLoadFailed which rejects a promise with
a TypeError.

https://codereview.chromium.org/2292763002/diff/450001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp (right):

https://codereview.chromium.org/2292763002/diff/450001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp:292: if
(Controller())
On 2017/05/10 05:59:36, yhirano wrote:
> Can you tell me why we need this change?

Without this change, external/wpt/fetch/api/response/response-consume.html test
cases "from stream without correct multipart type to formData (error case)" and
"from stream without correct urlencoded type to formData (error case)" will
trigger null pointer exception here.

formData is different from, say, json because formData can trigger an error path
also before bytes are actually read (if content type is not valid) whereas json
can trigger an error path only after bytes are read and are being parsed.

https://codereview.chromium.org/2292763002/diff/450001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp (right):

https://codereview.chromium.org/2292763002/diff/450001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:279: const
ParsedContentType disposition(
On 2017/05/10 05:59:36, yhirano wrote:
> Need some comments why we use x-fake.

Done.

I also created https://crrev.com/2880543003/ to resolve the added TODOs.

https://codereview.chromium.org/2292763002/diff/450001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/fetch/FetchDataLoader.cpp:281: const String
disposition_type = disposition.MimeType().Substring(7u);
On 2017/05/10 05:59:36, yhirano wrote:
> Need some comments for the magic number.

Done.

e_hakkinen

Description was changed from ========== [Fetch API] Implement Request.formData and Response.formData. This contains form data ...

3 years, 7 months ago (2017-05-11 11:09:43 UTC) #70

horo

https://codereview.chromium.org/2292763002/diff/450001/third_party/WebKit/Source/modules/fetch/Body.cpp File third_party/WebKit/Source/modules/fetch/Body.cpp (right): https://codereview.chromium.org/2292763002/diff/450001/third_party/WebKit/Source/modules/fetch/Body.cpp#newcode218 third_party/WebKit/Source/modules/fetch/Body.cpp:218: resolver->Reject(V8ThrowException::CreateTypeError(script_state->GetIsolate(), On 2017/05/11 11:01:51, e_hakkinen wrote: > On 2017/05/11 ...

3 years, 7 months ago (2017-05-12 08:43:02 UTC) #71

yhirano

https://codereview.chromium.org/2292763002/diff/450001/third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp File third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp (right): https://codereview.chromium.org/2292763002/diff/450001/third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp#newcode292 third_party/WebKit/Source/modules/fetch/BodyStreamBuffer.cpp:292: if (Controller()) On 2017/05/11 11:01:51, e_hakkinen wrote: > On ...

3 years, 7 months ago (2017-05-12 11:54:12 UTC) #72

e_hakkinen

https://codereview.chromium.org/2292763002/diff/450001/third_party/WebKit/Source/modules/fetch/Body.cpp File third_party/WebKit/Source/modules/fetch/Body.cpp (right): https://codereview.chromium.org/2292763002/diff/450001/third_party/WebKit/Source/modules/fetch/Body.cpp#newcode218 third_party/WebKit/Source/modules/fetch/Body.cpp:218: resolver->Reject(V8ThrowException::CreateTypeError(script_state->GetIsolate(), On 2017/05/12 08:43:02, horo wrote: > What I ...

3 years, 7 months ago (2017-05-12 14:56:12 UTC) #73

yhirano

lgtm https://codereview.chromium.org/2292763002/diff/470001/third_party/WebKit/Source/modules/fetch/Body.cpp File third_party/WebKit/Source/modules/fetch/Body.cpp (right): https://codereview.chromium.org/2292763002/diff/470001/third_party/WebKit/Source/modules/fetch/Body.cpp#newcode217 third_party/WebKit/Source/modules/fetch/Body.cpp:217: BodyBuffer()->CloseAndLockAndDisturb(); On 2017/05/12 14:56:12, e_hakkinen wrote: > On ...

3 years, 7 months ago (2017-05-15 11:54:48 UTC) #74

horo

lgtm https://codereview.chromium.org/2292763002/diff/450001/third_party/WebKit/Source/modules/fetch/Body.cpp File third_party/WebKit/Source/modules/fetch/Body.cpp (right): https://codereview.chromium.org/2292763002/diff/450001/third_party/WebKit/Source/modules/fetch/Body.cpp#newcode218 third_party/WebKit/Source/modules/fetch/Body.cpp:218: resolver->Reject(V8ThrowException::CreateTypeError(script_state->GetIsolate(), On 2017/05/12 14:56:12, e_hakkinen wrote: > On ...

3 years, 7 months ago (2017-05-16 00:58:10 UTC) #75

yhirano

Please put the intent thread link to the issue description.

3 years, 7 months ago (2017-05-16 03:18:29 UTC) #76

e_hakkinen

Description was changed from ========== [Fetch API] Implement Request.formData and Response.formData. This contains form data ...

3 years, 7 months ago (2017-05-17 09:04:33 UTC) #77

e_hakkinen

On 2017/05/16 03:18:29, yhirano wrote: > Please put the intent thread link to the issue ...

3 years, 7 months ago (2017-05-17 09:46:59 UTC) #78

e_hakkinen

eero.hakkinen@intel.com changed reviewers: + jochen@chromium.org, kinuko@chromium.org, timvolodine@chromium.org

3 years, 7 months ago (2017-05-17 09:53:19 UTC) #79

e_hakkinen

+@jochen,@timvolodine: android_webview/tools/system_webview_shell/test/data/webexposed/global-interface-listing-expected.txt +@jochen: third_party/WebKit/LayoutTests/platform/win/virtual/stable/webexposed/global-interface-listing-expected.txt +@jochen: third_party/WebKit/LayoutTests/virtual/stable/http/tests/serviceworker/webexposed/global-interface-listing-service-worker-expected.txt +@jochen: third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-dedicated-worker-expected.txt +@jochen: third_party/WebKit/LayoutTests/virtual/stable/webexposed/global-interface-listing-shared-worker-expected.txt +@jochen: third_party/WebKit/Source/modules/BUILD.gn +@jochen,@kinuko: third_party/WebKit/Source/platform/network/HTTPParsers.{cpp,h}

3 years, 7 months ago (2017-05-17 09:53:21 UTC) #80

e_hakkinen

The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org, horo@chromium.org Link to ...

3 years, 7 months ago (2017-05-21 20:50:03 UTC) #84

commit-bot: I haz the power

CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2292763002/510001

3 years, 7 months ago (2017-05-21 20:50:14 UTC) #85

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-21 20:52:45 UTC) #86

commit-bot: I haz the power

Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/215649) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, ...

3 years, 7 months ago (2017-05-21 20:52:47 UTC) #87

e_hakkinen

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, jochen@chromium.org, yhirano@chromium.org, horo@chromium.org ...

3 years, 7 months ago (2017-05-21 21:00:29 UTC) #89

commit-bot: I haz the power

CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2292763002/530001

3 years, 7 months ago (2017-05-21 21:00:35 UTC) #90

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

3 years, 7 months ago (2017-05-21 21:53:57 UTC) #91

commit-bot: I haz the power

Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/220050)

3 years, 7 months ago (2017-05-21 21:54:02 UTC) #92

e_hakkinen

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, jochen@chromium.org, yhirano@chromium.org, horo@chromium.org ...

3 years, 7 months ago (2017-05-22 09:32:23 UTC) #94

commit-bot: I haz the power

CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2292763002/550001

3 years, 7 months ago (2017-05-22 09:32:44 UTC) #95

timvolodine

On 2017/05/17 09:53:21, e_hakkinen wrote: > +@jochen,@timvolodine: > android_webview/tools/system_webview_shell/test/data/webexposed/global-interface-listing-expected.txt > +@jochen: > third_party/WebKit/LayoutTests/platform/win/virtual/stable/webexposed/global-interface-listing-expected.txt > +@jochen: ...

3 years, 7 months ago (2017-05-22 09:35:40 UTC) #96

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 550001, "attempt_start_ts": 1495445542110410, "parent_rev": "6b15b25c276ea7a06d3e0964b1cbbbc46f29823b", "commit_rev": "6bd271fe88b9917fef4206aecc60b39b4f58652e"}

3 years, 7 months ago (2017-05-22 11:36:11 UTC) #97

commit-bot: I haz the power

Description was changed from ========== [Fetch API] Implement Request.formData and Response.formData. This contains form data ...

3 years, 7 months ago (2017-05-22 11:36:33 UTC) #98

commit-bot: I haz the power

3 years, 7 months ago (2017-05-22 11:36:35 UTC) #99

Message was sent while issue was closed.

Committed patchset #24 (id:550001) as
https://chromium.googlesource.com/chromium/src/+/6bd271fe88b9917fef4206aecc60...

Issue 2292763002: [Fetch API] Implement Request.formData and Response.formData. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Assertions, DCHECKs, includes #

Patch Set 3 : Multipart parser and unit tests #

Patch Set 4 #

Patch Set 5 : Handle partial delimiter prefixes correctly and really test them #

Patch Set 6 : Review based fixes #

Patch Set 7 : Rebase #

Patch Set 8 : Implementation clean-up #

Patch Set 9 : Rebase #

Patch Set 10 : Parse functions etc #

Patch Set 11 #

Patch Set 12 : #

Patch Set 13 : Rebase #

Patch Set 14 : body for invalid MIME type, */global-interface-listing{,-{dedicated,service,shared}-worker}-expecte… #

Patch Set 15 : Revert Content-Type presence based parsing decisions #

Patch Set 16 : Rebase #

Patch Set 17 : #

Patch Set 18 : #

Patch Set 19 : Rebase #

Patch Set 20 : Rebase, LayoutTests updates #

Patch Set 21 : LayoutTests, FetchDataLoader #

Patch Set 22 : #

Patch Set 23 : Rebase #

Patch Set 24 : More global-interface-listing*-expected.txt, urlencoded-parser-expected.txt #

Messages