Add SVG as a picture supported mime type

Add SVG as a picture supported mime type

Add a method that checks a type string against the list of supported images as well as SVG.

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

[pdr., 2014-07-29 20:10:15]

What happens if you just add image/svg+xml to supported_image_types in
mime_util.cc? The fact that it's missing feels like a mistake.

[Yoav Weiss, 2014-07-29 20:20:01]

On 2014/07/29 20:10:15, pdr wrote:
> What happens if you just add image/svg+xml to supported_image_types in
> mime_util.cc? The fact that it's missing feels like a mistake.

I started out by trying that. The image is for some reason broken in that case,
so I assumed there's a good reason it is not there.

[esprehn, 2014-07-30 05:18:41]

On 2014/07/29 at 20:20:01, yoav wrote:
> On 2014/07/29 20:10:15, pdr wrote:
> > What happens if you just add image/svg+xml to supported_image_types in
> > mime_util.cc? The fact that it's missing feels like a mistake.
> 
> I started out by trying that. The image is for some reason broken in that
case, so I assumed there's a good reason it is not there.

Please don't add hacks like this.

[esprehn, 2014-07-30 05:19:08]

abarth@ Any idea why it might be missing from that list?

[abarth-chromium, 2014-07-30 06:11:45]

Looks like isSupportedImageResourceMIMEType gets used by Image::supportsType,
which gets used by DOMImplementation::createDocument to create a MediaDocument
instead of an SVGDocument.  Probably we need to re-order the tests in
DOMImplementation::createDocument.

[Yoav Weiss, 2014-08-05 11:55:17]

On 2014/07/30 06:11:45, abarth wrote:
> Looks like isSupportedImageResourceMIMEType gets used by Image::supportsType,
> which gets used by DOMImplementation::createDocument to create a MediaDocument
> instead of an SVGDocument.  Probably we need to re-order the tests in
> DOMImplementation::createDocument.

I've tried re-ordering the tests in createDocument, and various tests broke.

so, an alternative route to resolve this:
* Rename the current MimeUtil::IsSupportedImageMimeType to be
IsSupportedNonSVGMimeType, use it from
SimpleWebMimeRegistryImpl::supportsImageMimeType (that should be renamed to
supportsNonSVGImageMimeType)
* Rename & change platform/MimeTypeRegistry::isSupportedImageMimeType similarly
* Add a new method MimeUtil::IsSupportedImageMimeType that would covers both all
image types including SVG (which would be implemented similarly to
MimeUtil::IsSupportedMimeType, without adding any extra type lists.
* Create new method on SimpleWebMimeRegistryImpl and MimeTypeRegistry that would
use the new method.

Does that sound like an acceptable way to resolve this? (the reason I'm not just
uploading a CL for review, is that it'd require multiple CLs to get this done)

[Yoav Weiss, 2014-08-08 17:10:38]

After further thought, I've uploaded what's the start of yet another alternative
approach:
* The above CL, which adds WebMimeRegistry::supportsImagePrefixedMIMEType, which
will cover both supported binary image types, as well as SVG.
 - Currently it's added as a hack
* The next CL, will override this func on the implementation side, as part of
SimpleWebMimeRegistryImpl, as well as add a matching func to MimeUtil (as
described above)
* Then, a CL that turns current hacked
WebMimeRegistry::supportsImagePrefixedMIMEType into a pure virtual.

abarth and esprehn - let me know if this approach looks better.

Thanks! :)

[abarth-chromium, 2014-08-09 00:29:44]

I'm sorry, but I don't have the bandwidth to review this CL.

[Yoav Weiss, 2014-08-11 15:19:12]

On 2014/08/09 00:29:44, abarth wrote:
> I'm sorry, but I don't have the bandwidth to review this CL.

Esprehn, will you be able to review this?

[esprehn, 2014-08-11 21:22:21]

Why aren't we just fixing the net:: library that contains the real table of
mimes? Specifically all impls of this just call into net/base/mime_util.cc.

https://codereview.chromium.org/430583002/diff/20001/public/platform/WebMimeR...
File public/platform/WebMimeRegistry.h (right):

https://codereview.chromium.org/430583002/diff/20001/public/platform/WebMimeR...
public/platform/WebMimeRegistry.h:48: return (supportsImageMIMEType(mimeType) ||
mimeType == blink::WebString("image/svg+xml")) ?
This doesn't seem like the right layer for this. WebMimeRegistry is currently
pure virtual and you're changing that. Just change the implementor of it.

[Yoav Weiss, 2014-08-11 21:29:15]

On 2014/08/11 21:22:21, esprehn wrote:
> Why aren't we just fixing the net:: library that contains the real table of
> mimes? Specifically all impls of this just call into net/base/mime_util.cc.

I 100% agree that we should fix it there, but we can't just add it to the image
mime type list, since that breaks DOMImplementation::createDocument (which
creates the wrong document type for SVGs).

So, I want to add an extra method to SimpleWebMimeRegistryImpl, which requires
adding one to WebMimeRegistry that it implements.
Since the interface is on the Blink side, and the impl on the Chrome side, I
thought that the best way to add that would be to add the (virtual but not pure)
method to the Blink side, wait for a roll, override it on the Chrome side
(moving the actual implementation into net/base/mime_util), and then make that
method pure virtual (and bringing back WebMimeRegistry to a pure virtual class)

Does that make sense?

> 
>
https://codereview.chromium.org/430583002/diff/20001/public/platform/WebMimeR...
> File public/platform/WebMimeRegistry.h (right):
> 
>
https://codereview.chromium.org/430583002/diff/20001/public/platform/WebMimeR...
> public/platform/WebMimeRegistry.h:48: return (supportsImageMIMEType(mimeType)
||
> mimeType == blink::WebString("image/svg+xml")) ?
> This doesn't seem like the right layer for this. WebMimeRegistry is currently
> pure virtual and you're changing that. Just change the implementor of it.

[esprehn, 2014-08-13 22:19:09]

abarth says we usually add the override to chromium first and then add the pure
virtual one here later.

[Yoav Weiss, 2014-08-14 09:37:59]

On 2014/08/13 22:19:09, esprehn wrote:
> abarth says we usually add the override to chromium first and then add the
pure
> virtual one here later.

Thanks. I've uploaded https://codereview.chromium.org/471913002/ and changed the
current's patch approach.

[esprehn, 2014-08-26 01:10:26]

lgtm

[Yoav Weiss, 2014-08-26 03:35:02]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2014-08-26 03:35:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yoav@yoav.ws/430583002/40001

[commit-bot: I haz the power, 2014-08-26 03:37:53]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

[commit-bot: I haz the power, 2014-08-26 03:41:21]

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

[commit-bot: I haz the power, 2014-08-26 03:41:23]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

[jamesr, 2014-08-26 17:10:24]

jamesr@chromium.org changed reviewers:
+ jamesr@chromium.org

[jamesr, 2014-08-26 17:10:24]

public/platform lgtm

[Yoav Weiss, 2014-08-26 17:23:25]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2014-08-26 17:24:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yoav@yoav.ws/430583002/40001

[commit-bot: I haz the power, 2014-08-26 17:27:49]

Committed patchset #3 (40001) as 180912