[ABANDONED] Move multipart/x-mixed-replace related code to ImageResource

Move multipart/x-mixed-replace related code to ImageResource

 - Remove multipart related code from DocumentLoader and ImageDocument.
 - ImageDocument previously re-fetched the resource, but with this change
   it uses the currently loading resource.
 - Move multipart handling code from Resource to ImageResource.
 - Remove WebURLResponse::isMultipartPayload.

BUG=570608

[yhirano, 2016-02-25 00:49:03]

Description was changed from

==========
Move multipart/x-mixed-replace related code to ImageResource

This change moves multipart related code scattered in core to ImageResouce.

 - Remove multipart related code from DocumentLoader and ImageDocument.
 - ImageDocument previously re-fetched the resource, but with this change
   it uses the currently loading resource.
 - Move multipart handling code from Resource to ImageResource.
 - Remove WebURLResponse::isMultipartPayload.

BUG=570608
==========

to

==========
Move multipart/x-mixed-replace related code to ImageResource

 - Remove multipart related code from DocumentLoader and ImageDocument.
 - ImageDocument previously re-fetched the resource, but with this change
   it uses the currently loading resource.
 - Move multipart handling code from Resource to ImageResource.
 - Remove WebURLResponse::isMultipartPayload.

BUG=570608
==========

[yhirano, 2016-02-25 02:24:12]

yhirano@chromium.org changed reviewers:
+ hiroshige@chromium.org, japhet@chromium.org

[yhirano, 2016-02-25 02:24:13]

This CL changes ImageDocument's behavior a bit: notifyFinished is dispatched
when loading the resource is done, not when loading the first part is done. I
think it is an acceptable change. What do you think?

[hiroshige, 2016-02-25 23:13:05]

On 2016/02/25 02:24:13, yhirano wrote:
> This CL changes ImageDocument's behavior a bit: notifyFinished is dispatched
> when loading the resource is done, not when loading the first part is done. I
> think it is an acceptable change. What do you think?

This CL seems to revert many changes made in
https://codereview.chromium.org/1710733002/.
Perhaps merging two might result fewer changes and the CL might be clearer?
(not sure though)

[hiroshige, 2016-02-26 19:03:35]

https://codereview.chromium.org/1738553002/diff/40001/chrome/renderer/page_lo...
File chrome/renderer/page_load_histograms.cc (left):

https://codereview.chromium.org/1738553002/diff/40001/chrome/renderer/page_lo...
chrome/renderer/page_load_histograms.cc:888: if
(frame->dataSource()->response().isMultipartPayload())
This change and one in metrics_render_frame_observer.cc result in metrics
change, right? We have to add a reviewer related to this later.

https://codereview.chromium.org/1738553002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/1738553002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:369: ASSERT(!handle);
Can we add |ASSERT(!m_multipartParser);|?

Also, in general, can we add ASSERT() to Resource::responseReceived() that
responseReceived() is called only once (in this or existing related CLs, or in a
new separate CL)?

https://codereview.chromium.org/1738553002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ImageResource.h (right):

https://codereview.chromium.org/1738553002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResource.h:100: bool
isMultipartImage() const { return m_multipartParser; }
nit optional: I think this corresponds to loadingMultipartContent(). Do we need
to rename it? (OK to rename, just want to check)

https://codereview.chromium.org/1738553002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/Resource.cpp (left):

https://codereview.chromium.org/1738553002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/Resource.cpp:328: ASSERT(!errorOccurred());
What is the reason for removing ASSERT(!errorOccurred())?

[yhirano, 2016-02-27 02:03:20]

https://codereview.chromium.org/1738553002/diff/40001/chrome/renderer/page_lo...
File chrome/renderer/page_load_histograms.cc (left):

https://codereview.chromium.org/1738553002/diff/40001/chrome/renderer/page_lo...
chrome/renderer/page_load_histograms.cc:888: if
(frame->dataSource()->response().isMultipartPayload())
On 2016/02/26 19:03:35, hiroshige wrote:
> This change and one in metrics_render_frame_observer.cc result in metrics
> change, right? We have to add a reviewer related to this later.

They are for ignoring pseudo responses in multipart, so I think it's no problem
and desirable to delete them.

https://codereview.chromium.org/1738553002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/1738553002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:369: ASSERT(!handle);
On 2016/02/26 19:03:35, hiroshige wrote:
> Can we add |ASSERT(!m_multipartParser);|?
> 
> Also, in general, can we add ASSERT() to Resource::responseReceived() that
> responseReceived() is called only once (in this or existing related CLs, or in
a
> new separate CL)?

Added an assertion for multipartParser.

Not sure for the receive-response-only-once assertion.

https://codereview.chromium.org/1738553002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/ImageResource.h (right):

https://codereview.chromium.org/1738553002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/ImageResource.h:100: bool
isMultipartImage() const { return m_multipartParser; }
On 2016/02/26 19:03:35, hiroshige wrote:
> nit optional: I think this corresponds to loadingMultipartContent(). Do we
need
> to rename it? (OK to rename, just want to check)

The original name contained "loading" and the implementation checked it. This
one does not.

https://codereview.chromium.org/1738553002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/Resource.cpp (left):

https://codereview.chromium.org/1738553002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/Resource.cpp:328: ASSERT(!errorOccurred());
On 2016/02/26 19:03:35, hiroshige wrote:
> What is the reason for removing ASSERT(!errorOccurred())?

It's possible that m_status is modified in ImageResource::finish. We call
Resource::finish in it.

[yhirano, 2016-03-01 00:01:57]

Closing the issue because it's merged to
https://codereview.chromium.org/1710733002/.