Move media related mimetype functionality out of net/ and into media/.

Move media related mimetype functionality out of net/ and into media/.

Code that determines whether particular mimetypes are supported is being
moved into media/ so that we can ultimately make this code base decisions
on what decoders we actually have available on the platform. This
change simply moves the existing code from net/base to media/base.

BUG=318217, 163800

[acolwell GONE FROM CHROMIUM, 2014-07-17 23:19:31]

scherkus & rsleevi: please review everything
darin: Owners for content changes

https://codereview.chromium.org/401523002/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_impl.cc (left):

https://codereview.chromium.org/401523002/diff/40001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl.cc:487:
!net::IsSupportedMediaMimeType(mime_type);
Since media mimetypes aren't in the non-image map, we don't have to make the
extra call to exclude them.

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

https://codereview.chromium.org/401523002/diff/40001/content/child/simple_web...
content/child/simple_webmimeregistry_impl.cc:74: ||
media::IsSupportedMediaMimeType(ToASCIIOrEmpty(mime_type))) ?
Now that the media types aren't in the non-image map, we need the extra check.

[Ryan Sleevi, 2014-07-17 23:21:58]

Yay! Thanks for this. I think it's the right place, since media, more than
anything, is a sort of embedder-specific view on things.

Thanks and LGTM

[scherkus (not reviewing), 2014-07-18 01:02:33]

long overdue!

now if you also upload media/base/mime_util* files we can look at them too ;)

https://codereview.chromium.org/401523002/diff/100001/content/child/simple_we...
File content/child/simple_webmimeregistry_impl.cc (right):

https://codereview.chromium.org/401523002/diff/100001/content/child/simple_we...
content/child/simple_webmimeregistry_impl.cc:74: ||
media::IsSupportedMediaMimeType(ToASCIIOrEmpty(mime_type))) ?
nit: ||'s go on the previous line

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

https://codereview.chromium.org/401523002/diff/100001/net/base/mime_util.cc#n...
net/base/mime_util.cc:36: { "audio", "audio/" },
still needed?

https://codereview.chromium.org/401523002/diff/100001/net/base/mime_util.cc#n...
net/base/mime_util.cc:43: { "video", "video/" },
still needed?

https://codereview.chromium.org/401523002/diff/100001/net/base/mime_util.cc#n...
net/base/mime_util.cc:115: { "video/mp4", "mp4,m4v" },
codec-y stuff in here

still needed?

https://codereview.chromium.org/401523002/diff/100001/net/base/mime_util.cc#n...
net/base/mime_util.cc:331: static bool IsCodecSupportedOnAndroid(const
std::string& codec) {
shouldn't these also get moved?

https://codereview.chromium.org/401523002/diff/100001/net/base/mime_util.cc#n...
net/base/mime_util.cc:361: struct MediaFormatStrict {
struct isn't used anymore

[scherkus (not reviewing), 2014-07-18 01:06:03]

also here is a sampling of bugs previously filed that relate to this work:

https://code.google.com/p/chromium/issues/detail?id=21318
https://code.google.com/p/chromium/issues/detail?id=188852
https://code.google.com/p/chromium/issues/detail?id=163800
https://code.google.com/p/chromium/issues/detail?id=318217

I think you can use 318217

[acolwell GONE FROM CHROMIUM, 2014-07-18 01:33:51]

new files are included now. They got lost as I was moving things around and
rebasing.

https://codereview.chromium.org/401523002/diff/100001/content/child/simple_we...
File content/child/simple_webmimeregistry_impl.cc (right):

https://codereview.chromium.org/401523002/diff/100001/content/child/simple_we...
content/child/simple_webmimeregistry_impl.cc:74: ||
media::IsSupportedMediaMimeType(ToASCIIOrEmpty(mime_type))) ?
On 2014/07/18 01:02:33, scherkus wrote:
> nit: ||'s go on the previous line

Done.

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

https://codereview.chromium.org/401523002/diff/100001/net/base/mime_util.cc#n...
net/base/mime_util.cc:36: { "audio", "audio/" },
On 2014/07/18 01:02:33, scherkus wrote:
> still needed?
Yes. These are still valid media types.

https://codereview.chromium.org/401523002/diff/100001/net/base/mime_util.cc#n...
net/base/mime_util.cc:43: { "video", "video/" },
On 2014/07/18 01:02:33, scherkus wrote:
> still needed?
ditto

https://codereview.chromium.org/401523002/diff/100001/net/base/mime_util.cc#n...
net/base/mime_util.cc:115: { "video/mp4", "mp4,m4v" },
On 2014/07/18 01:02:33, scherkus wrote:
> codec-y stuff in here
> 
> still needed?
For now yes. I'd like to defer moving this out to another CL since it may be a
little more tricky.

https://codereview.chromium.org/401523002/diff/100001/net/base/mime_util.cc#n...
net/base/mime_util.cc:331: static bool IsCodecSupportedOnAndroid(const
std::string& codec) {
On 2014/07/18 01:02:33, scherkus wrote:
> shouldn't these also get moved?

Done.

https://codereview.chromium.org/401523002/diff/100001/net/base/mime_util.cc#n...
net/base/mime_util.cc:361: struct MediaFormatStrict {
On 2014/07/18 01:02:33, scherkus wrote:
> struct isn't used anymore

Done.

[scherkus (not reviewing), 2014-07-18 02:00:00]

https://codereview.chromium.org/401523002/diff/120001/media/base/mime_util.h
File media/base/mime_util.h (right):

https://codereview.chromium.org/401523002/diff/120001/media/base/mime_util.h#...
media/base/mime_util.h:4: #ifndef MEDIA_BASE_MIME_UTIL_H_
blank line before this

https://codereview.chromium.org/401523002/diff/120001/media/base/mime_util.h#...
media/base/mime_util.h:70: MEDIA_EXPORT void GetExtensionsForMimeType(
this isn't implemented

https://codereview.chromium.org/401523002/diff/120001/media/base/mime_util_un...
File media/base/mime_util_unittest.cc (right):

https://codereview.chromium.org/401523002/diff/120001/media/base/mime_util_un...
media/base/mime_util_unittest.cc:48: bool HLSSupported;
mind changing this to hls_supported?

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

https://codereview.chromium.org/401523002/diff/120001/net/base/mime_util.cc#n...
net/base/mime_util.cc:22: #include "base/android/build_info.h"
header doesn't look like it's needed anymore

[darin (slow to review), 2014-07-18 04:25:44]

LGTM for content/ changes

[acolwell GONE FROM CHROMIUM, 2014-07-18 16:17:12]

https://codereview.chromium.org/401523002/diff/120001/media/base/mime_util.h
File media/base/mime_util.h (right):

https://codereview.chromium.org/401523002/diff/120001/media/base/mime_util.h#...
media/base/mime_util.h:4: #ifndef MEDIA_BASE_MIME_UTIL_H_
On 2014/07/18 02:00:00, scherkus wrote:
> blank line before this

Done.

https://codereview.chromium.org/401523002/diff/120001/media/base/mime_util.h#...
media/base/mime_util.h:70: MEDIA_EXPORT void GetExtensionsForMimeType(
On 2014/07/18 02:00:00, scherkus wrote:
> this isn't implemented
removed

https://codereview.chromium.org/401523002/diff/120001/media/base/mime_util_un...
File media/base/mime_util_unittest.cc (right):

https://codereview.chromium.org/401523002/diff/120001/media/base/mime_util_un...
media/base/mime_util_unittest.cc:48: bool HLSSupported;
On 2014/07/18 02:00:00, scherkus wrote:
> mind changing this to hls_supported?

Done.

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

https://codereview.chromium.org/401523002/diff/120001/net/base/mime_util.cc#n...
net/base/mime_util.cc:22: #include "base/android/build_info.h"
On 2014/07/18 02:00:00, scherkus wrote:
> header doesn't look like it's needed anymore

Done.

[scherkus (not reviewing), 2014-07-18 16:18:08]

lgtm!

[acolwell GONE FROM CHROMIUM, 2014-07-18 16:18:32]

The CQ bit was checked by acolwell@chromium.org

[commit-bot: I haz the power, 2014-07-18 16:19:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/acolwell@chromium.org/401523002/140001

[commit-bot: I haz the power, 2014-07-18 20:32:49]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-07-18 20:54:48]

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

[commit-bot: I haz the power, 2014-07-18 20:54:49]

Try jobs failed on following builders:
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)

[acolwell GONE FROM CHROMIUM, 2014-07-18 22:34:53]

The CQ bit was checked by acolwell@chromium.org

[commit-bot: I haz the power, 2014-07-18 22:37:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/acolwell@chromium.org/401523002/140001

[acolwell GONE FROM CHROMIUM, 2014-07-18 23:22:37]

The CQ bit was unchecked by acolwell@chromium.org

[acolwell GONE FROM CHROMIUM, 2014-07-23 16:44:24]

darin: PTAL since I had to make several content/ & chrome/ changes to make the
bots happy. Specifically, I wanted to make sure you were ok with the
introduction of content/common/mime_util.h to avoid duplicate code in various
places.

[darin (slow to review), 2014-07-24 17:52:53]

https://codereview.chromium.org/401523002/diff/200001/content/common/mime_uti...
File content/common/mime_util.cc (right):

https://codereview.chromium.org/401523002/diff/200001/content/common/mime_uti...
content/common/mime_util.cc:15: return net::IsSupportedMimeType(mime_type) ||
the only concern I have is that it could be confusing what version of
IsSupportedMimeType one is to call. i would almost prefer some form of
dependency injection into net's mime_util. you could imagine pushing settings
into it, or set a delegate method for it to call. that way everyone will still
think the truth of mime type info lives in net/base/mime_util.h.

[Ryan Sleevi, 2014-07-24 18:24:09]

https://codereview.chromium.org/401523002/diff/200001/content/common/mime_uti...
File content/common/mime_util.cc (right):

https://codereview.chromium.org/401523002/diff/200001/content/common/mime_uti...
content/common/mime_util.cc:15: return net::IsSupportedMimeType(mime_type) ||
On 2014/07/24 17:52:53, darin wrote:
> the only concern I have is that it could be confusing what version of
> IsSupportedMimeType one is to call. i would almost prefer some form of
> dependency injection into net's mime_util. you could imagine pushing settings
> into it, or set a delegate method for it to call. that way everyone will still
> think the truth of mime type info lives in net/base/mime_util.h.

I do think it's weird to have the truth of mime type info in //net. There's
really all sorts of lies in it.

For example, if a plugin handles a mime-type, is it supported or not? What about
a CDM (where some of this is coming up w/r/t //media)?

There's actually only one place in //net it's used, and that's whether or not to
download something if it wasn't explicitly requested, and if and only if it's an
explicitly .gz/.tgz file. Which is, well, weird.

[darin (slow to review), 2014-07-25 04:45:40]

On Thu, Jul 24, 2014 at 11:24 AM, <rsleevi@chromium.org> wrote:

>
> https://codereview.chromium.org/401523002/diff/200001/
> content/common/mime_util.cc
> File content/common/mime_util.cc (right):
>
> https://codereview.chromium.org/401523002/diff/200001/
> content/common/mime_util.cc#newcode15
> content/common/mime_util.cc:15: return
> net::IsSupportedMimeType(mime_type) ||
> On 2014/07/24 17:52:53, darin wrote:
>
>> the only concern I have is that it could be confusing what version of
>> IsSupportedMimeType one is to call. i would almost prefer some form of
>> dependency injection into net's mime_util. you could imagine pushing
>>
> settings
>
>> into it, or set a delegate method for it to call. that way everyone
>>
> will still
>
>> think the truth of mime type info lives in net/base/mime_util.h.
>>
>
> I do think it's weird to have the truth of mime type info in //net.
> There's really all sorts of lies in it.
>
> For example, if a plugin handles a mime-type, is it supported or not?
> What about a CDM (where some of this is coming up w/r/t //media)?
>
> There's actually only one place in //net it's used, and that's whether
> or not to download something if it wasn't explicitly requested, and if
> and only if it's an explicitly .gz/.tgz file. Which is, well, weird.
>
>

Perhaps the answer is that we should move the entire system out of src/net,
and then use some other means to address the needs there. I'd be OK with
that too. It has always been an awkward fit there. (I think we may have
used it more from src/net at one point in the past.)

-Darin



> https://codereview.chromium.org/401523002/
>

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