[webkit-reviews] review granted: [Bug 173295] [Fetch API] TypeError when called with body === {} : [Attachment 313116] Patch

Fri Jun 16 18:33:50 PDT 2017

Sam Weinig <sam at webkit.org> has granted youenn fablet <youennf at gmail.com>'s
request for review:
Bug 173295: [Fetch API] TypeError when called with body === {}
https://bugs.webkit.org/show_bug.cgi?id=173295

Attachment 313116: Patch

https://bugs.webkit.org/attachment.cgi?id=313116&action=review

--- Comment #16 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 313116
  --> https://bugs.webkit.org/attachment.cgi?id=313116
Patch

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

> Source/WebCore/ChangeLog:17
> +
> +	   Handling of ReadableStream bodies remains in JS builtin for
Response.
> +	   This allows easier handling cloning and consumption of body.
> +	   Adding setBodyAsReadableStream since this is no longer handled by
extractBody.

While not necessary for this change, which is great, I'd like to restart my
effort to understand why the streams code has to be so different than the rest
of the bindings.

> Source/WebCore/Modules/fetch/FetchBody.cpp:47
> +    if (WTF::holds_alternative<RefPtr<Blob>>(value)) {

For these kind of things, we usually use WTF::switchOn(..)

> Source/WebCore/Modules/fetch/FetchBody.h:69
> +    using DataType = Variant<RefPtr<Blob>, RefPtr<ArrayBufferView>,
RefPtr<ArrayBuffer>, RefPtr<DOMFormData>, RefPtr<URLSearchParams>, String>;

I think the names Data and DataType are too close for there not to be more
information about what they are.  Can they be named more precisely?

> Source/WebCore/Modules/fetch/FetchRequest.js:52
>      else if (!@isObject(init))
>	   @throwTypeError("Request init must be an object");
>  
> -    let headers = this. at initializeWith(input, init);
> +    const headers = this. at initializeWith(input, init);
>      @assert(headers instanceof @Headers);
>  
> -    let inputIsRequest = input instanceof @Request;
> +    const inputIsRequest = input instanceof @Request;
>      if ("headers" in init)
>	   @fillFetchHeaders(headers, init.headers)
>      else if (inputIsRequest)
>	   @fillFetchHeaders(headers, input.headers)
>  
> -    let hasInitBody = init.body !== @undefined && init.body !== null;
> -    this. at setBody(hasInitBody ? init.body : null, inputIsRequest ? input :
null);
> +    const hasInitBody = init.body !== @undefined && init.body !== null;
> +    if (hasInitBody)
> +	   this. at setBody(init.body)
> +    else
> +	   this. at setBodyFromInputRequest(inputIsRequest ? input : null);
>  
>      return this;

Does this need to remain a builtin?

> LayoutTests/webrtc/test.html:8
> +<!doctype html>
> +<html>
> +    <head>
> +	   <meta charset="utf-8">
> +	   <title>Testing basic video exchange from offerer to receiver</title>
> +	   <script src="../resources/testharness.js"></script>
> +	   <script src="../resources/testharnessreport.js"></script>
> +    </head>

Seems unrelated.