Bug 159804 - [Fetch API] Response constructor should be able to take a ReadableStream as body
Summary: [Fetch API] Response constructor should be able to take a ReadableStream as 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:
Depends on: 159808 160200
Blocks: 151937
  Show dependency treegraph
 
Reported: 2016-07-15 02:55 PDT by youenn fablet
Modified: 2016-07-26 23:39 PDT (History)
4 users (show)

See Also:


Attachments
WIP (5.98 KB, patch)
2016-07-15 07:43 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (63.85 KB, patch)
2016-07-25 08:23 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Rebasing (63.82 KB, patch)
2016-07-25 08:26 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Rebasing (62.98 KB, patch)
2016-07-25 08:30 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Fixing build (63.00 KB, patch)
2016-07-25 09:30 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Fixing build (62.98 KB, patch)
2016-07-25 09:53 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Fixing Debug build (62.91 KB, patch)
2016-07-25 10:21 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Fixing according review (63.41 KB, patch)
2016-07-25 13:32 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (58.61 KB, patch)
2016-07-26 08:34 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Removing WebCoreJSBuiltins.cpp from CMake WebCore_DERIVED_SOURCES (59.29 KB, patch)
2016-07-26 10:39 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 2016-07-15 02:55:47 PDT
As per https://fetch.spec.whatwg.org/#responsebodyinit
Comment 1 youenn fablet 2016-07-15 07:43:55 PDT
Created attachment 283761 [details]
WIP
Comment 2 youenn fablet 2016-07-25 08:23:53 PDT
Created attachment 284484 [details]
Patch
Comment 3 youenn fablet 2016-07-25 08:26:54 PDT
Created attachment 284485 [details]
Rebasing
Comment 4 WebKit Commit Bot 2016-07-25 08:28:23 PDT
Attachment 284485 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/fetch/FetchBodyConsumer.h:43:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/Modules/fetch/FetchBodyConsumer.h:45:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 2 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 youenn fablet 2016-07-25 08:30:00 PDT
Created attachment 284486 [details]
Rebasing
Comment 6 WebKit Commit Bot 2016-07-25 08:31:05 PDT
Attachment 284486 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/fetch/FetchBodyConsumer.h:43:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/Modules/fetch/FetchBodyConsumer.h:45:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 2 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 youenn fablet 2016-07-25 09:30:36 PDT
Created attachment 284492 [details]
Fixing build
Comment 8 WebKit Commit Bot 2016-07-25 09:31:57 PDT
Attachment 284492 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/fetch/FetchBodyConsumer.h:43:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/Modules/fetch/FetchBodyConsumer.h:45:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 2 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 youenn fablet 2016-07-25 09:53:24 PDT
Created attachment 284493 [details]
Fixing build
Comment 10 WebKit Commit Bot 2016-07-25 10:01:24 PDT
Attachment 284493 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/fetch/FetchBodyConsumer.h:43:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/Modules/fetch/FetchBodyConsumer.h:45:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 2 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 youenn fablet 2016-07-25 10:21:31 PDT
Created attachment 284498 [details]
Fixing Debug build
Comment 12 WebKit Commit Bot 2016-07-25 10:25:02 PDT
Attachment 284498 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/fetch/FetchBodyConsumer.h:43:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/Modules/fetch/FetchBodyConsumer.h:45:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 2 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Alex Christensen 2016-07-25 12:02:26 PDT
Comment on attachment 284498 [details]
Fixing Debug build

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

> Source/WebCore/Modules/fetch/FetchResponse.js:115
> +    return @consumeStream(this, 2);

These numbers should at least have a name.

> Source/WebCore/bindings/js/WebCoreBuiltinNames.h:56
> +    macro(cloneForJS) \

These are out of alphabetical order.
Comment 14 Alex Christensen 2016-07-25 12:15:03 PDT
Comment on attachment 284498 [details]
Fixing Debug build

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

> Source/WebCore/ChangeLog:15
> +        Clients of FetchLoader needs to oass a FetchBodyConsumer to the FetchLoader to do the data adaptation at loader creation time.

pass

> Source/WebCore/Modules/fetch/FetchLoader.h:66
> +    FetchBodyConsumer* m_consumer;

Do the FetchLoader and FetchBodyConsumer have the same lifetime?
Comment 15 youenn fablet 2016-07-25 12:38:40 PDT
(In reply to comment #13)
> Comment on attachment 284498 [details]
> Fixing Debug build
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284498&action=review
> 
> > Source/WebCore/Modules/fetch/FetchResponse.js:115
> > +    return @consumeStream(this, 2);
> 
> These numbers should at least have a name.

What do you mean?
Something like the status added for ReadableStream: @readableStreamClosed.
We can do that to ensure the values and enum class remain in sync. Is it worth it?
> 
> > Source/WebCore/bindings/js/WebCoreBuiltinNames.h:56
> > +    macro(cloneForJS) \
> 
> These are out of alphabetical order.
OK


(In reply to comment #14)
> Comment on attachment 284498 [details]
> Fixing Debug build
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284498&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        Clients of FetchLoader needs to oass a FetchBodyConsumer to the FetchLoader to do the data adaptation at loader creation time.
> 
> pass

OK

>Source/WebCore/Modules/fetch/FetchLoader.h:66
> > +    FetchBodyConsumer* m_consumer;
> 
> Do the FetchLoader and FetchBodyConsumer have the same lifetime?

FetchBodyConsumer has the safe lifetime as FetchLoaderClient.
FWIW, it might be good to retire FetchLoader and move the code in its clients (FetchBodyOwner for blob loading and FetchResponse for fetch loads).
Comment 16 Alex Christensen 2016-07-25 12:43:35 PDT
(In reply to comment #15)
> (In reply to comment #13)
> > Comment on attachment 284498 [details]
> > Fixing Debug build
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=284498&action=review
> > 
> > > Source/WebCore/Modules/fetch/FetchResponse.js:115
> > > +    return @consumeStream(this, 2);
> > 
> > These numbers should at least have a name.
> 
> What do you mean?
We should at least have something like let meaningfulName = 2;
Comment 17 youenn fablet 2016-07-25 13:32:56 PDT
Created attachment 284522 [details]
Fixing according review
Comment 18 WebKit Commit Bot 2016-07-25 13:34:31 PDT
Attachment 284522 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/fetch/FetchBodyConsumer.h:43:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/Modules/fetch/FetchBodyConsumer.h:45:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 2 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 youenn fablet 2016-07-25 13:35:21 PDT
(In reply to comment #17)
> Created attachment 284522 [details]
> Fixing according review

Thanks for the review.
I integrated the different points you mentioned.
Comment 20 Alex Christensen 2016-07-25 15:59:57 PDT
Comment on attachment 284522 [details]
Fixing according review

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

Test failure might be unrelated.  Please double check before landing.

> Source/WebCore/Modules/fetch/FetchResponse.js:137
> +    return @consumeStream(this, 3);

jsonConsumerType
Comment 21 youenn fablet 2016-07-26 08:34:36 PDT
Created attachment 284592 [details]
Patch for landing
Comment 22 youenn fablet 2016-07-26 08:58:42 PDT
> > Source/WebCore/Modules/fetch/FetchResponse.js:137
> > +    return @consumeStream(this, 3);
> 
> jsonConsumerType

Done
Comment 23 WebKit Commit Bot 2016-07-26 09:04:13 PDT
Comment on attachment 284592 [details]
Patch for landing

Clearing flags on attachment: 284592

Committed r203719: <http://trac.webkit.org/changeset/203719>
Comment 24 WebKit Commit Bot 2016-07-26 09:04:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 WebKit Commit Bot 2016-07-26 10:12:14 PDT
Re-opened since this is blocked by bug 160200
Comment 26 youenn fablet 2016-07-26 10:15:58 PDT
(In reply to comment #25)
> Re-opened since this is blocked by bug 160200

WebCoreJSBuiltins.cpp is added in CMakeLists.txt while it does not need to.
Comment 27 youenn fablet 2016-07-26 10:39:09 PDT
Created attachment 284603 [details]
Removing WebCoreJSBuiltins.cpp from CMake WebCore_DERIVED_SOURCES
Comment 28 WebKit Commit Bot 2016-07-26 10:41:14 PDT
Attachment 284603 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/fetch/FetchBodyConsumer.h:43:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebCore/Modules/fetch/FetchBodyConsumer.h:45:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 2 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 29 WebKit Commit Bot 2016-07-26 23:39:27 PDT
Comment on attachment 284603 [details]
Removing WebCoreJSBuiltins.cpp from CMake WebCore_DERIVED_SOURCES

Clearing flags on attachment: 284603

Committed r203767: <http://trac.webkit.org/changeset/203767>
Comment 30 WebKit Commit Bot 2016-07-26 23:39:30 PDT
All reviewed patches have been landed.  Closing bug.