WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
155359
[Fetch API] Implement data resolution for blob stored in Body
https://bugs.webkit.org/show_bug.cgi?id=155359
Summary
[Fetch API] Implement data resolution for blob stored in Body
youenn fablet
Reported
2016-03-11 05:42:36 PST
Implement data resolution for blob stored in Body
Attachments
Patch
(36.50 KB, patch)
2016-03-11 06:42 PST
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch for landing
(37.15 KB, patch)
2016-03-14 03:38 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch for landing
(37.09 KB, patch)
2016-03-14 03:58 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch for landing
(37.09 KB, patch)
2016-03-14 05:42 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Patch for landing
(37.06 KB, patch)
2016-03-14 06:46 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from webkit-cq-03 for mac-yosemite
(811.50 KB, application/zip)
2016-03-14 08:08 PDT
,
WebKit Commit Bot
no flags
Details
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
youenn fablet
Comment 1
2016-03-11 06:42:29 PST
Created
attachment 273723
[details]
Patch
WebKit Commit Bot
Comment 2
2016-03-11 06:45:34 PST
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
Comment 3
2016-03-11 06:47:38 PST
(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 4
2016-03-11 12:43:21 PST
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
Comment 5
2016-03-14 03:36:15 PDT
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
Comment 6
2016-03-14 03:38:40 PDT
Created
attachment 273952
[details]
Patch for landing
youenn fablet
Comment 7
2016-03-14 03:58:00 PDT
Created
attachment 273954
[details]
Patch for landing
youenn fablet
Comment 8
2016-03-14 03:58:54 PDT
(In reply to
comment #7
)
> Created
attachment 273954
[details]
> Patch for landing
Removed unneeded line and readded needed Blob class declaration.
WebKit Commit Bot
Comment 9
2016-03-14 05:11:30 PDT
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
Comment 10
2016-03-14 05:11:33 PDT
The commit-queue encountered the following flaky tests while processing
attachment 273954
[details]
: The commit-queue is continuing to process your patch.
youenn fablet
Comment 11
2016-03-14 05:42:43 PDT
Created
attachment 273964
[details]
Patch for landing
youenn fablet
Comment 12
2016-03-14 06:46:39 PDT
Created
attachment 273967
[details]
Patch for landing
WebKit Commit Bot
Comment 13
2016-03-14 06:53:21 PDT
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 14
2016-03-14 06:53:23 PDT
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 15
2016-03-14 08:07:59 PDT
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
Comment 16
2016-03-14 08:08:01 PDT
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
Comment 17
2016-03-14 08:34:05 PDT
The commit-queue encountered the following flaky tests while processing
attachment 273967
[details]
: transitions/default-timing-function.html
bug 138901
(author:
simon.fraser@apple.com
) The commit-queue is continuing to process your patch.
WebKit Commit Bot
Comment 18
2016-03-14 08:34:07 PDT
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 19
2016-03-14 08:58:02 PDT
Comment on
attachment 273967
[details]
Patch for landing Clearing flags on attachment: 273967 Committed
r198133
: <
http://trac.webkit.org/changeset/198133
>
WebKit Commit Bot
Comment 20
2016-03-14 08:58:05 PDT
All reviewed patches have been landed. Closing bug.
Alexey Proskuryakov
Comment 21
2016-03-14 09:17:14 PDT
Debug build fix in
r198134
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug