Bug 176066 - Add support for FetchRequest.body
Summary: Add support for FetchRequest.body
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-08-29 14:10 PDT by youenn fablet
Modified: 2017-08-30 13:19 PDT (History)
8 users (show)

See Also:


Attachments
Patch (21.76 KB, patch)
2017-08-29 14:14 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (25.11 KB, patch)
2017-08-29 14:26 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Addendum (3.25 KB, patch)
2017-08-29 21:00 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Addendum (3.20 KB, patch)
2017-08-30 11:43 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2017-08-29 14:10:26 PDT
Add support for FetchRequest.body
Comment 1 youenn fablet 2017-08-29 14:14:30 PDT
Created attachment 319281 [details]
Patch
Comment 2 Build Bot 2017-08-29 14:16:07 PDT
Attachment 319281 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/fetch/FetchBody.h:99:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 youenn fablet 2017-08-29 14:26:54 PDT
Created attachment 319282 [details]
Patch
Comment 4 Build Bot 2017-08-29 14:29:19 PDT
Attachment 319282 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/fetch/FetchBody.h:99:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Alex Christensen 2017-08-29 16:56:24 PDT
Comment on attachment 319282 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319282&action=review

> LayoutTests/imported/w3c/web-platform-tests/fetch/api/request/request-disturbed-expected.txt:6
> +FAIL Input request used for creating new request became disturbed assert_equals: body should not change expected object "[object ReadableStream]" but got object "[object ReadableStream]"

I don't understand this failure.
Comment 6 youenn fablet 2017-08-29 17:10:27 PDT
Thanks for the review.

(In reply to Alex Christensen from comment #5)
> Comment on attachment 319282 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319282&action=review
> 
> > LayoutTests/imported/w3c/web-platform-tests/fetch/api/request/request-disturbed-expected.txt:6
> > +FAIL Input request used for creating new request became disturbed assert_equals: body should not change expected object "[object ReadableStream]" but got object "[object ReadableStream]"
> 
> I don't understand this failure.

When using a request A as init to another request B, the expected behavior is to pipe request A stream to request B stream.

Request A stream then gets disturbed but remains the same.

Currently we are not doing so, we are moving the stream to the other request.
I added a FIXME for it in the patch.
Comment 7 Sam Weinig 2017-08-29 17:39:12 PDT
Comment on attachment 319282 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319282&action=review

> Source/WebCore/bindings/js/ReadableStream.h:81
> +template<> struct JSConverter<IDLInterface<ReadableStream>> {
> +    static constexpr bool needsState = false;
> +    static constexpr bool needsGlobalObject = false;
> +
> +    static JSC::JSValue convert(ReadableStream* value)
> +    {
> +        if (!value)
> +            return JSC::jsNull();
> +        return value->readableStream();
> +    }
> +};

Can this just be a toJS() implementation?

inline JSC::JSValue toJS(JSC::ExecState*, JSDOMGlobalObject*, ReadableStream& impl) { return impl.readableStream(); }
inline JSC::JSValue toJS(JSC::ExecState* state, JSDOMGlobalObject* globalObject, ReadableStream* impl) { return impl ? toJS(state, globalObject, *impl) : JSC::jsNull(); }

The top one is often out-of-line.
Comment 8 WebKit Commit Bot 2017-08-29 17:39:29 PDT
Comment on attachment 319282 [details]
Patch

Clearing flags on attachment: 319282

Committed r221329: <http://trac.webkit.org/changeset/221329>
Comment 9 WebKit Commit Bot 2017-08-29 17:39:31 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2017-08-29 17:40:45 PDT
<rdar://problem/34148373>
Comment 11 youenn fablet 2017-08-29 18:10:05 PDT
(In reply to Sam Weinig from comment #7)
> Comment on attachment 319282 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319282&action=review
> 
> > Source/WebCore/bindings/js/ReadableStream.h:81
> > +template<> struct JSConverter<IDLInterface<ReadableStream>> {
> > +    static constexpr bool needsState = false;
> > +    static constexpr bool needsGlobalObject = false;
> > +
> > +    static JSC::JSValue convert(ReadableStream* value)
> > +    {
> > +        if (!value)
> > +            return JSC::jsNull();
> > +        return value->readableStream();
> > +    }
> > +};
> 
> Can this just be a toJS() implementation?

Right, I'll fix it as a follow-up
Comment 12 youenn fablet 2017-08-29 21:00:10 PDT
Reopening to attach new patch.
Comment 13 youenn fablet 2017-08-29 21:00:11 PDT
Created attachment 319331 [details]
Addendum
Comment 14 WebKit Commit Bot 2017-08-29 21:08:10 PDT
Comment on attachment 319331 [details]
Addendum

Rejecting attachment 319331 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 319331, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.webkit.org/results/4407892
Comment 15 youenn fablet 2017-08-30 11:43:10 PDT
Created attachment 319384 [details]
Addendum
Comment 16 WebKit Commit Bot 2017-08-30 13:19:21 PDT
Comment on attachment 319384 [details]
Addendum

Clearing flags on attachment: 319384

Committed r221395: <http://trac.webkit.org/changeset/221395>
Comment 17 WebKit Commit Bot 2017-08-30 13:19:22 PDT
All reviewed patches have been landed.  Closing bug.