Special case support for MHTML archives consisting of a single image.

Special case support for MHTML archives consisting of a single image.

BUG=584866

[bburns, 2016-02-18 00:16:59]

bburns@chromium.org changed reviewers:
+ dimich@chromium.org, mkwst@chromium.org, senorblanco@chromium.org

[bburns, 2016-02-18 00:17:00]

Please review this CL.

Thanks!
--brendan

[Dmitry Titov, 2016-02-18 01:30:05]

Does it fix the bug? It'll need a blink test.

https://codereview.chromium.org/1705903003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/mhtml/MHTMLArchive.cpp (right):

https://codereview.chromium.org/1705903003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/mhtml/MHTMLArchive.cpp:93:
archive->addSubresource(resources[0].get());
Is there a need to add non-recognized mime type as subresource in this case? It
seems your snippet form the bug would just work, right?

[Dmitry Titov, 2016-02-18 01:31:15]

also:

https://codereview.chromium.org/1705903003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/mhtml/MHTMLArchive.cpp (right):

https://codereview.chromium.org/1705903003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/mhtml/MHTMLArchive.cpp:89: if
(MIMETypeRegistry::isSupportedNonImageMIMEType(mimeType)
I am not sure why you are checking for both non-image and image mime types. Is
there a reason to check for non-image ones here?

[Mike West, 2016-02-18 10:48:20]

Why would we create an MHTML archive for a single resource, rather than just
downloading the resource?

[Dmitry Titov, 2016-02-18 21:03:43]

On 2016/02/18 10:48:20, Mike West wrote:
> Why would we create an MHTML archive for a single resource, rather than just
> downloading the resource?

Of course, and it is already possible to save an image - as a separate feature.
However, MHTML may want to handle standalone images as well, for a various
reasons:
- if user selects a "Save page as MHTML" option, they should be able to get what
they expect, an .mhtml file - out of any page (they not necessarily realize it's
just an image)
- Chrome can register for mhtml (multipart/related) and be able to open any page
it saved. If the user saves a page into MHTML archive, they should be able to
expect Chrome to open it, not the other image viewer. Of course, if they saved
an image (via right-click menu for example), they should get an image file and
then expectation of opening it in whatever happens to be an image viewer is
correct.
- MHTML contains "headers" with additional metadata - origin for example. Those
can be useful, within MHTML set of scenarios.

[Dmitry Titov, 2016-02-18 21:03:47]

On 2016/02/18 10:48:20, Mike West wrote:
> Why would we create an MHTML archive for a single resource, rather than just
> downloading the resource?

Of course, and it is already possible to save an image - as a separate feature.
However, MHTML may want to handle standalone images as well, for a various
reasons:
- if user selects a "Save page as MHTML" option, they should be able to get what
they expect, an .mhtml file - out of any page (they not necessarily realize it's
just an image)
- Chrome can register for mhtml (multipart/related) and be able to open any page
it saved. If the user saves a page into MHTML archive, they should be able to
expect Chrome to open it, not the other image viewer. Of course, if they saved
an image (via right-click menu for example), they should get an image file and
then expectation of opening it in whatever happens to be an image viewer is
correct.
- MHTML contains "headers" with additional metadata - origin for example. Those
can be useful, within MHTML set of scenarios.

[Mike West, 2016-02-19 06:54:09]

On 2016/02/18 at 21:03:43, dimich wrote:
> Of course, and it is already possible to save an image - as a separate
feature. However, MHTML may want to handle standalone images as well, for a
various reasons:
> - if user selects a "Save page as MHTML" option, they should be able to get
what they expect, an .mhtml file - out of any page (they not necessarily realize
it's just an image)

I'm looking for this option, and haven't found it. There's a flag that's
apparently been around since 2012... are there plans to enable it?

Basically, I'm wondering whether this code is going to ship (in which case small
improvements like this one make sense), or whether we should remove it entirely.
:)

> - Chrome can register for mhtml (multipart/related) and be able to open any
page it saved. If the user saves a page into MHTML archive, they should be able
to expect Chrome to open it, not the other image viewer. Of course, if they
saved an image (via right-click menu for example), they should get an image file
and then expectation of opening it in whatever happens to be an image viewer is
correct.
> - MHTML contains "headers" with additional metadata - origin for example.
Those can be useful, within MHTML set of scenarios.

Ok. I'm not entirely convinced that this ends up with a better result for users,
but I don't feel strongly enough about it to block the change. As you noted,
however, this needs tests.

[Dmitry Titov, 2016-02-19 18:14:51]

On 2016/02/19 06:54:09, Mike West wrote:
> On 2016/02/18 at 21:03:43, dimich wrote:
> > Of course, and it is already possible to save an image - as a separate
> feature. However, MHTML may want to handle standalone images as well, for a
> various reasons:
> > - if user selects a "Save page as MHTML" option, they should be able to get
> what they expect, an .mhtml file - out of any page (they not necessarily
realize
> it's just an image)
> 
> I'm looking for this option, and haven't found it. There's a flag that's
> apparently been around since 2012... are there plans to enable it?
> 
> Basically, I'm wondering whether this code is going to ship (in which case
small
> improvements like this one make sense), or whether we should remove it
entirely.
> :)


Yep, the reason we are starting to look into those bugs is that MHTML
generator/viewer is a part of "Offline Pages" feature (go/offline-pages) that is
experimental now and is on the path to shipping (perhaps initially in EM). There
will be much more work on upkeep of MHTML soon... We might start owning it.

Offline pages will use MHTML for now as implementation mechanism. It is unclear
if the "Save page as MHTML" will be enabled as it is, it's a somewhat separate
feature.