Bug 175866 - Add support for ReadableStream storage in FetchBody
Summary: Add support for ReadableStream storage in FetchBody
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-22 17:08 PDT by youenn fablet
Modified: 2017-08-25 13:10 PDT (History)
7 users (show)

See Also:


Attachments
Patch (19.73 KB, patch)
2017-08-22 17:13 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (19.81 KB, patch)
2017-08-22 17:40 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews115 for mac-elcapitan (2.06 MB, application/zip)
2017-08-22 19:17 PDT, Build Bot
no flags Details
Patch for landing (19.76 KB, patch)
2017-08-24 08:15 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (19.75 KB, patch)
2017-08-24 08:22 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-22 17:08:21 PDT
This will allow better support of request/cache API stream manipulation.
Comment 1 youenn fablet 2017-08-22 17:13:50 PDT
Created attachment 318829 [details]
Patch
Comment 2 Build Bot 2017-08-22 17:16:17 PDT
Attachment 318829 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/ReadableStream.h:42:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 youenn fablet 2017-08-22 17:40:52 PDT
Created attachment 318834 [details]
Patch
Comment 4 Build Bot 2017-08-22 17:43:35 PDT
Attachment 318834 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/ReadableStream.h:42:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Build Bot 2017-08-22 19:17:56 PDT
Comment on attachment 318834 [details]
Patch

Attachment 318834 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4365417

New failing tests:
media/track/track-element-dom-change-crash.html
Comment 6 Build Bot 2017-08-22 19:17:58 PDT
Created attachment 318846 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 7 youenn fablet 2017-08-22 20:41:21 PDT
Failure is unrelated to this patch.
I went with the approach of having IDLInterface<ReadableStream>, which works and has the benefit to keep the union conversion semantics the same.
This also automatically creates a Dom guarded object which is probably always wanted since processing ReadableStream can only be done asynchronously.

The alternative might be to introduce something like IDLReadableStream, but we might need to special case the binding generator for it and we would have to update the checks for the union conversion so that IDLReadableStream is part of the IDLInterface conversion.
Comment 8 Sam Weinig 2017-08-23 15:38:16 PDT
Comment on attachment 318834 [details]
Patch

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

> Source/WebCore/Modules/fetch/FetchBody.cpp:67
> +    }, [&](RefPtr<ReadableStream>&) mutable {
> +        FetchBody body;
> +        body.m_isReadableStream = true;
> +        return body;

Now that ReadableStream has a form, can FetchBody store it? The 'm_isReadableStream' has always been a bit confusing.

> Source/WebCore/Modules/fetch/FetchRequest.idl:34
> -// FIXME: Should include ReadableStream
>  typedef (Blob or BufferSource or DOMFormData or URLSearchParams or USVString) BodyInit;

Should this not include ReadableStream?
Comment 9 youenn fablet 2017-08-23 16:22:19 PDT
Thanks for the review.

> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318834&action=review
> 
> > Source/WebCore/Modules/fetch/FetchBody.cpp:67
> > +    }, [&](RefPtr<ReadableStream>&) mutable {
> > +        FetchBody body;
> > +        body.m_isReadableStream = true;
> > +        return body;
> 
> Now that ReadableStream has a form, can FetchBody store it? The
> 'm_isReadableStream' has always been a bit confusing.

Yes, that is the plan.

> 
> > Source/WebCore/Modules/fetch/FetchRequest.idl:34
> > -// FIXME: Should include ReadableStream
> >  typedef (Blob or BufferSource or DOMFormData or URLSearchParams or USVString) BodyInit;
> 
> Should this not include ReadableStream?

Will fix it.
Comment 10 youenn fablet 2017-08-24 08:15:45 PDT
Created attachment 318987 [details]
Patch for landing
Comment 11 Build Bot 2017-08-24 08:17:44 PDT
Attachment 318987 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/ReadableStream.h:42:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Build Bot 2017-08-24 08:18:56 PDT
Comment on attachment 318987 [details]
Patch for landing

Attachment 318987 [details] did not pass bindings-ews (mac):
Output: http://webkit-queues.webkit.org/results/4375353

New failing tests:
Please see test output for results
Comment 13 youenn fablet 2017-08-24 08:22:35 PDT
Created attachment 318988 [details]
Patch for landing
Comment 14 Build Bot 2017-08-24 08:24:54 PDT
Attachment 318988 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/ReadableStream.h:42:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 WebKit Commit Bot 2017-08-25 11:57:48 PDT
Comment on attachment 318988 [details]
Patch for landing

Rejecting attachment 318988 [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', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 318988, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
.webkit.org/git/WebKit
   60a3be7..786241c  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 221196 = 60a3be7be0314d49e5410e88f8f612aa61843bd3
r221197 = 786241ced815bc6a0eae8b6b2281848520660e1a
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: http://webkit-queues.webkit.org/results/4382158
Comment 16 WebKit Commit Bot 2017-08-25 13:09:35 PDT
Comment on attachment 318988 [details]
Patch for landing

Clearing flags on attachment: 318988

Committed r221201: <http://trac.webkit.org/changeset/221201>
Comment 17 WebKit Commit Bot 2017-08-25 13:09:36 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Radar WebKit Bug Importer 2017-08-25 13:10:29 PDT
<rdar://problem/34087334>