Move the IsSupported* mime functions out of //net and into //components/mime_util

Move the IsSupported* mime functions out of //net and into //components/mime_util

Fundamentally, the question of "Is mime type X supported" is a question
about what the embedder supports. There's a variety of platform-specific
logic already encompassed in this code, particularly around media types
(which are not yet extracted), while handling for items such as images
are just "assumed" that the embedder will support them, because Blink
supports them.

Since these aren't questions about the //net stack's support, nor are
these functions actually used by //net, uplift these to //components.
The media team will extract the remaining functions related to audio,
video, and codec support up out of //net and into //media, at which
point, //net will no longer worry about these.

Note that this is more of a manual move - it intentionally does not
correct some of the "obvious" misuses - to facilitate //media owning
the media move.

BUG=318217
TBR=jochen

Committed: https://crrev.com/c327b48f8ef537bb7fe407d4aa2532a4947c2923
Cr-Commit-Position: refs/heads/master@{#327640}

[Ryan Sleevi, 2015-04-28 00:42:17]

rsleevi@chromium.org changed reviewers:
+ asanka@chromium.org, sky@chromium.org, thestig@chromium.org

[Ryan Sleevi, 2015-04-28 00:42:17]

asanka: Can you review the //net bits
sky: Can you ensure I properly grokked your remarks re: component-ization?
leiz: base/ OWNERs [DEPS add] & chrome/ OWNERS [general blanket for shuffling]
servolk, ddorwin: FYI.

I plan to TBR the remaining bits to appropriate OWNERS, assuming sky@, asanka@,
and thestig@ are happy.

[Lei Zhang, 2015-04-28 04:01:03]

https://codereview.chromium.org/1110833003/diff/1/chrome/browser/DEPS
File chrome/browser/DEPS (left):

https://codereview.chromium.org/1110833003/diff/1/chrome/browser/DEPS#oldcode35
chrome/browser/DEPS:35: "+components/network_hints",
Sorting is hard.

https://codereview.chromium.org/1110833003/diff/1/chrome/browser/chromeos/DEPS
File chrome/browser/chromeos/DEPS (right):

https://codereview.chromium.org/1110833003/diff/1/chrome/browser/chromeos/DEP...
chrome/browser/chromeos/DEPS:2: "+components/mime_util"
Not needed since this rule is already in chrome/browser/DEPS.

https://codereview.chromium.org/1110833003/diff/1/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1110833003/diff/1/chrome/chrome_browser.gypi#...
chrome/chrome_browser.gypi:3045:
'../components/mime_util/mime_util.gyp:mime_util',
Need to do change the GN version as well.

https://codereview.chromium.org/1110833003/diff/1/components/mime_util/DEPS
File components/mime_util/DEPS (right):

https://codereview.chromium.org/1110833003/diff/1/components/mime_util/DEPS#n...
components/mime_util/DEPS:3: "+base",
There's a blanket everyone outside of third_party/ can use base/ DEPS rule in
src/DEPS, so you shouldn't need this.

https://codereview.chromium.org/1110833003/diff/1/components/mime_util/mime_u...
File components/mime_util/mime_util.h (right):

https://codereview.chromium.org/1110833003/diff/1/components/mime_util/mime_u...
components/mime_util/mime_util.h:1: // Copyright (c) 2015 The Chromium Authors.
All rights reserved.
nit: no (C)

[sky, 2015-04-28 13:22:50]

This looks fine to me. You'll need to update bots to run the new tests.

[Ryan Sleevi, 2015-04-28 22:37:23]

Updated with feedback and all tests passing now.

[Lei Zhang, 2015-04-28 22:46:21]

lgtm

[sky, 2015-04-28 23:35:33]

LGTM

[servolk, 2015-04-28 23:38:47]

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

[servolk, 2015-04-28 23:38:48]

https://codereview.chromium.org/1110833003/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (left):

https://codereview.chromium.org/1110833003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:497:
!net::IsSupportedMediaMimeType(mime_type);
I believe this can be simplified to:

bool is_viewable_mime_type = mime_util::IsSupportedNonImageMimeType(mime_type);

Before your change net::IsSupportedNonImageMimeType used to return true for
media mime types (e.g. 'video/webm'), because common_media_types used to be
inserted into MimeUtil::non_image_map_ at
https://code.google.com/p/chromium/codesearch#chromium/src/net/base/mime_util....
But after your change mime_util::non_image_map_ no longer contains
common_media_types, and so there's no need to exclude
net::IsSupportedMediaMimeType explicitly here.
Also, strictly speaking this changes semantics of IsSupportedNonImageMimeType,
so we should look carefully at all 3 places where IsSupportedNonImageMimeType is
being used, other than unit tests. E.g. we should probably change
SimpleWebMimeRegistryImpl::supportsNonImageMIMEType to include
common_media_types, to make sure behavior is preserved on blink level.

[asanka, 2015-04-29 00:14:06]

asanka@chromium.org changed reviewers:
- servolk@chromium.org

[asanka, 2015-04-29 00:14:07]

Thanks for doing this! LGTM. Comments are minor nits.

My only concern, which I expressed over IM to rsleevi is that we are moving
around a blink::WebMimeRegistry implementation. I think ideally content and
html_viewer should have their own implementation of capabilities rather than
share a common set. But I understand that this is known will be addressed at a
later time.

https://codereview.chromium.org/1110833003/diff/20001/components/mime_util/mi...
File components/mime_util/mime_util.cc (right):

https://codereview.chromium.org/1110833003/diff/20001/components/mime_util/mi...
components/mime_util/mime_util.cc:121: using MimeMappings =
base::hash_set<std::string>;
Minor nit: there are sets. Not maps.

(I know you are just moving code).

https://codereview.chromium.org/1110833003/diff/20001/components/mime_util/mi...
components/mime_util/mime_util.cc:134: // Initialize the supported non-image
types.
Nit: remove.

https://codereview.chromium.org/1110833003/diff/20001/components/mime_util/mi...
components/mime_util/mime_util.cc:174: // Mirrors WebViewImpl::CanShowMIMEType()
Nit: Comment appears to have bitrotted.

[asanka, 2015-04-29 02:26:37]

https://codereview.chromium.org/1110833003/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (left):

https://codereview.chromium.org/1110833003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:497:
!net::IsSupportedMediaMimeType(mime_type);
On 2015/04/28 at 23:38:48, servolk wrote:
> I believe this can be simplified to:
> 
> bool is_viewable_mime_type =
mime_util::IsSupportedNonImageMimeType(mime_type);
> 
> Before your change net::IsSupportedNonImageMimeType used to return true for
media mime types (e.g. 'video/webm'), because common_media_types used to be
inserted into MimeUtil::non_image_map_ at
https://code.google.com/p/chromium/codesearch#chromium/src/net/base/mime_util....
But after your change mime_util::non_image_map_ no longer contains
common_media_types, and so there's no need to exclude
net::IsSupportedMediaMimeType explicitly here.
> Also, strictly speaking this changes semantics of IsSupportedNonImageMimeType,
so we should look carefully at all 3 places where IsSupportedNonImageMimeType is
being used, other than unit tests. E.g. we should probably change
SimpleWebMimeRegistryImpl::supportsNonImageMIMEType to include
common_media_types, to make sure behavior is preserved on blink level.


The behavior is preserved by having mime_util::IsSupportedNonImageMimeType()
call into net::IsSupportedMediaMimeType(). Can you doublecheck?

[Ryan Sleevi, 2015-04-29 05:29:07]

https://codereview.chromium.org/1110833003/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (left):

https://codereview.chromium.org/1110833003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:497:
!net::IsSupportedMediaMimeType(mime_type);
On 2015/04/29 02:26:37, asanka wrote:
> The behavior is preserved by having mime_util::IsSupportedNonImageMimeType()
> call into net::IsSupportedMediaMimeType(). Can you doublecheck?

Right, Sergey, I'm not quite sure I follow - the media map and modifications are
still preserved by having IsSupportedMediaMimeType call, which is effectively
all the non_image_map_ was doing. Could you double check your analysis and let
us know if we've missed something?

[servolk, 2015-04-29 16:58:30]

On 2015/04/29 05:29:07, Ryan Sleevi wrote:
>
https://codereview.chromium.org/1110833003/diff/20001/content/browser/frame_h...
> File content/browser/frame_host/navigation_controller_impl.cc (left):
> 
>
https://codereview.chromium.org/1110833003/diff/20001/content/browser/frame_h...
> content/browser/frame_host/navigation_controller_impl.cc:497:
> !net::IsSupportedMediaMimeType(mime_type);
> On 2015/04/29 02:26:37, asanka wrote:
> > The behavior is preserved by having mime_util::IsSupportedNonImageMimeType()
> > call into net::IsSupportedMediaMimeType(). Can you doublecheck?
> 
> Right, Sergey, I'm not quite sure I follow - the media map and modifications
are
> still preserved by having IsSupportedMediaMimeType call, which is effectively
> all the non_image_map_ was doing. Could you double check your analysis and let
> us know if we've missed something?

Oops, sorry, my bad. I haven't noticed that the new IsSupportedNonImageMimeType
calls net::IsSupportedMediaMimeType now, the implementation looked similar to
the old implementation that checked the non_image_map_ and then a couple of
other (text, "application/...") things. After looking again it looks good to me.
Thanks!

[Ryan Sleevi, 2015-04-29 23:23:48]

rsleevi@chromium.org changed reviewers:
+ davidben@chromium.org, jochen@chromium.org

[Ryan Sleevi, 2015-04-29 23:23:48]

davidben@chromium.org: Please review changes in 
//content

jochen@chromium.org: Please review changes in 
//components

[davidben, 2015-04-29 23:25:11]

content lgtm apart from IWYU file nits. I did not review the scope of the move
(apparently there are interesting constraints in what can be reshuffled when?)
and assume that other reviewers covered that. I just looked at content w.r.t.
stuff getting moved from net to components/mime_util.

https://codereview.chromium.org/1110833003/diff/20001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1110833003/diff/20001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:95: #include
"net/base/mime_util.h"
No longer needed?

https://codereview.chromium.org/1110833003/diff/20001/content/child/blink_pla...
File content/child/blink_platform_impl.cc (right):

https://codereview.chromium.org/1110833003/diff/20001/content/child/blink_pla...
content/child/blink_platform_impl.cc:53: #include "net/base/mime_util.h"
No longer needed?

https://codereview.chromium.org/1110833003/diff/20001/content/child/web_url_l...
File content/child/web_url_loader_impl.cc (right):

https://codereview.chromium.org/1110833003/diff/20001/content/child/web_url_l...
content/child/web_url_loader_impl.cc:36: #include "net/base/mime_util.h"
no longer needed?

https://codereview.chromium.org/1110833003/diff/20001/net/base/mime_util.cc
File net/base/mime_util.cc (left):

https://codereview.chromium.org/1110833003/diff/20001/net/base/mime_util.cc#o...
net/base/mime_util.cc:1393: 
Remove this line too?

[Ryan Sleevi, 2015-04-30 00:34:37]

TBR jochen for the last part, which is //components/BUILD.gn

[Ryan Sleevi, 2015-04-30 00:34:41]

The CQ bit was checked by rsleevi@chromium.org

[Ryan Sleevi, 2015-04-30 00:34:42]

The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org,
thestig@chromium.org, sky@chromium.org, davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1110833003/#ps60001
(title: "IWYU fix")

[commit-bot: I haz the power, 2015-04-30 00:35:31]

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

[commit-bot: I haz the power, 2015-04-30 00:40:06]

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

[commit-bot: I haz the power, 2015-04-30 00:40:07]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ryan Sleevi, 2015-04-30 00:46:37]

The CQ bit was checked by rsleevi@chromium.org

[Ryan Sleevi, 2015-04-30 00:46:37]

The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org,
thestig@chromium.org, sky@chromium.org, davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1110833003/#ps80001
(title: "Rebased")

[commit-bot: I haz the power, 2015-04-30 00:47:10]

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

[jam, 2015-04-30 00:49:53]

The CQ bit was unchecked by jam@chromium.org

[jam, 2015-04-30 00:51:16]

not lgtm

content can only depend on components that it shares with mandoline (see cr-dev
thread which was clear about this, and also comments in DEPS files that this
changes). components isn't a place to put random code which content uses now.

[Ryan Sleevi, 2015-04-30 00:51:38]

On 2015/04/30 00:51:16, jam wrote:
> not lgtm
> 
> content can only depend on components that it shares with mandoline (see
cr-dev
> thread which was clear about this, and also comments in DEPS files that this
> changes). components isn't a place to put random code which content uses now.

And see sky's LGTM and related bug that made this explicitly clear that it was
OK.

[Ryan Sleevi, 2015-04-30 00:52:08]

On 2015/04/30 00:51:38, Ryan Sleevi wrote:
> On 2015/04/30 00:51:16, jam wrote:
> > not lgtm
> > 
> > content can only depend on components that it shares with mandoline (see
> cr-dev
> > thread which was clear about this, and also comments in DEPS files that this
> > changes). components isn't a place to put random code which content uses
now.
> 
> And see sky's LGTM and related bug that made this explicitly clear that it was
> OK.

https://code.google.com/p/chromium/issues/detail?id=478344

[jam, 2015-04-30 00:59:48]

On 2015/04/30 00:52:08, Ryan Sleevi wrote:
> On 2015/04/30 00:51:38, Ryan Sleevi wrote:
> > On 2015/04/30 00:51:16, jam wrote:
> > > not lgtm
> > > 
> > > content can only depend on components that it shares with mandoline (see
> > cr-dev
> > > thread which was clear about this, and also comments in DEPS files that
this
> > > changes). components isn't a place to put random code which content uses
> now.
> > 
> > And see sky's LGTM and related bug that made this explicitly clear that it
was
> > OK.

I think Scott wasn't aware of the policy; I'll email owners in content to remind
them.

> 
> https://code.google.com/p/chromium/issues/detail?id=478344

The policy in content to relax content to depend on a few select components
(only those shared with html_viewer) was pretty specific. Other bugs written by
non-content owners doesn't change which modules content can or can't depend on.

[Ryan Sleevi, 2015-04-30 01:02:26]

On 2015/04/30 00:59:48, jam wrote:
> I think Scott wasn't aware of the policy; I'll email owners in content to
remind
> them.

I'm surprised you say that, because it's specifically referenced on the bug I
linked to you, and what we discussed.

It's unclear what you're talking about as a separate policy, but this seems
entirely in line with what erg@ emailed chromium-dev, as discussed on the bug,
so rather than emailing //content owners, it would help to email chromium-dev
about this.

> The policy in content to relax content to depend on a few select components
> (only those shared with html_viewer) was pretty specific. Other bugs written
by
> non-content owners doesn't change which modules content can or can't depend
on.

And this *is* a component shared with html_viewer, as mentioned in the bug I
linked, along with the attendant issues.

Could you explain how this violates this policy, preferably either here or in
this bug? And can you provide an alternative solution for the high friction this
is presently causing, as otherwise blocking refactoring "because mojo" seems an
unfortunate way to make progress.

[jam, 2015-04-30 01:16:32]

On 2015/04/30 01:02:26, Ryan Sleevi wrote:
> On 2015/04/30 00:59:48, jam wrote:
> > I think Scott wasn't aware of the policy; I'll email owners in content to
> remind
> > them.
> 
> I'm surprised you say that, because it's specifically referenced on the bug I
> linked to you, and what we discussed.

ah sorry I was looking at the bug linked to in the cl description and not the
one in the previous comment

> 
> It's unclear what you're talking about as a separate policy, but this seems
> entirely in line with what erg@ emailed chromium-dev, as discussed on the bug,
> so rather than emailing //content owners, it would help to email chromium-dev
> about this.
> 
> > The policy in content to relax content to depend on a few select components
> > (only those shared with html_viewer) was pretty specific. Other bugs written
> by
> > non-content owners doesn't change which modules content can or can't depend
> on.
> 
> And this *is* a component shared with html_viewer, as mentioned in the bug I
> linked, along with the attendant issues.
> 
> Could you explain how this violates this policy, preferably either here or in
> this bug? And can you provide an alternative solution for the high friction
this
> is presently causing, as otherwise blocking refactoring "because mojo" seems
an
> unfortunate way to make progress.

apologies i read this change too fast and didn't see the part where html_viewer
calls out to the new code. you're absolutely right this is fine according to the
new policy, so lgtm

[jam, 2015-04-30 01:16:38]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2015-04-30 01:17:48]

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

[commit-bot: I haz the power, 2015-04-30 02:03:34]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-04-30 02:04:26]

Patchset 5 (id:??) landed as
https://crrev.com/c327b48f8ef537bb7fe407d4aa2532a4947c2923
Cr-Commit-Position: refs/heads/master@{#327640}