Bug 175658 – [Cache API] Add response body storage

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

[youenn fablet]

[Cache API] Add response body storage

[youenn fablet]

Created attachment 318332 [details]
Patch

[youenn fablet]

Created attachment 318375 [details]
Patch

[youenn fablet]

Created attachment 318377 [details]
Patch

[youenn fablet]

Created attachment 318389 [details]
Patch

[youenn fablet]

Created attachment 318390 [details]
Patch

[Alex Christensen]

Comment on attachment 318390 [details]
Patch

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

> Source/WebCore/Modules/fetch/FetchResponse.h:109
> +    static void startFetching(ScriptExecutionContext&, FetchRequest&, std::optional<FetchPromise>&&, std::optional<EndOfLoad>&&);

I don't think it's elegant to change FetchResponse from a class that used to always resolve a promise when it's done to a class that either resolves a promise when it's done or calls a lambda.  I think instead we should make it a class that always calls a lambda when it's done and the lambda would resolve a promise in the cases where it should, or we should take a Variant<FetchPromise, EndOfLoadCallback> to make it clear that it does one or the other, then make better abstractions for what it is supposed to do when it's done.

[youenn fablet]

> I don't think it's elegant to change FetchResponse from a class that used to
> always resolve a promise when it's done to a class that either resolves a
> promise when it's done or calls a lambda.  I think instead we should make it
> a class that always calls a lambda when it's done and the lambda would
> resolve a promise in the cases where it should, or we should take a
> Variant<FetchPromise, EndOfLoadCallback> to make it clear that it does one
> or the other, then make better abstractions for what it is supposed to do
> when it's done.

The two lambdas will be useful for the same fetch response in the future.
I will change the promise into a lambda and add a setter for EndOfLoadCallback.

[youenn fablet]

Created attachment 318514 [details]
Patch

[Alex Christensen]

Comment on attachment 318514 [details]
Patch

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

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:114
> +static void encodeSharedBuffer(Encoder& encoder, const SharedBuffer* buffer)

We're just moving this code to use it below?  We already kind of have this in SharedBufferDataReference.  This should be united with that, and maybe that should be cleaned up.

[youenn fablet]

(In reply to Alex Christensen from comment #9)
> Comment on attachment 318514 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318514&action=review
> 
> > Source/WebKit/Shared/WebCoreArgumentCoders.cpp:114
> > +static void encodeSharedBuffer(Encoder& encoder, const SharedBuffer* buffer)
> 
> We're just moving this code to use it below?  We already kind of have this
> in SharedBufferDataReference.  This should be united with that, and maybe
> that should be cleaned up.

Didn't know about SharedBufferDataReference.
Code is moved and refreshed a bit.
It also fixes a potential bug when trying to encode a SharedBuffer whose size is 0.

[youenn fablet]

Comment on attachment 318514 [details]
Patch

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

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:367
> +        responseBody = buffer.releaseNonNull();

But then, we should only set responseBody if buffer is not null here...

[youenn fablet]

Created attachment 318519 [details]
Patch for landing

[Alex Christensen]

(In reply to youenn fablet from comment #10)
> Didn't know about SharedBufferDataReference.
We should probably get rid of it.

[WebKit Commit Bot]

Comment on attachment 318519 [details]
Patch for landing

Clearing flags on attachment: 318519

Committed r220928: <http://trac.webkit.org/changeset/220928>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/33967936>