[webkit-reviews] review granted: [Bug 175732] [Cache API] Add support for being loaded responses : [Attachment 318551] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted youenn fablet <youennf at gmail.com>'s
request for review:
Bug 175732: [Cache API] Add support for being loaded responses
https://bugs.webkit.org/show_bug.cgi?id=175732

Attachment 318551: Patch

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




--- Comment #6 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 318551
  --> https://bugs.webkit.org/attachment.cgi?id=318551
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.