Make multipart image documents work again.

Make multipart image documents work again.

We used to have generic multipart main resource plumbing, which was removed in https://codereview.chromium.org/16848005. However, we can implement narrow support for just multipage images as the main resource (i.e., mjpeg) with relatively minor modifications to ImageDocument.

BUG=308999
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196101

[Nate Chapin, 2015-05-27 20:45:22]

japhet@chromium.org changed reviewers:
+ mkwst@chromium.org, sigbjornf@opera.com

[Nate Chapin, 2015-05-27 20:45:25]

https://codereview.chromium.org/1154413002/diff/60001/Source/core/fetch/Image...
File Source/core/fetch/ImageResourceTest.cpp (right):

https://codereview.chromium.org/1154413002/diff/60001/Source/core/fetch/Image...
Source/core/fetch/ImageResourceTest.cpp:224:
ASSERT_EQ(client.imageChangedCount(), 2);
Because this test no longer unnecessarily uses multipart, it gets an
imageChanged() callback when data is appended as well as on finish().

https://codereview.chromium.org/1154413002/diff/60001/Source/core/fetch/Resou...
File Source/core/fetch/ResourceLoader.h (right):

https://codereview.chromium.org/1154413002/diff/60001/Source/core/fetch/Resou...
Source/core/fetch/ResourceLoader.h:113: bool m_loadingMultipartContent;
Moved from ImageResource to a more shared location. Also, having "loading" in
the variable name suggests it belongs on ResourceLoader instead of Resource.

https://codereview.chromium.org/1154413002/diff/60001/Source/core/html/ImageD...
File Source/core/html/ImageDocument.cpp (right):

https://codereview.chromium.org/1154413002/diff/60001/Source/core/html/ImageD...
Source/core/html/ImageDocument.cpp:140: if (!document())
Here and in finish(), if createDocumentStructure() is called inside
cachedImage() and the content is multipart, we will finish parsing synchronously
and document() will be null by the time we reach here.

https://codereview.chromium.org/1154413002/diff/60001/Source/core/html/ImageD...
Source/core/html/ImageDocument.cpp:240: finishedParsing();
The stopLoading() call above leaves us in a half-complete state where
finishedParsing() will never get called. Call finishedParsing here to ensure
mjpeg ImageDocuments in iframes work.

https://codereview.chromium.org/1154413002/diff/60001/Source/core/html/ImageD...
Source/core/html/ImageDocument.cpp:411: return loadingMultipartContent ? nullptr
: m_imageElement->cachedImage();
Since multipart images will load their own content, don't let the DocumentParser
add data to it.

[Mike West, 2015-05-28 05:51:54]

LGTM in general, but I'm not sure about the check in Resource.h.

https://codereview.chromium.org/1154413002/diff/60001/Source/core/fetch/Resou...
File Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/1154413002/diff/60001/Source/core/fetch/Resou...
Source/core/fetch/Resource.h:173: bool canBeMultipart() const { return type() ==
Image || type() == MainResource; }
This seems unclear; we don't support multipart for _any_ MainResource, do we?
What happens if we try to load a non-image multipart document?

[Nate Chapin, 2015-05-28 17:44:24]

https://codereview.chromium.org/1154413002/diff/60001/Source/core/fetch/Resou...
File Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/1154413002/diff/60001/Source/core/fetch/Resou...
Source/core/fetch/Resource.h:173: bool canBeMultipart() const { return type() ==
Image || type() == MainResource; }
On 2015/05/28 05:51:54, Mike West wrote:
> This seems unclear; we don't support multipart for _any_ MainResource, do we?
> What happens if we try to load a non-image multipart document?

We fail. I was hoping to convey some level of ambiguity with "can" in the
function name. Maybe leave this inlined with a comment in
ResourceLoader::didReceiveResponse() is clearer?

[Mike West, 2015-05-28 18:26:59]

On 2015/05/28 at 17:44:24, japhet wrote:
> We fail. I was hoping to convey some level of ambiguity with "can" in the
function name. Maybe leave this inlined with a comment in
ResourceLoader::didReceiveResponse() is clearer?

I think that makes more sense, yeah.

[Nate Chapin, 2015-05-28 22:02:17]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2015-05-28 22:02:18]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1154413002/#ps80001
(title: "Revert Resource.h change")

[commit-bot: I haz the power, 2015-05-28 22:02:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1154413002/80001

[commit-bot: I haz the power, 2015-05-28 22:38:48]

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

[commit-bot: I haz the power, 2015-05-28 22:38:50]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6...)

[Nate Chapin, 2015-05-28 23:08:21]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2015-05-28 23:08:22]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1154413002/#ps100001
(title: "Fix excessive clumsiness")

[commit-bot: I haz the power, 2015-05-28 23:08:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1154413002/100001

[commit-bot: I haz the power, 2015-05-29 00:20:40]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196101