Summary: | [Fetch API] Ensure response cloning works when data is loading | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||||
Component: | WebCore Misc. | Assignee: | youenn fablet <youennf> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | achristensen, cdumez, commit-queue, esprehn+autocc, kondapallykalyan, sam | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 151937 | ||||||||||
Attachments: |
|
Description
youenn fablet
2016-08-24 06:52:45 PDT
Created attachment 286852 [details]
Patch
Created attachment 286971 [details]
Improving test style
Comment on attachment 286971 [details] Improving test style View in context: https://bugs.webkit.org/attachment.cgi?id=286971&action=review > Source/WebCore/Modules/fetch/FetchResponse.cpp:140 > if (m_response.m_readableStreamSource) { > - m_response.m_readableStreamSource->close(); > - m_response.m_readableStreamSource = nullptr; > + // Let's close the stream except if we have data to enqueue. > + if (m_response.m_body.type() != FetchBody::Type::Loaded) { > + m_response.m_readableStreamSource->close(); > + m_response.m_readableStreamSource = nullptr; > + } > } In the case where we don’t close the stream here, what closes the stream? Why not use && instead of nested if statements? (In reply to comment #3) > Comment on attachment 286971 [details] > Improving test style > > View in context: > https://bugs.webkit.org/attachment.cgi?id=286971&action=review > > > Source/WebCore/Modules/fetch/FetchResponse.cpp:140 > > if (m_response.m_readableStreamSource) { > > - m_response.m_readableStreamSource->close(); > > - m_response.m_readableStreamSource = nullptr; > > + // Let's close the stream except if we have data to enqueue. > > + if (m_response.m_body.type() != FetchBody::Type::Loaded) { > > + m_response.m_readableStreamSource->close(); > > + m_response.m_readableStreamSource = nullptr; > > + } > > } > > In the case where we don’t close the stream here, what closes the stream? When type is Loaded, FetchBody has some data, which means the data was not enqueued in the stream. On the first read request, FetchBody::consumeAsStream will enqueue the data and close the stream. I'll update the comment. Overall, there is some complexity added by the idea of enqueuing data only on the first read call. Maybe we should simplify this and enqueue data as soon as the stream is created. > Why not use && instead of nested if statements? Right, I'll fix that. Created attachment 287249 [details]
Patch for landing
Comment on attachment 287249 [details] Patch for landing Clearing flags on attachment: 287249 Committed r205110: <http://trac.webkit.org/changeset/205110> All reviewed patches have been landed. Closing bug. |