Bug 153437 – [Fetch API] Implement Fetch API Request

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

Implement https://fetch.spec.whatwg.org/#request-class

[youenn fablet]

Created attachment 269882 [details]
Patch

[youenn fablet]

Created attachment 270112 [details]
Adding more tests

[youenn fablet]

Created attachment 270115 [details]
More tests and updating body init

[WebKit Commit Bot]

Attachment 270115 [details] did not pass style-queue:


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


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 270115 [details]
More tests and updating body init

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

We should consider later if it is better to use AtomicString more in this code. ASCII literals in JavaScript are already atomic strings, so it’s fast to pass them to a function that takes const AtomicString& and if we make AtomicString constants for the various expected strings we can compare more quickly than a string compare.

> Source/WebCore/Modules/fetch/FetchBody.cpp:51
> +        m_formData= JSDOMFormData::toWrapped(value);

Missing space before the "=" here.

> Source/WebCore/Modules/fetch/FetchBody.cpp:69
> +    m_blob = nullptr;
> +    m_formData = nullptr;
> +    m_text = String();

This leaves behind m_mimeType. Is that OK? Is there a test covering this?

> Source/WebCore/Modules/fetch/FetchBody.h:89
> +    bool m_disturbedFlag = false;

I think a better name for this is m_isDisturbed or m_wasDisturbed. Generally we want to name booleans with adjective phrases, not the noun "flag", since that way of naming makes it more explicit what the boolean means.

> Source/WebCore/Modules/fetch/FetchHeaders.cpp:176
> +    for (auto iter = headers.m_headers.begin(); iter != headers.m_headers.end(); ++iter) {
> +        if (canWriteHeader(iter->key, iter->value, m_guard, ec)) {
> +            if (iter->keyAsHTTPHeaderName)
> +                m_headers.add(iter->keyAsHTTPHeaderName.value(), iter->value);
> +            else
> +                m_headers.add(iter->key, iter->value);
> +        }
> +    }

WebKit style prohibits an abbreviation like "iter". But also, this can be a modern for loop instead:

    for (auto& header : headers.m_headers) {
        if (canWriteHeader(header.key, header.value, m_guard, ec)) {
            if (header.keyAsHTTPHeaderName)
                m_headers.add(header.keyAsHTTPHeaderName.value(), header.value);
            else
                m_headers.add(header.key, header.value);
        }
    }

I think it reads more clearly this way.

> Source/WebCore/Modules/fetch/FetchRequest.cpp:86
> +    : m_referrer("client")

ASCIILiteral("client")

> Source/WebCore/Modules/fetch/FetchRequest.cpp:138
> +        if ((method != "GET" && method != "POST" && method != "HEAD") || !m_integrity.isEmpty())

Is it OK to compare the method in a case matching way? Maybe this should be using a comparison that ignores differences in case?

> Source/WebCore/Modules/fetch/FetchRequest.cpp:160
> +    if (!isEmpty() && (m_request.httpMethod() == "GET" || m_request.httpMethod() == "HEAD"))

Is it OK to compare the method in a case matching way? Maybe this should be using a comparison that ignores differences in case?

> Source/WebCore/Modules/fetch/FetchRequest.cpp:179
> +    if (referrerURL.protocol() == "about" && referrerURL.path().contains("client")) {

It’s not correct to use == to compare protocols, since protocols are ASCII case insensitive. This should use the URL::protocolIs function.

> Source/WebCore/Modules/fetch/FetchRequest.cpp:408
> +    String upper = method.upper();

This should be using convertToASCIIUppercase. I also think that “upper” is not a great name for the local variable.

> Source/WebCore/Modules/fetch/FetchRequest.h:46
> +class FetchRequest : public FetchBody {

It’s peculiar to have a FetchRequest derive from FetchBody instead of just having a FetchBody as a member. Is this strange class hierarchy dictated by the specification?

> Source/WebCore/Modules/fetch/FetchRequest.h:51
> +    String method () const { return m_request.httpMethod(); }

Extra space after the word "method" here.

More efficient to have the type be const String& rather than String, avoids a bit of unnecessary reference count churn.

> Source/WebCore/Modules/fetch/FetchRequest.h:52
> +    String url() const { return m_request.url().string(); }

More efficient to have the type be const String& rather than String, avoids a bit of unnecessary reference count churn.

> Source/WebCore/Modules/fetch/FetchRequest.h:62
> +    String type() const;
> +    String destination() const;
> +    String referrer() const;
> +    String referrerPolicy() const;
> +    String mode() const;
> +    String credentials() const;
> +    String cache() const;
> +    String redirect() const;
> +    String integrity() const { return m_integrity; }

More efficient to have the type be const String& rather than String in as many of these functions as possible; avoids a bit of unnecessary reference count churn.

Note that if the code is returning an ASCIILiteral, we do need to return String unless we put the value in a global variable so we can return it without creating a new string each time.

> Source/WebCore/Modules/fetch/FetchRequest.h:70
> +    FetchRequest(const FetchRequest&);

We should consider whether we want to make a move constructor in addition to, or instead of, the copy constructor. I’d like to understand where we copy one of these FetchRequest objects and why. I think it would be better to delete the copy constructor, second best would be to have a move constructor and not a copy constructor.

It’s a mistake to have a copy constructor but no assignment operator. We need to either define or delete the assignment operator.

> Source/WebCore/Modules/fetch/FetchRequest.h:81
> +    RefPtr<FetchHeaders> m_headers;

It would be much better if we could find a way to make this a Ref instead of a RefPtr, since it’s not legal for it to be null. But that might be tricky given the way the code in FetchRequest::init is written. It would be cleaner to do more of the *before* constructing the object in the create functions, rather than making a half-formed object and the setting it up a bit at a time. Ideally I’d like to see us get rid of the init function.

[Darin Adler]

I’d like to see more thorough test coverage that focuses on testing the FetchRequest itself without actually doing a fetch, but maybe that’s not possible; maybe the actual content is only visible indirectly by seeing what’s sent out in the fetch.

[youenn fablet]

Created attachment 270214 [details]
Updated according review

[youenn fablet]

Thanks for the review.
Based on your comment, I rewrote the patch to remove the init method.
That said, the code is now less straightfowardly mapping to the spec.

Given the amount of change, a new r? makes sense.

As of the test coverage, this patch adds a lot of tests dedicated to cover various aspects of Request (no need for actual fetch in those cases). It is not yet fully complete, but this should improve over time.

More comments below.

(In reply to comment #5)
> Comment on attachment 270115 [details]
> More tests and updating body init
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270115&action=review
> 
> > Source/WebCore/Modules/fetch/FetchBody.cpp:69
> > +    m_blob = nullptr;
> > +    m_formData = nullptr;
> > +    m_text = String();
> 
> This leaves behind m_mimeType. Is that OK? Is there a test covering this?

This follows the spec.
I am not entirely sure m_mimeType is actually needed for the implementation.
But I prefer starting with code that matches the spec.
AIUI, m_mimeType could be emptied, as this might not be observable.
I added a FIXME in the latest patch for that.

> 
> > Source/WebCore/Modules/fetch/FetchBody.h:89
> > +    bool m_disturbedFlag = false;
> 
> I think a better name for this is m_isDisturbed or m_wasDisturbed. Generally
> we want to name booleans with adjective phrases, not the noun "flag", since
> that way of naming makes it more explicit what the boolean means.

Renamed to m_isDisturbed.

> WebKit style prohibits an abbreviation like "iter". But also, this can be a
> modern for loop instead:
> 
>     for (auto& header : headers.m_headers) {
>         if (canWriteHeader(header.key, header.value, m_guard, ec)) {
>             if (header.keyAsHTTPHeaderName)
>                 m_headers.add(header.keyAsHTTPHeaderName.value(),
> header.value);
>             else
>                 m_headers.add(header.key, header.value);
>         }
>     }
> 
> I think it reads more clearly this way.

Fixed accordingly.

> > Source/WebCore/Modules/fetch/FetchRequest.cpp:86
> > +    : m_referrer("client")
> 
> ASCIILiteral("client")

OK

> > Source/WebCore/Modules/fetch/FetchRequest.cpp:138
> > +        if ((method != "GET" && method != "POST" && method != "HEAD") || !m_integrity.isEmpty())
> 
> Is it OK to compare the method in a case matching way? Maybe this should be
> using a comparison that ignores differences in case?

This is ok as the spec mandates usual verbs to be uppercased (for compatibility purposes) in setMethod routine.

> > Source/WebCore/Modules/fetch/FetchRequest.cpp:179
> > +    if (referrerURL.protocol() == "about" && referrerURL.path().contains("client")) {
> 
> It’s not correct to use == to compare protocols, since protocols are ASCII
> case insensitive. This should use the URL::protocolIs function.

Fixed.

> > Source/WebCore/Modules/fetch/FetchRequest.cpp:408
> > +    String upper = method.upper();
> 
> This should be using convertToASCIIUppercase. I also think that “upper” is
> not a great name for the local variable.

OK.

> > Source/WebCore/Modules/fetch/FetchRequest.h:46
> > +class FetchRequest : public FetchBody {
> 
> It’s peculiar to have a FetchRequest derive from FetchBody instead of just
> having a FetchBody as a member. Is this strange class hierarchy dictated by
> the specification?

I initially thought it was needed by the WebIDL binding but it is not since there is no JSBody.
Updated accordingly.

> > Source/WebCore/Modules/fetch/FetchRequest.h:81
> > +    RefPtr<FetchHeaders> m_headers;
> 
> It would be much better if we could find a way to make this a Ref instead of
> a RefPtr, since it’s not legal for it to be null. But that might be tricky
> given the way the code in FetchRequest::init is written. It would be cleaner
> to do more of the *before* constructing the object in the create functions,
> rather than making a half-formed object and the setting it up a bit at a
> time. Ideally I’d like to see us get rid of the init function.

init method removed.

[WebKit Commit Bot]

Attachment 270214 [details] did not pass style-queue:


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


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Comment on attachment 270214 [details]
Updated according review

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

> Source/WebCore/Modules/fetch/FetchBody.cpp:47
> +    ASSERT(blob);

If this should never be null, can this take a Blob&?

> Source/WebCore/Modules/fetch/FetchBody.cpp:56
> +    // FIXME: Handle the boundary parameter of multipart/form-data MIME type.
> +    ASSERT(formData);

If this should never be null, can this take a DOMFormData&?

[Darin Adler]

Comment on attachment 270214 [details]
Updated according review

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

> Source/WebCore/Modules/fetch/FetchBody.cpp:94
> +FetchBody FetchBody::fromRequestBody(FetchBody* body)
> +{
> +    if (!body)
> +        return FetchBody();
> +
> +    FetchBody result;
> +    result.m_type = body->m_type;
> +    result.m_mimeType = body->m_mimeType;
> +
> +    // FIXME: Once implementation is completed, check whether using a move constructor and set m_mimeType to the empty string.
> +    result.m_blob = WTFMove(body->m_blob);
> +    result.m_formData = WTFMove(body->m_formData);
> +    result.m_text = WTFMove(body->m_text);
> +
> +    body->m_isDisturbed = true;
> +
> +    return result;
> +}

I’m puzzled by this function. Why do we move the contents of one FetchBody to FetchBody, leaving behind copies of the type and MIME type strings, but no content. Why do we want to support this operation?

Maybe that’s what the FIXME means, although it’s not a complete thought so I was not able to figure out for sure what it meant.

> Source/WebCore/Modules/fetch/FetchBody.cpp:103
> +    if (isDisturbed()) {

A little peculiar to use isDisturbed() here but then set m_disturbed on the next line after the if. More consistent to use the data member in both cases.

> Source/WebCore/Modules/fetch/FetchBody.cpp:120
> +        CString data = UTF8Encoding().encode(m_text, EntitiesForUnencodables);
> +        Vector<unsigned char> value(data.length());
> +        // FIXME: refactor this code.
> +        memcpy(value.data(), data.data(), data.length());

Are we using UTF8Encoding() because we need the “EntitiesForUnencodables” behavior?

The FIXME here isn’t clear enough. I think you are saying you’d like to convert the string into a buffer of UTF-8 characters without having to allocate an intermediate CString.

> Source/WebCore/Modules/fetch/FetchBody.cpp:179
> +        JSC::JSValue value = JSC::JSONParse(&state, m_text);
> +        if (!value)
> +            promise.reject(SYNTAX_ERR);
> +        else
> +            promise.resolve(value);

Does JSONParse really guarantee it will return nullptr if it fails? I’m surprised by this!

> Source/WebCore/Modules/fetch/FetchBody.h:50
> +    typedef DOMPromise<Vector<unsigned char>, ExceptionCode> ArrayBufferPromise;

Why unsigned char for the bytes instead of char?

> Source/WebCore/Modules/fetch/FetchBody.h:51
> +    void arrayBuffer(ArrayBufferPromise&&);

Why take an rvalue reference if we are not moving the argument in? Maybe this should just be a plain old reference.

> Source/WebCore/Modules/fetch/FetchBody.h:54
> +    void formData(FormDataPromise&&);

Ditto.

> Source/WebCore/Modules/fetch/FetchBody.h:57
> +    void blob(BlobPromise&&);

Ditto.

> Source/WebCore/Modules/fetch/FetchBody.h:60
> +    void json(JSC::ExecState&, JSONPromise&&);

Ditto.

> Source/WebCore/Modules/fetch/FetchBody.h:63
> +    void text(TextPromise&&);

Ditto.

> Source/WebCore/Modules/fetch/FetchBody.h:71
> +    static FetchBody fromJSValue(JSC::JSValue, JSC::ExecState&);

We normally put the ExecState argument first.

> Source/WebCore/Modules/fetch/FetchBody.h:72
> +    static FetchBody fromRequestBody(FetchBody*);

Unclear to me why we want this to move data out of the passed-in FetchBody. Peculiar design. Why not just use the FetchBody or use WTFMove if we want to move it rather than writing a function to do it.

> Source/WebCore/Modules/fetch/FetchBody.h:82
> +    enum class Type {
> +        None,
> +        Text,
> +        Blob,
> +        FormData
> +    };

I’d find this easier to read as a one-liner.

> Source/WebCore/Modules/fetch/FetchBody.h:84
> +    FetchBody(Blob*);

Could make this more efficient by taking a Ref<Blob>&& and moving it in.

> Source/WebCore/Modules/fetch/FetchBody.h:85
> +    FetchBody(DOMFormData*);

Could make this more efficient by taking a Ref<DOMFormData>&& and moving it in.

> Source/WebCore/Modules/fetch/FetchBody.h:86
> +    FetchBody(const String&);

Could make this more efficient by taking a String&& and moving it in.

[youenn fablet]

Created attachment 270381 [details]
Patch for landing

[youenn fablet]

> I’m puzzled by this function. Why do we move the contents of one FetchBody
> to FetchBody, leaving behind copies of the type and MIME type strings, but
> no content. Why do we want to support this operation?
> Maybe that’s what the FIXME means, although it’s not a complete thought so I
> was not able to figure out for sure what it meant.

At now, I just try to follow closely the spec.
I think it is safe resetting m_mimeType.
Once Response is implemented, it may be time to use the move constructor.
I improved the FIXME.

> > Source/WebCore/Modules/fetch/FetchBody.cpp:103
> > +    if (isDisturbed()) {
> 
> A little peculiar to use isDisturbed() here but then set m_disturbed on the
> next line after the if. More consistent to use the data member in both cases.

OK

> > Source/WebCore/Modules/fetch/FetchBody.cpp:120
> > +        CString data = UTF8Encoding().encode(m_text, EntitiesForUnencodables);
> > +        Vector<unsigned char> value(data.length());
> > +        // FIXME: refactor this code.
> > +        memcpy(value.data(), data.data(), data.length());
> 
> Are we using UTF8Encoding() because we need the “EntitiesForUnencodables”
> behavior?
> 
> The FIXME here isn’t clear enough. I think you are saying you’d like to
> convert the string into a buffer of UTF-8 characters without having to
> allocate an intermediate CString.

I removed UTF8Encoding use and made the FIXME clearer.

> > Source/WebCore/Modules/fetch/FetchBody.cpp:179
> > +        JSC::JSValue value = JSC::JSONParse(&state, m_text);
> > +        if (!value)
> > +            promise.reject(SYNTAX_ERR);
> > +        else
> > +            promise.resolve(value);
> 
> Does JSONParse really guarantee it will return nullptr if it fails? I’m
> surprised by this!

There is a test for that in the patch.
I beefed up it with additional good and bad values at the end of fetch/api/request/request-consume.html

> 
> > Source/WebCore/Modules/fetch/FetchBody.h:50
> > +    typedef DOMPromise<Vector<unsigned char>, ExceptionCode> ArrayBufferPromise;
> 
> Why unsigned char for the bytes instead of char?

The ArrayBuffer resolve method in JSDOMPromise.h is expecting a Vector<unsigned char>.

> > Source/WebCore/Modules/fetch/FetchBody.h:51
> > +    void arrayBuffer(ArrayBufferPromise&&);
> 
> Why take an rvalue reference if we are not moving the argument in? Maybe
> this should just be a plain old reference.

We will need it at some point, for instance when resolving a blob data as an array buffer.
Also JSFetchRequest is moving the DeferredWrapper so FetchRequest methods will take rvalue references.

> > Source/WebCore/Modules/fetch/FetchBody.h:71
> > +    static FetchBody fromJSValue(JSC::JSValue, JSC::ExecState&);
> 
> We normally put the ExecState argument first.

OK

> > Source/WebCore/Modules/fetch/FetchBody.h:82
> > +    enum class Type {
> > +        None,
> > +        Text,
> > +        Blob,
> > +        FormData
> > +    };
> 
> I’d find this easier to read as a one-liner.

OK

> > Source/WebCore/Modules/fetch/FetchBody.h:84
> > +    FetchBody(Blob*);
> 
> Could make this more efficient by taking a Ref<Blob>&& and moving it in.

OK

> > Source/WebCore/Modules/fetch/FetchBody.h:85
> > +    FetchBody(DOMFormData*);
> 
> Could make this more efficient by taking a Ref<DOMFormData>&& and moving it
> in.

OK

> > Source/WebCore/Modules/fetch/FetchBody.h:86
> > +    FetchBody(const String&);
> 
> Could make this more efficient by taking a String&& and moving it in.

OK

[WebKit Commit Bot]

Comment on attachment 270381 [details]
Patch for landing

Clearing flags on attachment: 270381

Committed r195954: <http://trac.webkit.org/changeset/195954>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.