Bug 175732 - [Cache API] Add support for being loaded responses
Summary: [Cache API] Add support for being loaded responses
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-08-18 12:08 PDT by youenn fablet
Modified: 2017-08-18 18:30 PDT (History)
4 users (show)

See Also:


Attachments
Patch (13.40 KB, patch)
2017-08-18 13:03 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (13.55 KB, patch)
2017-08-18 14:29 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (15.73 KB, patch)
2017-08-18 15:32 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (15.72 KB, patch)
2017-08-18 17:30 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (15.72 KB, patch)
2017-08-18 17:47 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2017-08-18 12:08:34 PDT
[Cache API] Add support for being loaded responses
Comment 1 youenn fablet 2017-08-18 13:03:11 PDT
Created attachment 318532 [details]
Patch
Comment 2 youenn fablet 2017-08-18 14:29:34 PDT
Created attachment 318543 [details]
Patch
Comment 3 Chris Dumez 2017-08-18 15:03:20 PDT
orange bubbles!
Comment 4 youenn fablet 2017-08-18 15:32:16 PDT
Created attachment 318551 [details]
Patch
Comment 5 youenn fablet 2017-08-18 16:09:47 PDT
No more orange bubbles related to this patch apparently...
Comment 6 Chris Dumez 2017-08-18 16:35:15 PDT
Comment on attachment 318551 [details]
Patch

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

r=me with comments.

> Source/WebCore/Modules/fetch/FetchResponse.cpp:137
> +        responseDataCallback(m_response.body().consumer().takeData());

Is it ok to just steal the data from m_response?

> Source/WebCore/Modules/fetch/FetchResponse.cpp:152
> +        responseDataCallback(Exception { TypeError });

Would be nice to pass a second parameter, like a couple of lines above.

> Source/WebCore/Modules/fetch/FetchResponse.cpp:282
> +    if (!isLoading())

I think this should either be an assertion OR it should call the callback before returning. Otherwise, it is confusing from the client's point of view that the callback may or may not get called.
Comment 7 youenn fablet 2017-08-18 17:26:14 PDT
Comment on attachment 318551 [details]
Patch

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

>> Source/WebCore/Modules/fetch/FetchResponse.cpp:137
>> +        responseDataCallback(m_response.body().consumer().takeData());
> 
> Is it ok to just steal the data from m_response?

Yes, this is the principle behind consuming a response.
I'll use consume to make that clearer.

>> Source/WebCore/Modules/fetch/FetchResponse.cpp:152
>> +        responseDataCallback(Exception { TypeError });
> 
> Would be nice to pass a second parameter, like a couple of lines above.

OK

>> Source/WebCore/Modules/fetch/FetchResponse.cpp:282
>> +    if (!isLoading())
> 
> I think this should either be an assertion OR it should call the callback before returning. Otherwise, it is confusing from the client's point of view that the callback may or may not get called.

I'll change to an ASSERT(isLoading()).
Comment 8 youenn fablet 2017-08-18 17:30:14 PDT
Created attachment 318564 [details]
Patch
Comment 9 youenn fablet 2017-08-18 17:47:57 PDT
Created attachment 318566 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2017-08-18 18:29:54 PDT
Comment on attachment 318566 [details]
Patch for landing

Clearing flags on attachment: 318566

Committed r220948: <http://trac.webkit.org/changeset/220948>
Comment 11 WebKit Commit Bot 2017-08-18 18:29:55 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2017-08-18 18:30:54 PDT
<rdar://problem/33974575>