Bug 155359 – [Fetch API] Implement data resolution for blob stored in Body

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

Implement data resolution for blob stored in Body

[youenn fablet]

Created attachment 273723 [details]
Patch

[WebKit Commit Bot]

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


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


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

[youenn fablet]

(In reply to comment #1)
> Created attachment 273723 [details]
> Patch

This patch is introducing FetchLoader as a wrapper above ThreadableLoader.
In the future, FetchLoader could become the only client of ThreadableLoader, the other clients of ThreadableLoader (XHR, FileReaderLoader) going through FetchLoader.

[Darin Adler]

Comment on attachment 273723 [details]
Patch

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

> Source/WebCore/Modules/fetch/FetchBody.cpp:235
> +void FetchBody::loadedAsArrayBuffer(RefPtr<ArrayBuffer>&& buffer)

This function does not seem to take ownership of the buffer. Should the argument be ArrayBuffer& or ArrayBuffer* instead? Are there missing calls to WTFMove?

> Source/WebCore/Modules/fetch/FetchBody.cpp:251
> +void FetchBody::loadedAsText(ScriptExecutionContext* context, String&& text)

This function does not seem to take ownership of the text. Should the argument be const String& instead? Are there missing calls to WTFMove?

Does this function work with a context of nullptr?

> Source/WebCore/Modules/fetch/FetchBody.h:36
> +#include "FetchLoader.h"

Can you remove any other includes now that we are including FetchLoader.h here?

> Source/WebCore/Modules/fetch/FetchBodyOwner.cpp:58
> +    m_blobLoader = BlobLoader(*this);

Should we be doing ASSERT(!m_blobLoader) here? Seems that if we are already loading, then destroying the loader without calling stop() would not do the right thing.

Should we change this so it’s harder to user wrong? Multiple calls to this seems like they could happen.

Also, should just be able to write:

    m_blobLoader = *this;

Even though it might be slightly oblique, I think it’s a good way to write it.

> Source/WebCore/Modules/fetch/FetchBodyOwner.cpp:59
> +    m_blobLoader->loader = std::make_unique<FetchLoader>(type, *m_blobLoader);

Shouldn’t the BlobLoader constructor do this instead of doing this here?

> Source/WebCore/Modules/fetch/FetchBodyOwner.cpp:69
> +    m_blobLoader = Nullopt;
> +    unsetPendingActivity(this);

This should ASSERT(m_blobLoader).

> Source/WebCore/Modules/fetch/FetchBodyOwner.h:68
> +    struct BlobLoader final : public FetchLoaderClient {

public is the default for struct inheritance so you could omit that

> Source/WebCore/Modules/fetch/FetchBodyOwner.h:69
> +        BlobLoader(FetchBodyOwner& o) : owner(o) { }

We prefer words, like "owner", over letters, like "o", in WebKit coding style. In a constructor it can even be the same as the data member name, and that just works fine.

> Source/WebCore/Modules/fetch/FetchLoader.h:40
> +class Blob;

Forward declared but not used.

> Source/WebCore/Modules/fetch/FetchLoader.h:46
> +    // FIXME: Support Blob as loading type.

Not 100% sure this needs to be marked FIXME. Seems like if we are adding Blob, then we will understand we need to add the type. If we are not, the FIXME isn’t the right place to remind us to do that. Just my take.

> Source/WebCore/Modules/fetch/FetchLoader.h:58
> +    // ThreadableLoaderClient API.
> +    void didReceiveResponse(unsigned long, const ResourceResponse&) final;
> +    void didReceiveData(const char*, int) final;
> +    void didFinishLoading(unsigned long, double) final;
> +    void didFail(const ResourceError&) final;

Do these need to be public?

> Source/WebCore/Modules/fetch/FetchLoader.h:61
> +    Type m_type = Type::ArrayBuffer;

We’ve been using the brace syntax instead:

    Type m_type { Type::ArrayBuffer };

I’m not sure I prefer it, but I like the idea of being consistent.

> Source/WebCore/Modules/fetch/FetchLoaderClient.h:37
> +namespace WTF {
> +template<typename T> class RefPtr;
> +class String;
> +} // namespace WTF

Supposed to use <wtf/Forward.h> instead of doing this.

> Source/WebCore/Modules/fetch/FetchLoaderClient.h:42
> +namespace JSC {
> +class ArrayBuffer;
> +} // namespace JSC

Namespace comment doesn’t seem to add anything when it’s this small a block.

[youenn fablet]

Thanks for the review.
I will update the patch accordingly.

Please see inline for additional comments.

> > Source/WebCore/Modules/fetch/FetchBody.cpp:235
> > +void FetchBody::loadedAsArrayBuffer(RefPtr<ArrayBuffer>&& buffer)
> 
> This function does not seem to take ownership of the buffer. Should the
> argument be ArrayBuffer& or ArrayBuffer* instead? Are there missing calls to
> WTFMove?

Plan is that buffer will be moved to a FetchBody field in the case of regular fetch loading, for which result call (json, arraybuffer...) is not pending.

That said, the point is more about whether FetchLoaderClient API should use a rvalue or just a reference.
FileReader might for instance naturally take ownership of the loaded data (buffer or string).


> > Source/WebCore/Modules/fetch/FetchBody.cpp:251
> > +void FetchBody::loadedAsText(ScriptExecutionContext* context, String&& text)
> 
> This function does not seem to take ownership of the text. Should the
> argument be const String& instead? Are there missing calls to WTFMove?

There is no plan for FetchBody to take ownership of the string here.
This is more for consistency.

> Does this function work with a context of nullptr?

Changed that to a reference here.

> > Source/WebCore/Modules/fetch/FetchBody.h:36
> > +#include "FetchLoader.h"
> 
> Can you remove any other includes now that we are including FetchLoader.h
> here?

FetchLoader.h is very light on include and there is no savings here.

> > Source/WebCore/Modules/fetch/FetchBodyOwner.cpp:58
> > +    m_blobLoader = BlobLoader(*this);
> 
> Should we be doing ASSERT(!m_blobLoader) here? Seems that if we are already
> loading, then destroying the loader without calling stop() would not do the
> right thing.

I added two asserts here: one for blobLoader and one checking that body is in consumed mode, which means that JS API will not be able to trigger a call to loadBlob.

> Should we change this so it’s harder to user wrong? Multiple calls to this
> seems like they could happen.

It cannot happen right now but I guess additional code may create such bug.
Ideally, loadBlob should be a private method that would only be called by FetchBody.
We can make FetchBody as a friend of FetchBodyOwner.

> Also, should just be able to write:
> 
>     m_blobLoader = *this;
> 
> Even though it might be slightly oblique, I think it’s a good way to write
> it.
> 
> > Source/WebCore/Modules/fetch/FetchBodyOwner.cpp:59
> > +    m_blobLoader->loader = std::make_unique<FetchLoader>(type, *m_blobLoader);
> 
> Shouldn’t the BlobLoader constructor do this instead of doing this here?

Changed the constructor.

> > Source/WebCore/Modules/fetch/FetchBodyOwner.cpp:69
> > +    m_blobLoader = Nullopt;
> > +    unsetPendingActivity(this);
> 
> This should ASSERT(m_blobLoader).

OK

> > Source/WebCore/Modules/fetch/FetchBodyOwner.h:68
> > +    struct BlobLoader final : public FetchLoaderClient {
> 
> public is the default for struct inheritance so you could omit that

OK

> > Source/WebCore/Modules/fetch/FetchBodyOwner.h:69
> > +        BlobLoader(FetchBodyOwner& o) : owner(o) { }
> 
> We prefer words, like "owner", over letters, like "o", in WebKit coding
> style. In a constructor it can even be the same as the data member name, and
> that just works fine.

OK

> > Source/WebCore/Modules/fetch/FetchLoader.h:40
> > +class Blob;
> 
> Forward declared but not used.

OK

> > Source/WebCore/Modules/fetch/FetchLoader.h:46
> > +    // FIXME: Support Blob as loading type.
> 
> Not 100% sure this needs to be marked FIXME. Seems like if we are adding
> Blob, then we will understand we need to add the type. If we are not, the
> FIXME isn’t the right place to remind us to do that. Just my take.

Yes, it is more a reminder for me.
I removed it.

> > Source/WebCore/Modules/fetch/FetchLoader.h:58
> > +    // ThreadableLoaderClient API.
> > +    void didReceiveResponse(unsigned long, const ResourceResponse&) final;
> > +    void didReceiveData(const char*, int) final;
> > +    void didFinishLoading(unsigned long, double) final;
> > +    void didFail(const ResourceError&) final;
> 
> Do these need to be public?

Changed to private.

> > Source/WebCore/Modules/fetch/FetchLoader.h:61
> > +    Type m_type = Type::ArrayBuffer;
> 
> We’ve been using the brace syntax instead:
> 
>     Type m_type { Type::ArrayBuffer };
> 
> I’m not sure I prefer it, but I like the idea of being consistent.

OK

> > Source/WebCore/Modules/fetch/FetchLoaderClient.h:37
> > +namespace WTF {
> > +template<typename T> class RefPtr;
> > +class String;
> > +} // namespace WTF
> 
> Supposed to use <wtf/Forward.h> instead of doing this.

OK

> > Source/WebCore/Modules/fetch/FetchLoaderClient.h:42
> > +namespace JSC {
> > +class ArrayBuffer;
> > +} // namespace JSC
> 
> Namespace comment doesn’t seem to add anything when it’s this small a block.

OK

[youenn fablet]

Created attachment 273952 [details]
Patch for landing

[youenn fablet]

Created attachment 273954 [details]
Patch for landing

[youenn fablet]

  (In reply to comment #7)
> Created attachment 273954 [details]
> Patch for landing

Removed unneeded line and readded needed Blob class declaration.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 273954 [details]:

The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 273954 [details]:

The commit-queue is continuing to process your patch.

[youenn fablet]

Created attachment 273964 [details]
Patch for landing

[youenn fablet]

Created attachment 273967 [details]
Patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 273964 [details]:

The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 273964 [details]:

The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 273967 [details]
Patch for landing

Rejecting attachment 273967 [details] from commit-queue.

New failing tests:
transitions/default-timing-function.html
Full output: http://webkit-queues.webkit.org/results/977320

[WebKit Commit Bot]

Created attachment 273975 [details]
Archive of layout-test-results from webkit-cq-03 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-03  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 273967 [details]:

transitions/default-timing-function.html bug 138901 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 273967 [details]:

The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 273967 [details]
Patch for landing

Clearing flags on attachment: 273967

Committed r198133: <http://trac.webkit.org/changeset/198133>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

Debug build fix in r198134.