Move multipart resource handling to core/fetch (2/2)

Move multipart resource handling to core/fetch (2/2)

Currently a multipart/x-mixed-replace response is parsed in the
content layer and dispatched to clients in blink. It is problematic
because the parser calls callbacks in a strange way and special
handling code scatters from core/html to content/child.

This change removes multipart_response_delegate and starts using
MultipartImageResourceParser. Also, this change removes
multipart/x-mixed-replace related code from ImageDocument. ImageDocument
previously re-fetched the resource, but with this change it uses the
currently loading resource.

BUG=570608
Committed: https://crrev.com/998e1a037a811ec74dbcca021503ddf3f4b86d60
Cr-Commit-Position: refs/heads/master@{#380523}

[yhirano, 2016-02-19 00:24:36]

Description was changed from

==========
multipart cleanup 2 / wip

BUG=
==========

to

==========
Move multipart resource handling to core/fetch (2/2)

Currently a multipart/x-mixed-replace response is parsed in the
content layer and dispatched to clients in blink. It is problematic
because the parser calls callbacks in a strange way and special
handling code scatters from core/html to content/child.

This change removes multipart_response_delegate and starts using
MultipartImageResourceParser. The main focus of this change is
content/child and core/fetch. Depending modules will be modified
in subsequent CLs.

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

[yhirano, 2016-02-19 21:02:24]

Patchset #1 (id:1) has been deleted

[yhirano, 2016-02-19 21:02:36]

Patchset #1 (id:20001) has been deleted

[yhirano, 2016-02-19 21:02:50]

Patchset #3 (id:80001) has been deleted

[yhirano, 2016-02-19 21:04:05]

Patchset #1 (id:40001) has been deleted

[yhirano, 2016-02-20 01:10:49]

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

[yhirano, 2016-02-20 01:10:50]

+hiroshige@, japhet@

[hiroshige, 2016-02-22 06:19:30]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-22 06:20:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1710733002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1710733002/180001

[commit-bot: I haz the power, 2016-02-22 07:48:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-22 07:48:43]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2016-02-25 21:49:29]

https://codereview.chromium.org/1710733002/diff/200001/content/child/multipar...
File content/child/multipart_response_delegate_unittest.cc (left):

https://codereview.chromium.org/1710733002/diff/200001/content/child/multipar...
content/child/multipart_response_delegate_unittest.cc:137: // ParseHeaders tests
The tests in Lines 137--184 are missing in the new Blink-side tests in
https://codereview.chromium.org/1693183002/?

[Nate Chapin, 2016-02-25 22:07:53]

https://codereview.chromium.org/1710733002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/1710733002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:476: // If there's no
boundary, just handle the request normally.  In the gecko
This comment looks like very outdated.

https://codereview.chromium.org/1710733002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:551:
m_loader->fetcher()->subresourceLoaderFinishedLoadingOnePart(m_loader);
Instead of Exposing ResourceLoader::fetcher(), can we just do this work in
didFinishLoadingOnePart, passing in a boolean isMultipart to
didFinishLoadingOnePart if needed?

https://codereview.chromium.org/1710733002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:557: void
Resource::didReceiveData(const char* bytes, size_t size)
I agree with hiroshige's comment on https://codereview.chromium.org/1738553002,
these names should mention multipart explicitly. It's confusing to have a name
as general as didReceiveData() and have it then ASSERT(isMultipartImage()).

https://codereview.chromium.org/1710733002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/1710733002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.h:63: class CORE_EXPORT Resource :
public RefCountedWillBeGarbageCollectedFinalized<Resource>, public
MultipartImageResourceParser::Client {
We should at least take a look at weather we can make just ImageResource a
MultipartImageResourceParser::Client.

[hiroshige, 2016-02-26 19:02:13]

https://codereview.chromium.org/1710733002/diff/200001/content/child/web_url_...
File content/child/web_url_loader_impl_unittest.cc (left):

https://codereview.chromium.org/1710733002/diff/200001/content/child/web_url_...
content/child/web_url_loader_impl_unittest.cc:160:
EXPECT_TRUE(expect_multipart_response_ || !did_receive_response_);
Can this be |EXPECT_FALSE(did_receive_response_);|?

[hiroshige, 2016-02-26 19:06:19]

Actually, I'm merging two of your CLs:
- https://codereview.chromium.org/1710733002/ (this CL)
- https://codereview.chromium.org/1738553002/
locally at https://codereview.chromium.org/1740893003/ and reviewing on it.
If we merge two, we no longer need to change RawResource etc. or make Resource
to be a MultipartImageResourceParser::Client subclass, and only ImageResource
becomes a MultipartImageResourceParser::Client subclass.

[yhirano, 2016-02-26 22:23:39]

On 2016/02/26 19:06:19, hiroshige wrote:
> Actually, I'm merging two of your CLs:
> - https://codereview.chromium.org/1710733002/ (this CL)
> - https://codereview.chromium.org/1738553002/
> locally at https://codereview.chromium.org/1740893003/ and reviewing on it.
> If we merge two, we no longer need to change RawResource etc. or make Resource
> to be a MultipartImageResourceParser::Client subclass, and only ImageResource
> becomes a MultipartImageResourceParser::Client subclass.

japhet@, what do you think? I generally like smaller CLs, but if both of you
think the combined CL is easier to review, I will merge them.

[yhirano, 2016-02-26 23:07:42]

https://codereview.chromium.org/1710733002/diff/200001/content/child/multipar...
File content/child/multipart_response_delegate_unittest.cc (left):

https://codereview.chromium.org/1710733002/diff/200001/content/child/multipar...
content/child/multipart_response_delegate_unittest.cc:137: // ParseHeaders tests
On 2016/02/25 21:49:29, hiroshige wrote:
> The tests in Lines 137--184 are missing in the new Blink-side tests in
> https://codereview.chromium.org/1693183002/

Added in the other CL: https://codereview.chromium.org/1693183002/#msg25

[yhirano, 2016-02-27 01:27:45]

https://codereview.chromium.org/1710733002/diff/200001/content/child/web_url_...
File content/child/web_url_loader_impl_unittest.cc (left):

https://codereview.chromium.org/1710733002/diff/200001/content/child/web_url_...
content/child/web_url_loader_impl_unittest.cc:160:
EXPECT_TRUE(expect_multipart_response_ || !did_receive_response_);
On 2016/02/26 19:02:12, hiroshige wrote:
> Can this be |EXPECT_FALSE(did_receive_response_);|?

Done.

https://codereview.chromium.org/1710733002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/1710733002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:476: // If there's no
boundary, just handle the request normally.  In the gecko
On 2016/02/25 22:07:53, Nate Chapin wrote:
> This comment looks like very outdated.
Deleted.

https://codereview.chromium.org/1710733002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:551:
m_loader->fetcher()->subresourceLoaderFinishedLoadingOnePart(m_loader);
On 2016/02/25 22:07:53, Nate Chapin wrote:
> Instead of Exposing ResourceLoader::fetcher(), can we just do this work in
> didFinishLoadingOnePart, passing in a boolean isMultipart to
> didFinishLoadingOnePart if needed?

Done.

https://codereview.chromium.org/1710733002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:557: void
Resource::didReceiveData(const char* bytes, size_t size)
On 2016/02/25 22:07:53, Nate Chapin wrote:
> I agree with hiroshige's comment on
https://codereview.chromium.org/1738553002,
> these names should mention multipart explicitly. It's confusing to have a name
> as general as didReceiveData() and have it then ASSERT(isMultipartImage()).

Done.

https://codereview.chromium.org/1710733002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/1710733002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.h:63: class CORE_EXPORT Resource :
public RefCountedWillBeGarbageCollectedFinalized<Resource>, public
MultipartImageResourceParser::Client {
On 2016/02/25 22:07:53, Nate Chapin wrote:
> We should at least take a look at weather we can make just ImageResource a
> MultipartImageResourceParser::Client.

It's not possible at this moment because ImageDocument wants the resource
parsed.  The next CL (https://codereview.chromium.org/1738553002/) moves this
interface to ImageResource.

[yhirano, 2016-02-29 23:53:29]

Description was changed from

==========
Move multipart resource handling to core/fetch (2/2)

Currently a multipart/x-mixed-replace response is parsed in the
content layer and dispatched to clients in blink. It is problematic
because the parser calls callbacks in a strange way and special
handling code scatters from core/html to content/child.

This change removes multipart_response_delegate and starts using
MultipartImageResourceParser. The main focus of this change is
content/child and core/fetch. Depending modules will be modified
in subsequent CLs.

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

to

==========
Move multipart resource handling to core/fetch (2/2)

Currently a multipart/x-mixed-replace response is parsed in the
content layer and dispatched to clients in blink. It is problematic
because the parser calls callbacks in a strange way and special
handling code scatters from core/html to content/child.

This change removes multipart_response_delegate and starts using
MultipartImageResourceParser. Also, this change removes
multipart/x-mixed-replace related from ImageDocument. ImageDocument previously
re-fetched the
resource, but with this change it uses the currently loading resource.
Also, this change removes WebURLResponse::isMultipartPayload.

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

[yhirano, 2016-02-29 23:53:51]

Description was changed from

==========
Move multipart resource handling to core/fetch (2/2)

Currently a multipart/x-mixed-replace response is parsed in the
content layer and dispatched to clients in blink. It is problematic
because the parser calls callbacks in a strange way and special
handling code scatters from core/html to content/child.

This change removes multipart_response_delegate and starts using
MultipartImageResourceParser. Also, this change removes
multipart/x-mixed-replace related from ImageDocument. ImageDocument previously
re-fetched the
resource, but with this change it uses the currently loading resource.
Also, this change removes WebURLResponse::isMultipartPayload.

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

to

==========
Move multipart resource handling to core/fetch (2/2)

Currently a multipart/x-mixed-replace response is parsed in the
content layer and dispatched to clients in blink. It is problematic
because the parser calls callbacks in a strange way and special
handling code scatters from core/html to content/child.

This change removes multipart_response_delegate and starts using
MultipartImageResourceParser. Also, this change removes
multipart/x-mixed-replace related from ImageDocument. ImageDocument
previously re-fetched the resource, but with this change it uses the
currently loading resource.
Also, this change removes WebURLResponse::isMultipartPayload.

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

[yhirano, 2016-02-29 23:54:14]

Description was changed from

==========
Move multipart resource handling to core/fetch (2/2)

Currently a multipart/x-mixed-replace response is parsed in the
content layer and dispatched to clients in blink. It is problematic
because the parser calls callbacks in a strange way and special
handling code scatters from core/html to content/child.

This change removes multipart_response_delegate and starts using
MultipartImageResourceParser. Also, this change removes
multipart/x-mixed-replace related from ImageDocument. ImageDocument
previously re-fetched the resource, but with this change it uses the
currently loading resource.
Also, this change removes WebURLResponse::isMultipartPayload.

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

to

==========
Move multipart resource handling to core/fetch (2/2)

Currently a multipart/x-mixed-replace response is parsed in the
content layer and dispatched to clients in blink. It is problematic
because the parser calls callbacks in a strange way and special
handling code scatters from core/html to content/child.

This change removes multipart_response_delegate and starts using
MultipartImageResourceParser. Also, this change removes
multipart/x-mixed-replace related from ImageDocument. ImageDocument
previously re-fetched the resource, but with this change it uses the
currently loading resource.

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

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

As talked offline, I merged https://codereview.chromium.org/1738553002/ to this
CL.

[hiroshige, 2016-03-01 01:12:12]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-01 01:13:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1710733002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1710733002/300001

[commit-bot: I haz the power, 2016-03-01 01:26:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-01 01:26:36]

Dry run: Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[hiroshige, 2016-03-01 01:34:04]

bots are failing due to compile errors.

[yhirano, 2016-03-01 02:47:28]

The CQ bit was checked by yhirano@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-01 02:48:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1710733002/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1710733002/320001

[commit-bot: I haz the power, 2016-03-01 04:10:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-01 04:10:38]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-03-01 17:29:30]

On 2016/03/01 01:34:04, hiroshige wrote:
> bots are failing due to compile errors.

Sorry for the mess, fixed.

[Nate Chapin, 2016-03-01 19:02:15]

:D

https://codereview.chromium.org/1710733002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/1710733002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:483: checkNotify();
I think this triggers multiple calls of ResourceClient::notifyFinished?

https://codereview.chromium.org/1710733002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.h (right):

https://codereview.chromium.org/1710733002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.h:100: bool
isMultipartImage() const { return m_multipartParser; }
This is only used for ASSERTs in ImageResource. Maybe remove this and ASSERT
m_multipartParser directly.

https://codereview.chromium.org/1710733002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right):

https://codereview.chromium.org/1710733002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:204: ASSERT(m_state !=
Terminated);
This means we both ASSERT and early-exit for this case. Can we do just one or
the other?

https://codereview.chromium.org/1710733002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/ImageDocument.cpp (right):

https://codereview.chromium.org/1710733002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/ImageDocument.cpp:231:
m_imageElement->cachedImage()->responseReceived(loader()->response(), nullptr);
Why is this necessary?

[yhirano, 2016-03-01 19:43:23]

https://codereview.chromium.org/1710733002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/1710733002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:483: checkNotify();
On 2016/03/01 19:02:15, Nate Chapin wrote:
> I think this triggers multiple calls of ResourceClient::notifyFinished?

Yes, and it's what the existing code is doing. Are you suggesting to fix it in
this CL?

https://codereview.chromium.org/1710733002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.h (right):

https://codereview.chromium.org/1710733002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.h:100: bool
isMultipartImage() const { return m_multipartParser; }
On 2016/03/01 19:02:15, Nate Chapin wrote:
> This is only used for ASSERTs in ImageResource. Maybe remove this and ASSERT
> m_multipartParser directly.

Done.

https://codereview.chromium.org/1710733002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right):

https://codereview.chromium.org/1710733002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:204: ASSERT(m_state !=
Terminated);
On 2016/03/01 19:02:15, Nate Chapin wrote:
> This means we both ASSERT and early-exit for this case. Can we do just one or
> the other?

Calling subresourceLoaderFinishedLoadingOnePar may change the state.

https://codereview.chromium.org/1710733002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/ImageDocument.cpp (right):

https://codereview.chromium.org/1710733002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/ImageDocument.cpp:231:
m_imageElement->cachedImage()->responseReceived(loader()->response(), nullptr);
On 2016/03/01 19:02:15, Nate Chapin wrote:
> Why is this necessary?

The cachedImage response is ignorant of the mime type without this statement.

[Nate Chapin, 2016-03-01 19:56:46]

https://codereview.chromium.org/1710733002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/1710733002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:483: checkNotify();
On 2016/03/01 19:43:23, yhirano wrote:
> On 2016/03/01 19:02:15, Nate Chapin wrote:
> > I think this triggers multiple calls of ResourceClient::notifyFinished?
> 
> Yes, and it's what the existing code is doing. Are you suggesting to fix it in
> this CL?

Oops. Didn't know the existing code did that :(

That sounds like a bug, but not one that should block this refactor.

https://codereview.chromium.org/1710733002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right):

https://codereview.chromium.org/1710733002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:204: ASSERT(m_state !=
Terminated);
On 2016/03/01 19:43:23, yhirano wrote:
> On 2016/03/01 19:02:15, Nate Chapin wrote:
> > This means we both ASSERT and early-exit for this case. Can we do just one
or
> > the other?
> 
> Calling subresourceLoaderFinishedLoadingOnePar may change the state.

...of course. Never mind :)

https://codereview.chromium.org/1710733002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/ImageDocument.cpp (right):

https://codereview.chromium.org/1710733002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/ImageDocument.cpp:231:
m_imageElement->cachedImage()->responseReceived(loader()->response(), nullptr);
On 2016/03/01 19:43:23, yhirano wrote:
> On 2016/03/01 19:02:15, Nate Chapin wrote:
> > Why is this necessary?
> 
> The cachedImage response is ignorant of the mime type without this statement.

Ok.

[hiroshige, 2016-03-01 22:34:24]

https://codereview.chromium.org/1710733002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/1710733002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:252: ASSERT(!errorOccurred());
This assertion fails on mac bots in layout test
http/tests/multipart/stop-crash.html (seems flaky though), probably due to this
CL:
https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel...

[yhirano, 2016-03-02 00:32:10]

https://codereview.chromium.org/1710733002/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/1710733002/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.cpp:252: ASSERT(!errorOccurred());
On 2016/03/01 22:34:24, hiroshige wrote:
> This assertion fails on mac bots in layout test
> http/tests/multipart/stop-crash.html (seems flaky though), probably due to
this
> CL:
>
https://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel...

Nice catch! Fixed.

[yhirano, 2016-03-02 00:38:11]

Description was changed from

==========
Move multipart resource handling to core/fetch (2/2)

Currently a multipart/x-mixed-replace response is parsed in the
content layer and dispatched to clients in blink. It is problematic
because the parser calls callbacks in a strange way and special
handling code scatters from core/html to content/child.

This change removes multipart_response_delegate and starts using
MultipartImageResourceParser. Also, this change removes
multipart/x-mixed-replace related from ImageDocument. ImageDocument
previously re-fetched the resource, but with this change it uses the
currently loading resource.

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

to

==========
Move multipart resource handling to core/fetch (2/2)

Currently a multipart/x-mixed-replace response is parsed in the
content layer and dispatched to clients in blink. It is problematic
because the parser calls callbacks in a strange way and special
handling code scatters from core/html to content/child.

This change removes multipart_response_delegate and starts using
MultipartImageResourceParser. Also, this change removes
multipart/x-mixed-replace related code from ImageDocument. ImageDocument
previously re-fetched the resource, but with this change it uses the
currently loading resource.

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

[yhirano, 2016-03-02 22:40:50]

yhirano@chromium.org changed reviewers:
+ torne@chromium.org

[yhirano, 2016-03-02 22:40:51]

+torne@ for tools/copyright_scanner/third_party_files_whitelist.txt.

[Torne, 2016-03-03 17:03:07]

https://codereview.chromium.org/1710733002/diff/380001/tools/copyright_scanne...
File tools/copyright_scanner/third_party_files_whitelist.txt (left):

https://codereview.chromium.org/1710733002/diff/380001/tools/copyright_scanne...
tools/copyright_scanner/third_party_files_whitelist.txt:247:
content/child/multipart_response_delegate.h
This is removed because the contents of this file moved to third_party/WebKit in
the previous CL, right? (and thus isn't being scanned for copyrights any more
because it's in third_party already). If so then this change to the whitelist
LGTM, but see my comment on the other CL about me not being a lawyer and not
knowing if it's okay to drop this code into the webkit dir.

[yhirano, 2016-03-04 17:40:10]

yhirano@chromium.org changed reviewers:
+ tsepez@chromium.org

[Tom Sepez, 2016-03-04 17:51:10]

So the guts of the parser remain the same, and it is still only called in a
sandboxed child process?  If so, LGTM.

https://codereview.chromium.org/1710733002/diff/380001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1710733002/diff/380001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4485: (response.httpStatusCode() != 404);
nit: overparenthesized.

[hiroshige, 2016-03-04 18:10:30]

non-owner lgtm.

[yhirano, 2016-03-07 19:57:39]

yhirano@chromium.org changed reviewers:
+ csharrison@chromium.org, jochen@chromium.org

[yhirano, 2016-03-07 19:57:41]

+jochen@ for content/ and chrome/.
+csharrison@ for components/page_load_metrics/.

[Charlie Harrison, 2016-03-07 20:07:05]

https://codereview.chromium.org/1710733002/diff/380001/components/page_load_m...
File components/page_load_metrics/renderer/metrics_render_frame_observer.cc
(left):

https://codereview.chromium.org/1710733002/diff/380001/components/page_load_m...
components/page_load_metrics/renderer/metrics_render_frame_observer.cc:90: //
Ignore multipart responses (e.g. MHTML).
Can you explain why this check isn't needed (along with the check in
page_load_histograms)?

[yhirano, 2016-03-07 20:18:27]

https://codereview.chromium.org/1710733002/diff/380001/components/page_load_m...
File components/page_load_metrics/renderer/metrics_render_frame_observer.cc
(left):

https://codereview.chromium.org/1710733002/diff/380001/components/page_load_m...
components/page_load_metrics/renderer/metrics_render_frame_observer.cc:90: //
Ignore multipart responses (e.g. MHTML).
On 2016/03/07 20:07:05, csharrison wrote:
> Can you explain why this check isn't needed (along with the check in
> page_load_histograms)?

Previously isMultipartPayload is set only for multipart/x-replace-image
responses[1] and I'm removing the flag now. IIUC MHTML is not related to this
flag.

For multipart/x-replace-image responses the function should return false based
on the mime type as implemented below.

1:
https://code.google.com/p/chromium/codesearch#chromium/src/content/child/mult...

[Charlie Harrison, 2016-03-07 20:24:14]

lgtm thanks for the explanation. cc bmcquade@ fyi.

[jochen (gone - plz use gerrit), 2016-03-08 15:52:29]

lgtm

[yhirano, 2016-03-08 17:30:28]

Nate, do you have any other comments?

[Nate Chapin, 2016-03-08 17:57:52]

LGTM, just a couple of little details I want to double check.

https://codereview.chromium.org/1710733002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/1710733002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:346:
m_multipartParser->finish();
Should we clear the MultipartParser after calling finish()? What about cancel()?

https://codereview.chromium.org/1710733002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right):

https://codereview.chromium.org/1710733002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:205: if (m_state !=
Finishing) {
This will need to be rebased.

Also, can this function ever be called when state is Finishing? Is this when
there is only one part?

https://codereview.chromium.org/1710733002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:208:
m_fetcher->subresourceLoaderFinishedLoadingOnePart(this);
Is it intentional to call this for non-multipart loads?

[yhirano, 2016-03-08 18:40:42]

https://codereview.chromium.org/1710733002/diff/380001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1710733002/diff/380001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4485: (response.httpStatusCode() != 404);
On 2016/03/04 17:51:10, Tom Sepez wrote:
> nit: overparenthesized.

Done.

https://codereview.chromium.org/1710733002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/1710733002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:346:
m_multipartParser->finish();
On 2016/03/08 17:57:52, Nate Chapin wrote:
> Should we clear the MultipartParser after calling finish()? What about
cancel()?

appendData and finish do nothing when m_sawLastBoundary is set. I made it
explicit.

appendData and finish should not be called after cancel() is called. I added an
assertion.

https://codereview.chromium.org/1710733002/diff/380001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (right):

https://codereview.chromium.org/1710733002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:205: if (m_state !=
Finishing) {
On 2016/03/08 17:57:52, Nate Chapin wrote:
> This will need to be rebased.
> 
> Also, can this function ever be called when state is Finishing? Is this when
> there is only one part?

Done.

isFinishing() returns true when called from didFinishLoading.

https://codereview.chromium.org/1710733002/diff/380001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:208:
m_fetcher->subresourceLoaderFinishedLoadingOnePart(this);
On 2016/03/08 17:57:52, Nate Chapin wrote:
> Is it intentional to call this for non-multipart loads?

The assumption here is that
 isFinishing() == !(loading a multipart image resource).

[yhirano, 2016-03-10 21:30:45]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2016-03-10 21:30:46]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
torne@chromium.org, hiroshige@chromium.org, csharrison@chromium.org,
tsepez@chromium.org, japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/1710733002/#ps480001
(title: " ")

[commit-bot: I haz the power, 2016-03-10 21:31:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1710733002/480001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1710733002/480001

[commit-bot: I haz the power, 2016-03-10 21:36:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-10 21:36:33]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yhirano, 2016-03-10 21:55:19]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2016-03-10 21:55:20]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
torne@chromium.org, hiroshige@chromium.org, csharrison@chromium.org,
tsepez@chromium.org, japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/1710733002/#ps500001
(title: " ")

[commit-bot: I haz the power, 2016-03-10 21:55:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1710733002/500001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1710733002/500001

[commit-bot: I haz the power, 2016-03-10 23:14:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-10 23:14:38]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yhirano, 2016-03-10 23:24:32]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2016-03-10 23:25:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1710733002/500001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1710733002/500001

[commit-bot: I haz the power, 2016-03-11 00:14:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-11 00:14:48]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hiroshige, 2016-03-11 00:22:55]

The CQ bit was checked by hiroshige@chromium.org

[commit-bot: I haz the power, 2016-03-11 00:23:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1710733002/500001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1710733002/500001

[commit-bot: I haz the power, 2016-03-11 01:02:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-11 01:02:46]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[yhirano, 2016-03-11 03:47:12]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2016-03-11 03:47:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1710733002/500001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1710733002/500001

[commit-bot: I haz the power, 2016-03-11 05:21:30]

Description was changed from

==========
Move multipart resource handling to core/fetch (2/2)

Currently a multipart/x-mixed-replace response is parsed in the
content layer and dispatched to clients in blink. It is problematic
because the parser calls callbacks in a strange way and special
handling code scatters from core/html to content/child.

This change removes multipart_response_delegate and starts using
MultipartImageResourceParser. Also, this change removes
multipart/x-mixed-replace related code from ImageDocument. ImageDocument
previously re-fetched the resource, but with this change it uses the
currently loading resource.

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

to

==========
Move multipart resource handling to core/fetch (2/2)

Currently a multipart/x-mixed-replace response is parsed in the
content layer and dispatched to clients in blink. It is problematic
because the parser calls callbacks in a strange way and special
handling code scatters from core/html to content/child.

This change removes multipart_response_delegate and starts using
MultipartImageResourceParser. Also, this change removes
multipart/x-mixed-replace related code from ImageDocument. ImageDocument
previously re-fetched the resource, but with this change it uses the
currently loading resource.

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

[commit-bot: I haz the power, 2016-03-11 05:21:33]

Committed patchset #22 (id:500001)

[commit-bot: I haz the power, 2016-03-11 05:23:40]

Description was changed from

==========
Move multipart resource handling to core/fetch (2/2)

Currently a multipart/x-mixed-replace response is parsed in the
content layer and dispatched to clients in blink. It is problematic
because the parser calls callbacks in a strange way and special
handling code scatters from core/html to content/child.

This change removes multipart_response_delegate and starts using
MultipartImageResourceParser. Also, this change removes
multipart/x-mixed-replace related code from ImageDocument. ImageDocument
previously re-fetched the resource, but with this change it uses the
currently loading resource.

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

to

==========
Move multipart resource handling to core/fetch (2/2)

Currently a multipart/x-mixed-replace response is parsed in the
content layer and dispatched to clients in blink. It is problematic
because the parser calls callbacks in a strange way and special
handling code scatters from core/html to content/child.

This change removes multipart_response_delegate and starts using
MultipartImageResourceParser. Also, this change removes
multipart/x-mixed-replace related code from ImageDocument. ImageDocument
previously re-fetched the resource, but with this change it uses the
currently loading resource.

BUG=570608

Committed: https://crrev.com/998e1a037a811ec74dbcca021503ddf3f4b86d60
Cr-Commit-Position: refs/heads/master@{#380523}
==========

[commit-bot: I haz the power, 2016-03-11 05:23:44]

Patchset 22 (id:??) landed as
https://crrev.com/998e1a037a811ec74dbcca021503ddf3f4b86d60
Cr-Commit-Position: refs/heads/master@{#380523}