Added a method to detect all supported "image/*" MIME types

Added a method to detect all supported "image/*" MIME types

Since other methods separate supported binary image MIME types from SVG,
where SVG was detected as a "non image type" along with XML, and since I
needed a method to detect MIME types that can be displayed as an image,
I've added such a method.

BUG=398443
Committed: https://crrev.com/4096a6ffddeeba5036aa23137bcbfae3f49d7442
Cr-Commit-Position: refs/heads/master@{#291750}

[Yoav Weiss, 2014-08-14 09:18:48]

As a result of the discussion on https://codereview.chromium.org/430583002/ ,
I'm adding the chrome side changes first.
They're not really used yet (they would be used once the Blink side changes
land), but I've added a unit test for them.

[esprehn, 2014-08-16 07:50:03]

This lgtm to me, but I think abarth@ or jamesr@ needs to review it for content/

[Yoav Weiss, 2014-08-20 20:02:02]

On 2014/08/16 07:50:03, esprehn wrote:
> This lgtm to me, but I think abarth@ or jamesr@ needs to review it for
content/

jam, seems like you reviewed the latest patches in these parts of the content/
code. Can you please review the content/ part of this patch?

[jam, 2014-08-21 19:37:28]

On 2014/08/20 20:02:02, Yoav Weiss wrote:
> On 2014/08/16 07:50:03, esprehn wrote:
> > This lgtm to me, but I think abarth@ or jamesr@ needs to review it for
> content/
> 
> jam, seems like you reviewed the latest patches in these parts of the content/
> code. Can you please review the content/ part of this patch?

lgtm

[Yoav Weiss, 2014-08-21 19:38:20]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2014-08-21 19:39:40]

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

[commit-bot: I haz the power, 2014-08-21 22:19:26]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-21 22:24:31]

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

[commit-bot: I haz the power, 2014-08-21 22:24:32]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Yoav Weiss, 2014-08-22 00:54:17]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2014-08-22 00:59:30]

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

[commit-bot: I haz the power, 2014-08-22 01:02:06]

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

[commit-bot: I haz the power, 2014-08-22 01:04:44]

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

[commit-bot: I haz the power, 2014-08-22 01:04:45]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Yoav Weiss, 2014-08-22 07:34:53]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2014-08-22 07:36:24]

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

[commit-bot: I haz the power, 2014-08-22 07:39:33]

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

[commit-bot: I haz the power, 2014-08-22 07:45:36]

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

[commit-bot: I haz the power, 2014-08-22 07:45:37]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Yoav Weiss, 2014-08-22 13:51:04]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2014-08-22 13:51:13]

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

[commit-bot: I haz the power, 2014-08-22 13:54:37]

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

[commit-bot: I haz the power, 2014-08-22 13:57:17]

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

[commit-bot: I haz the power, 2014-08-22 13:57:18]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Ryan Sleevi, 2014-08-22 23:08:35]

I'm gonna push back and say not LGTM.

It's not clear - from this CL, the bug, or from the comments, how one can or
should correctly use this API, and whether it represents a clean solution to a
real problem or a messy hack towards compatibility.

https://codereview.chromium.org/471913002/diff/1/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/471913002/diff/1/net/base/mime_util.cc#newcod...
net/base/mime_util.cc:735: IsSupportedNonImageMimeType(mime_type)));
This doesn't really make sense to me.

[Yoav Weiss, 2014-08-23 04:57:30]

On 2014/08/22 23:08:35, Ryan Sleevi wrote:
> I'm gonna push back and say not LGTM.
> 
> It's not clear - from this CL, the bug, or from the comments, how one can or
> should correctly use this API, and whether it represents a clean solution to a
> real problem or a messy hack towards compatibility.

Then let me try to clarify. 

The problem is that in some places in Blink, we need to know if current mime
type is a supported "binary" image mime type (e.g. when creating an
ImageDocument).
These parts use MIMETypeRegistry::isSupportedImageResourceMIMEType, which
returns true for supported image types that are not SVG.

What I needed, for the <picture> source selection algorithm, is to know whether
a MIME type is a supported image type (binary or SVG).
So basically, to know if a certain "image/*" MIME type is supported.

> 
> https://codereview.chromium.org/471913002/diff/1/net/base/mime_util.cc
> File net/base/mime_util.cc (right):
> 
>
https://codereview.chromium.org/471913002/diff/1/net/base/mime_util.cc#newcod...
> net/base/mime_util.cc:735: IsSupportedNonImageMimeType(mime_type)));
> This doesn't really make sense to me.

I used the current MIME type maps to check for supported "image/*", by checking
for "image/" and then seeing if the MIME type is in either the (binary) image
supported types map, or in the non-image supported types map(which includes
SVG).

If that's not the best way to tackle this, I'd appreciate some guidance towards
a better alternative.

[Ryan Sleevi, 2014-08-23 05:09:45]

The specific concern is how does someone use this in //net? Your response seems
to confirm my feeling that the answer is "they don't" - that is, it's something
specific to the implementation of <picture>.

I've been pushing back on much of the mime changes in //net, particularly around
audio/video codecs, as these are things generally best expressed in //content
and through the embedder interfaces.

It would seem this concept - the special casing of image/somenonimagetype - is
best when kept as close to the <picture> implementation as possible.

Does that make sense? Are there any other spec cases where this handling makes
any logical sense?

https://codereview.chromium.org/471913002/diff/1/net/base/mime_util.cc
File net/base/mime_util.cc (right):

https://codereview.chromium.org/471913002/diff/1/net/base/mime_util.cc#newcod...
net/base/mime_util.cc:735: IsSupportedNonImageMimeType(mime_type)));
On 2014/08/22 23:08:35, Ryan Sleevi wrote:
> This doesn't really make sense to me.

What I specifically mean is that you check for the image prefix, but then allow
a non-image mime type.

That is, checking for image/nonimagetype is, well, weird.

I realize that <picture> may itself be weird (hence the comment about clean via
messy).

[Yoav Weiss, 2014-08-23 05:24:09]

On 2014/08/23 05:09:45, Ryan Sleevi wrote:
> The specific concern is how does someone use this in //net? Your response
seems
> to confirm my feeling that the answer is "they don't" - that is, it's
something
> specific to the implementation of <picture>.
> 
> I've been pushing back on much of the mime changes in //net, particularly
around
> audio/video codecs, as these are things generally best expressed in //content
> and through the embedder interfaces.
> 
> It would seem this concept - the special casing of image/somenonimagetype - is
> best when kept as close to the <picture> implementation as possible.

That's how I tried to tackle this issue at first, but got push back from
esprehn. See https://codereview.chromium.org/430583002/#ps1

> 
> Does that make sense? Are there any other spec cases where this handling makes
> any logical sense?


I wouldn't say that this is specific to <picture>. I'm not aware of current APIs
that use it, 
but I can imagine the future addition of "type()" to CSS background images, that
would need a similar API.
It's basically an API that answers the question "Should I download this
resources in order to display it as an image?".

> 
> https://codereview.chromium.org/471913002/diff/1/net/base/mime_util.cc
> File net/base/mime_util.cc (right):
> 
>
https://codereview.chromium.org/471913002/diff/1/net/base/mime_util.cc#newcod...
> net/base/mime_util.cc:735: IsSupportedNonImageMimeType(mime_type)));
> On 2014/08/22 23:08:35, Ryan Sleevi wrote:
> > This doesn't really make sense to me.
> 
> What I specifically mean is that you check for the image prefix, but then
allow
> a non-image mime type.
> 
> That is, checking for image/nonimagetype is, well, weird.

Would it be clearer if I simply checked for an image type or SVG?

> 
> I realize that <picture> may itself be weird (hence the comment about clean
via
> messy).

[Ryan Sleevi, 2014-08-23 05:36:53]

On Aug 22, 2014 10:24 PM, <yoav@yoav.ws> wrote:
>
> On 2014/08/23 05:09:45, Ryan Sleevi wrote:
>>
>> The specific concern is how does someone use this in //net? Your response
>
> seems
>>
>> to confirm my feeling that the answer is "they don't" - that is, it's
>
> something
>>
>> specific to the implementation of <picture>.
>
>
>> I've been pushing back on much of the mime changes in //net, particularly
>
> around
>>
>> audio/video codecs, as these are things generally best expressed in
//content
>> and through the embedder interfaces.
>
>
>> It would seem this concept - the special casing of
image/somenonimagetype - is
>> best when kept as close to the <picture> implementation as possible.
>
>
> That's how I tried to tackle this issue at first, but got push back from
> esprehn. See https://codereview.chromium.org/430583002/#ps1

Right, so I totally understand with esprehn pushing back on putting this in
the blink side, what I'm trying to understand is whether this belongs in
//content vs //net

To be clear, I think some of our MIME handling in //net is already weird,
and its something that the media team are looking to clean up and redactor
in 39 (albeit primarily for determining audio/video codec support)

This is, I think, another one of those cases that I think feels like a
layering violation, in as much as //net isn't using this code, ergo it
shouldn't be in here.

>
>
>
>> Does that make sense? Are there any other spec cases where this handling
makes
>> any logical sense?
>
>
>
> I wouldn't say that this is specific to <picture>. I'm not aware of
current APIs
> that use it,
> but I can imagine the future addition of "type()" to CSS background
images, that
> would need a similar API.
> It's basically an API that answers the question "Should I download this
> resources in order to display it as an image?".
>

But that's something for //content to offer Blink via the
SimpleMIMERegistry, right?

That is, none of this behavior alters how the //net stack processes things
(or, largely, how //content behaves, other than to surface the data to
Blink for it to decide)

>
>
>> https://codereview.chromium.org/471913002/diff/1/net/base/mime_util.cc
>> File net/base/mime_util.cc (right):
>
>
>
>
https://codereview.chromium.org/471913002/diff/1/net/base/mime_util.cc#newcod...
>>
>> net/base/mime_util.cc:735: IsSupportedNonImageMimeType(mime_type)));
>> On 2014/08/22 23:08:35, Ryan Sleevi wrote:
>> > This doesn't really make sense to me.
>
>
>> What I specifically mean is that you check for the image prefix, but then
>
> allow
>>
>> a non-image mime type.
>
>
>> That is, checking for image/nonimagetype is, well, weird.
>
>
> Would it be clearer if I simply checked for an image type or SVG?

I think that's precisely why it doesn't make sense to me in //net. //net
shouldn't have to know about svg support (or heck, how enable_svg as a GYP
flag should alter things).

My general feeling is that this may be a question that Blink asks of
//content, and that //content composes it's answer out of //net primitives
(some of which may be moving out of //net and back into //content).

My biggest challenge is in trying to think of what 'harm' would come by
having //content compose this answer, rather than a canonical answer being
given by //net, and that's why I asked about other possible consumers.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Yoav Weiss, 2014-08-23 06:17:01]

On 2014/08/23 05:36:53, Ryan Sleevi wrote:

> Right, so I totally understand with esprehn pushing back on putting this in
> the blink side, what I'm trying to understand is whether this belongs in
> //content vs //net
> 
> To be clear, I think some of our MIME handling in //net is already weird,
> and its something that the media team are looking to clean up and redactor
> in 39 (albeit primarily for determining audio/video codec support)

Great!

> 
> This is, I think, another one of those cases that I think feels like a
> layering violation, in as much as //net isn't using this code, ergo it
> shouldn't be in here.
> 
> > It's basically an API that answers the question "Should I download this
> > resources in order to display it as an image?".
> >
> 
> But that's something for //content to offer Blink via the
> SimpleMIMERegistry, right?

Yeah.

> 
> That is, none of this behavior alters how the //net stack processes things
> (or, largely, how //content behaves, other than to surface the data to
> Blink for it to decide)



> 
> >
> > Would it be clearer if I simply checked for an image type or SVG?
> 
> I think that's precisely why it doesn't make sense to me in //net. //net
> shouldn't have to know about svg support (or heck, how enable_svg as a GYP
> flag should alter things).

But currently all mime support knowledge (including SVG support) is concentrated
in //net . I understand your point that it should move out of there though.

> 
> My general feeling is that this may be a question that Blink asks of
> //content, and that //content composes it's answer out of //net primitives
> (some of which may be moving out of //net and back into //content).
> 
> My biggest challenge is in trying to think of what 'harm' would come by
> having //content compose this answer, rather than a canonical answer being
> given by //net, and that's why I asked about other possible consumers.

I'll try to move that answer's composition into //content then

> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Ryan Sleevi, 2014-08-25 06:19:37]

Ok, clearing my not with a "LGTM", but I think you should push to
abarth/jamesr/jam again with the clean up. From the //net perspective, I think
this is the right layer, and am happy to go to bat for you or to understand why
a //content OWNER disagrees.

[Yoav Weiss, 2014-08-25 06:47:28]

On 2014/08/25 06:19:37, Ryan Sleevi wrote:
> Ok, clearing my not with a "LGTM", but I think you should push to
> abarth/jamesr/jam again with the clean up. From the //net perspective, I think
> this is the right layer, and am happy to go to bat for you or to understand
why
> a //content OWNER disagrees.

Thanks. Added a unit test as well.

jam, can you please take a second look?

[jam, 2014-08-25 18:01:16]

lgtm with nits

https://codereview.chromium.org/471913002/diff/40001/content/child/simple_web...
File content/child/simple_webmimeregistry_impl.cc (right):

https://codereview.chromium.org/471913002/diff/40001/content/child/simple_web...
content/child/simple_webmimeregistry_impl.cc:42: || (ascii_mime_type.substr(0,
6) == "image/"
nit: put the '||' in the previous line

also use StartsWithASCII instead of .substr(0,6)

https://codereview.chromium.org/471913002/diff/40001/content/child/simple_web...
content/child/simple_webmimeregistry_impl.cc:43: &&
net::IsSupportedNonImageMimeType(ascii_mime_type))) ?
&& in previous line

[Yoav Weiss, 2014-08-25 18:28:41]

On 2014/08/25 18:01:16, jam wrote:
> lgtm with nits

Fixed, thanks!

> 
>
https://codereview.chromium.org/471913002/diff/40001/content/child/simple_web...
> File content/child/simple_webmimeregistry_impl.cc (right):
> 
>
https://codereview.chromium.org/471913002/diff/40001/content/child/simple_web...
> content/child/simple_webmimeregistry_impl.cc:42: || (ascii_mime_type.substr(0,
> 6) == "image/"
> nit: put the '||' in the previous line

Blink style leaking :)

> 
> also use StartsWithASCII instead of .substr(0,6)
> 
>
https://codereview.chromium.org/471913002/diff/40001/content/child/simple_web...
> content/child/simple_webmimeregistry_impl.cc:43: &&
> net::IsSupportedNonImageMimeType(ascii_mime_type))) ?
> && in previous line

[Yoav Weiss, 2014-08-25 18:28:53]

The CQ bit was checked by yoav@yoav.ws

[commit-bot: I haz the power, 2014-08-25 18:30:04]

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

[commit-bot: I haz the power, 2014-08-25 19:25:36]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-25 20:24:16]

Committed patchset #4 (60001) as 94caf65c7a4519851055491db555fa74038f89a0

[commit-bot: I haz the power, 2014-09-10 02:37:09]

Patchset 4 (id:??) landed as
https://crrev.com/4096a6ffddeeba5036aa23137bcbfae3f49d7442
Cr-Commit-Position: refs/heads/master@{#291750}