Moved media mime type and codec checks to media/base/mime_util.*

Moved media mime type and codec checks to media/base/mime_util.*

BUG=318217
Committed: https://crrev.com/f395553f1e25bea309447770df093ed203ea111e
Cr-Commit-Position: refs/heads/master@{#330239}

[servolk, 2015-05-06 00:02:24]

servolk@chromium.org changed reviewers:
+ ddorwin@chromium.org, gunsch@chromium.org, lcwu@chromium.org,
rsleevi@chromium.org

[Ryan Sleevi, 2015-05-06 00:59:13]

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

https://codereview.chromium.org/1129643002/diff/80001/media/base/mime_util_un...
media/base/mime_util_unittest.cc:16: TEST(MimeUtilTest, LookupTypes) {
CLEANUP: Nothing for non-Android here? ;)

Should this be folded into CommonMediaMimeTypes?

https://codereview.chromium.org/1129643002/diff/80001/media/base/mime_util_un...
media/base/mime_util_unittest.cc:62: #endif
CLEANUP: It seems this is no longer needed, as we now require AT LEAST SDK
version 15 (aka ICS), AFAICT. I think we require version 16 (JB).

I would not recommend it for this CL, but something to do immediately after.

This would also let you remove the buildinfo include.

https://codereview.chromium.org/1129643002/diff/80001/media/blink/key_system_...
File media/blink/key_system_config_selector.cc (right):

https://codereview.chromium.org/1129643002/diff/80001/media/blink/key_system_...
media/blink/key_system_config_selector.cc:290: == media::IsSupported);
is this git cl formatted?

[servolk, 2015-05-06 01:13:39]

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

https://codereview.chromium.org/1129643002/diff/80001/media/base/mime_util_un...
media/base/mime_util_unittest.cc:16: TEST(MimeUtilTest, LookupTypes) {
On 2015/05/06 00:59:12, Ryan Sleevi wrote:
> CLEANUP: Nothing for non-Android here? ;)
> 
> Should this be folded into CommonMediaMimeTypes?

Yep, but it was like that in net/base/mime_util_unittest.cc and initially I just
moved some code with minimal changes as David suggested. I'll move this into
CommonMediaMimeTypes.

https://codereview.chromium.org/1129643002/diff/80001/media/base/mime_util_un...
media/base/mime_util_unittest.cc:62: #endif
On 2015/05/06 00:59:12, Ryan Sleevi wrote:
> CLEANUP: It seems this is no longer needed, as we now require AT LEAST SDK
> version 15 (aka ICS), AFAICT. I think we require version 16 (JB).
> 
> I would not recommend it for this CL, but something to do immediately after.
> 
> This would also let you remove the buildinfo include.

Ok, will do after this CL

https://codereview.chromium.org/1129643002/diff/80001/media/blink/key_system_...
File media/blink/key_system_config_selector.cc (right):

https://codereview.chromium.org/1129643002/diff/80001/media/blink/key_system_...
media/blink/key_system_config_selector.cc:290: == media::IsSupported);
On 2015/05/06 00:59:13, Ryan Sleevi wrote:
> is this git cl formatted?

No, for some reason it's broken on my machine, and I have no idea why:
servolk@servolk:/media/ssd850/chromium/src$ git cl format
Traceback (most recent call last):
  File
"/media/ssd850/chromium/src/buildtools/clang_format/script/clang-format-diff.py",
line 120, in <module>
    main()
  File
"/media/ssd850/chromium/src/buildtools/clang_format/script/clang-format-diff.py",
line 103, in main
    stderr=None, stdin=subprocess.PIPE)
  File "/usr/lib/python2.7/subprocess.py", line 710, in __init__
    errread, errwrite)
  File "/usr/lib/python2.7/subprocess.py", line 1327, in _execute_child
    raise child_exception
OSError: [Errno 13] Permission denied
Command "/usr/bin/python
/media/ssd850/chromium/src/buildtools/clang_format/script/clang-format-diff.py
-p0 -i" failed.

[servolk, 2015-05-06 17:03:56]

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

https://codereview.chromium.org/1129643002/diff/80001/media/base/mime_util_un...
media/base/mime_util_unittest.cc:62: #endif
On 2015/05/06 01:13:39, servolk wrote:
> On 2015/05/06 00:59:12, Ryan Sleevi wrote:
> > CLEANUP: It seems this is no longer needed, as we now require AT LEAST SDK
> > version 15 (aka ICS), AFAICT. I think we require version 16 (JB).
> > 
> > I would not recommend it for this CL, but something to do immediately after.
> > 
> > This would also let you remove the buildinfo include.
> 
> Ok, will do after this CL

Done as a separate CL:
https://codereview.chromium.org/1128983002

[servolk, 2015-05-07 22:47:32]

On 2015/05/06 17:03:56, servolk wrote:
>
https://codereview.chromium.org/1129643002/diff/80001/media/base/mime_util_un...
> File media/base/mime_util_unittest.cc (right):
> 
>
https://codereview.chromium.org/1129643002/diff/80001/media/base/mime_util_un...
> media/base/mime_util_unittest.cc:62: #endif
> On 2015/05/06 01:13:39, servolk wrote:
> > On 2015/05/06 00:59:12, Ryan Sleevi wrote:
> > > CLEANUP: It seems this is no longer needed, as we now require AT LEAST SDK
> > > version 15 (aka ICS), AFAICT. I think we require version 16 (JB).
> > > 
> > > I would not recommend it for this CL, but something to do immediately
after.
> > > 
> > > This would also let you remove the buildinfo include.
> > 
> > Ok, will do after this CL
> 
> Done as a separate CL:
> https://codereview.chromium.org/1128983002

Ping, PTAL at this CL. I believe it's good. There has been some weird
ios_rel_device_ninja trybot failures yesterday (e.g.
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
which I can't explain, looks like the trybot was trying to build SSE2 code on
armv7/arm64 platform for some reason. But I've tried re-running that trybot
today without making any changes to my CL and it has passed three times in a row
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...),
so if anything, I believe that's an issue with buildbot and not my CL.

[Ryan Sleevi, 2015-05-07 22:57:28]

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

[Ryan Sleevi, 2015-05-07 22:57:30]

//net and //components/mime_util LGTM.

You still need a //components and //content owner, and jochen@ happens to be in
both OWNERs, so adding him.

[ddorwin, 2015-05-08 16:29:19]

LG. Most of the comments are about #include "build/build_config.h", which we
learned the hard way needs to be included whenever OS_* defines are tested.

https://codereview.chromium.org/1129643002/diff/200001/components/mime_util/m...
File components/mime_util/mime_util.cc (right):

https://codereview.chromium.org/1129643002/diff/200001/components/mime_util/m...
components/mime_util/mime_util.cc:25: #if defined(OS_ANDROID)
#include "build/build_config.h"

https://codereview.chromium.org/1129643002/diff/200001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1129643002/diff/200001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:758: #if
defined(OS_ANDROID)
#include "build/build_config.h"

https://codereview.chromium.org/1129643002/diff/200001/content/renderer/rende...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/1129643002/diff/200001/content/renderer/rende...
content/renderer/renderer_blink_platform_impl.cc:87: #if defined(OS_ANDROID)
#include "build/build_config.h"

https://codereview.chromium.org/1129643002/diff/200001/content/shell/app/shel...
File content/shell/app/shell_main_delegate.cc (right):

https://codereview.chromium.org/1129643002/diff/200001/content/shell/app/shel...
content/shell/app/shell_main_delegate.cc:49: #if defined(OS_ANDROID)
#include "build/build_config.h"

https://codereview.chromium.org/1129643002/diff/200001/media/base/mime_util.cc
File media/base/mime_util.cc (right):

https://codereview.chromium.org/1129643002/diff/200001/media/base/mime_util.c...
media/base/mime_util.cc:1: // Copyright 2015 The Chromium Authors. All rights
reserved.
don't change the year

https://codereview.chromium.org/1129643002/diff/200001/media/base/mime_util.c...
media/base/mime_util.cc:14: #if defined(OS_ANDROID)
Need #include "build/build_config.h" to get these defines.

Please update //net's and //components' version too if appropriate.

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

https://codereview.chromium.org/1129643002/diff/200001/media/base/mime_util.h...
media/base/mime_util.h:1: // Copyright 2015 The Chromium Authors. All rights
reserved.
We should probably keep the year from the other file - this is not entirely new.

https://codereview.chromium.org/1129643002/diff/200001/media/base/mime_util.h...
media/base/mime_util.h:13: namespace media {
I'm assuming everything below was copied without changes, right?

https://codereview.chromium.org/1129643002/diff/200001/media/base/mime_util.h...
media/base/mime_util.h:15: // Check to see if a particular MIME type is in the
list of
In a future CL, we need to update these comments to "Returns..."

https://codereview.chromium.org/1129643002/diff/200001/media/base/mime_util_u...
File media/base/mime_util_unittest.cc (right):

https://codereview.chromium.org/1129643002/diff/200001/media/base/mime_util_u...
media/base/mime_util_unittest.cc:10: #if defined(OS_ANDROID)
#include "build/build_config.h"

https://codereview.chromium.org/1129643002/diff/200001/media/base/mime_util_u...
media/base/mime_util_unittest.cc:14: namespace media {
I'm assuming everything below was copied without changes, right?

https://codereview.chromium.org/1129643002/diff/200001/media/blink/key_system...
File media/blink/key_system_config_selector.cc (right):

https://codereview.chromium.org/1129643002/diff/200001/media/blink/key_system...
media/blink/key_system_config_selector.cc:180: #if defined(OS_ANDROID)
#include "build/build_config.h"

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

https://codereview.chromium.org/1129643002/diff/200001/net/base/mime_util.cc#...
net/base/mime_util.cc:466: #if defined(OS_WIN)
#include "build/build_config.h"

https://codereview.chromium.org/1129643002/diff/200001/net/base/mime_util_uni...
File net/base/mime_util_unittest.cc (right):

https://codereview.chromium.org/1129643002/diff/200001/net/base/mime_util_uni...
net/base/mime_util_unittest.cc:24: #if defined(OS_ANDROID)
#include "build/build_config.h"

[servolk, 2015-05-08 17:33:05]

https://codereview.chromium.org/1129643002/diff/200001/components/mime_util/m...
File components/mime_util/mime_util.cc (right):

https://codereview.chromium.org/1129643002/diff/200001/components/mime_util/m...
components/mime_util/mime_util.cc:25: #if defined(OS_ANDROID)
On 2015/05/08 16:29:18, ddorwin wrote:
> #include "build/build_config.h"

Already included in this file.

https://codereview.chromium.org/1129643002/diff/200001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1129643002/diff/200001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:758: #if
defined(OS_ANDROID)
On 2015/05/08 16:29:18, ddorwin wrote:
> #include "build/build_config.h"

Done.

https://codereview.chromium.org/1129643002/diff/200001/content/shell/app/shel...
File content/shell/app/shell_main_delegate.cc (right):

https://codereview.chromium.org/1129643002/diff/200001/content/shell/app/shel...
content/shell/app/shell_main_delegate.cc:49: #if defined(OS_ANDROID)
On 2015/05/08 16:29:18, ddorwin wrote:
> #include "build/build_config.h"

Done.

https://codereview.chromium.org/1129643002/diff/200001/media/base/mime_util.cc
File media/base/mime_util.cc (right):

https://codereview.chromium.org/1129643002/diff/200001/media/base/mime_util.c...
media/base/mime_util.cc:1: // Copyright 2015 The Chromium Authors. All rights
reserved.
On 2015/05/08 16:29:18, ddorwin wrote:
> don't change the year

Done.

https://codereview.chromium.org/1129643002/diff/200001/media/base/mime_util.c...
media/base/mime_util.cc:14: #if defined(OS_ANDROID)
On 2015/05/08 16:29:18, ddorwin wrote:
> Need #include "build/build_config.h" to get these defines.
> 
> Please update //net's and //components' version too if appropriate.

Done.

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

https://codereview.chromium.org/1129643002/diff/200001/media/base/mime_util.h...
media/base/mime_util.h:1: // Copyright 2015 The Chromium Authors. All rights
reserved.
On 2015/05/08 16:29:18, ddorwin wrote:
> We should probably keep the year from the other file - this is not entirely
new.

Done.

https://codereview.chromium.org/1129643002/diff/200001/media/base/mime_util.h...
media/base/mime_util.h:13: namespace media {
On 2015/05/08 16:29:18, ddorwin wrote:
> I'm assuming everything below was copied without changes, right?

Yes, the very first patchset was simple copy, in later patchsets there are only
necessary minor changes (like changing NET_EXPORT to MEDIA_EXPORT).

https://codereview.chromium.org/1129643002/diff/200001/media/base/mime_util.h...
media/base/mime_util.h:15: // Check to see if a particular MIME type is in the
list of
On 2015/05/08 16:29:18, ddorwin wrote:
> In a future CL, we need to update these comments to "Returns..."

Yeah, and since it lives under media/ now, it will be much easier to do.

https://codereview.chromium.org/1129643002/diff/200001/net/base/mime_util_uni...
File net/base/mime_util_unittest.cc (right):

https://codereview.chromium.org/1129643002/diff/200001/net/base/mime_util_uni...
net/base/mime_util_unittest.cc:24: #if defined(OS_ANDROID)
On 2015/05/08 16:29:19, ddorwin wrote:
> #include "build/build_config.h"

Done.

[ddorwin, 2015-05-09 02:12:02]

One missed include.

Other than that, LGTM.

https://codereview.chromium.org/1129643002/diff/220001/content/browser/frame_...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1129643002/diff/220001/content/browser/frame_...
content/browser/frame_host/navigation_controller_impl.cc:825: if
(!base::CommandLine::ForCurrentProcess()->HasSwitch(
In the future, if you have rebased, please upload the rebase as a separate patch
set then make changes. Thanks.

https://codereview.chromium.org/1129643002/diff/220001/content/renderer/rende...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/1129643002/diff/220001/content/renderer/rende...
content/renderer/renderer_blink_platform_impl.cc:87: #if defined(OS_ANDROID)
#include "build/build_config.h"

[servolk, 2015-05-11 19:30:49]

https://codereview.chromium.org/1129643002/diff/220001/content/renderer/rende...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/1129643002/diff/220001/content/renderer/rende...
content/renderer/renderer_blink_platform_impl.cc:87: #if defined(OS_ANDROID)
On 2015/05/09 02:12:02, ddorwin wrote:
> #include "build/build_config.h"

Done.

[jochen (gone - plz use gerrit), 2015-05-11 23:37:40]

lgtm

[servolk, 2015-05-12 01:13:07]

The CQ bit was checked by servolk@chromium.org

[servolk, 2015-05-12 01:13:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
rsleevi@chromium.org, ddorwin@chromium.org
Link to the patchset: https://codereview.chromium.org/1129643002/#ps240001
(title: "Added missing #include build_config.h")

[commit-bot: I haz the power, 2015-05-12 01:15:28]

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

[commit-bot: I haz the power, 2015-05-12 01:42:42]

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

[commit-bot: I haz the power, 2015-05-12 01:42:44]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[sdefresne, 2015-05-12 08:45:01]

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

[sdefresne, 2015-05-12 08:45:02]

not lgtm as iOS depends on //components/mime_util and cannot depends on //media

https://codereview.chromium.org/1129643002/diff/240001/components/mime_util/m...
File components/mime_util/mime_util.gyp (right):

https://codereview.chromium.org/1129643002/diff/240001/components/mime_util/m...
components/mime_util/mime_util.gyp:13: '../../media/media.gyp:media',
iOS currently builds mime_util.gyp but does not build //media so this dependency
will break compilation of iOS (as you've seen with your try bot failures).

I tried to remove the dependency and found that iOS do use
mime_util::IsSupportedImageMimeType and mime_util::IsSupportedJavascriptMimeType
so please make this dependency conditional on OS!=ios (and protect the code with
#if !defined(OS_IOS)).

I've not looked at the rest of the CL to check whether this would be possible or
not.

[sdefresne, 2015-05-12 08:45:21]

sdefresne@chromium.org changed reviewers:
+ blundell@chromium.org

[Ryan Sleevi, 2015-05-12 16:13:04]

On 2015/05/12 08:45:02, sdefresne wrote:
> not lgtm as iOS depends on //components/mime_util and cannot depends on
//media
> 

Where is that codified? //media already has build configurations for iOS and
does build there (as shown by build logs)

Could you explain more about the rationale and requirements to getting this in
to //media? I realized you offered conditional compilation, but I don't really
think that represents a good solution.

This code needs to get out of //net, for sure. I'm trying to get a further
picture for what our options are (please be as detailed as possible, given the
round trip latencies)

[Ryan Sleevi, 2015-05-12 16:13:07]

On 2015/05/12 08:45:02, sdefresne wrote:
> not lgtm as iOS depends on //components/mime_util and cannot depends on
//media
> 

Where is that codified? //media already has build configurations for iOS and
does build there (as shown by build logs)

Could you explain more about the rationale and requirements to getting this in
to //media? I realized you offered conditional compilation, but I don't really
think that represents a good solution.

This code needs to get out of //net, for sure. I'm trying to get a further
picture for what our options are (please be as detailed as possible, given the
round trip latencies)

[sdefresne, 2015-05-12 16:30:05]

On 2015/05/12 16:13:07, Ryan Sleevi wrote:
> On 2015/05/12 08:45:02, sdefresne wrote:
> > not lgtm as iOS depends on //components/mime_util and cannot depends on
> //media
> > 
> 
> Where is that codified? //media already has build configurations for iOS and
> does build there (as shown by build logs)

//media is not build on iOS neither downstream nor upstream (at least it was not
building before this patch introduced the dependency). This can be verified
using ninja:
$ ninja -C out/Debug-iphonesimulator -t graph|grep 'media/.*cc'
"0x10414fb30" [label="../../content/common/media/media_stream_options.cc"]

Can you point to the build log showing otherwise (apart from failure from this
patch since it is the one introducing the dependency)?

> Could you explain more about the rationale and requirements to getting this in
> to //media? I realized you offered conditional compilation, but I don't really
> think that represents a good solution.
> 
> This code needs to get out of //net, for sure. I'm trying to get a further
> picture for what our options are (please be as detailed as possible, given the
> round trip latencies)

I think there is a misunderstanding as I'm not suggesting moving the code into
//media.

The CL created by servolk is trying to move the method from
//net/base/mime_util.{h,cc} to //media/mime_util.{h,cc}. I'm saying that this
will break iOS as iOS uses //components/mime_util but does not currently build
//media (and if possible we'd like to avoid building it).

I understand that conditional compilation is not possible (I've looked more into
the code). If the code cannot live in //net, why not move the method
IsSupportedMediaMimeType into the component then? //content already depends on
//components/mime_util so it shouldn't be a problem.

[servolk, 2015-05-12 16:49:03]

On 2015/05/12 16:30:05, sdefresne wrote:
> On 2015/05/12 16:13:07, Ryan Sleevi wrote:
> > On 2015/05/12 08:45:02, sdefresne wrote:
> > > not lgtm as iOS depends on //components/mime_util and cannot depends on
> > //media
> > > 
> > 
> > Where is that codified? //media already has build configurations for iOS and
> > does build there (as shown by build logs)
> 
> //media is not build on iOS neither downstream nor upstream (at least it was
not
> building before this patch introduced the dependency). This can be verified
> using ninja:
> $ ninja -C out/Debug-iphonesimulator -t graph|grep 'media/.*cc'
> "0x10414fb30" [label="../../content/common/media/media_stream_options.cc"]
> 
> Can you point to the build log showing otherwise (apart from failure from this
> patch since it is the one introducing the dependency)?
> 
> > Could you explain more about the rationale and requirements to getting this
in
> > to //media? I realized you offered conditional compilation, but I don't
really
> > think that represents a good solution.
> > 
> > This code needs to get out of //net, for sure. I'm trying to get a further
> > picture for what our options are (please be as detailed as possible, given
the
> > round trip latencies)
> 
> I think there is a misunderstanding as I'm not suggesting moving the code into
> //media.
> 
> The CL created by servolk is trying to move the method from
> //net/base/mime_util.{h,cc} to //media/mime_util.{h,cc}. I'm saying that this
> will break iOS as iOS uses //components/mime_util but does not currently build
> //media (and if possible we'd like to avoid building it).
> 
> I understand that conditional compilation is not possible (I've looked more
into
> the code). If the code cannot live in //net, why not move the method
> IsSupportedMediaMimeType into the component then? //content already depends on
> //components/mime_util so it shouldn't be a problem.

I assumed that media was built and used on iOS, since I've noticed the comment
at
https://code.google.com/p/chromium/codesearch#chromium/src/media/media.gyp&l=16
and noticed that iOS was explicitly handled in media.gyp in a few places. Now
I've started digging deeper, and looks like only media_for_cast_ios target is
used on iOS. But that's only a very small subset of media. Should we make a
separate media subtarget for iOS that would contain only media/base/mime_util.*
for now? Or perhaps we could make media_base target build on iOS (shouldn't be
too hard) and then make the media_base subtarget public, so that
//component/media_base could depend only on it, but not on the larger media
target (I've tried that in patchset #8 here, but had to revert that change,
since media_base target is currently internal for //media).

[sdefresne, 2015-05-12 17:12:21]

On 2015/05/12 16:49:03, servolk wrote:
> On 2015/05/12 16:30:05, sdefresne wrote:
> > On 2015/05/12 16:13:07, Ryan Sleevi wrote:
> > > On 2015/05/12 08:45:02, sdefresne wrote:
> > > > not lgtm as iOS depends on //components/mime_util and cannot depends on
> > > //media
> > > > 
> > > 
> > > Where is that codified? //media already has build configurations for iOS
and
> > > does build there (as shown by build logs)
> > 
> > //media is not build on iOS neither downstream nor upstream (at least it was
> not
> > building before this patch introduced the dependency). This can be verified
> > using ninja:
> > $ ninja -C out/Debug-iphonesimulator -t graph|grep 'media/.*cc'
> > "0x10414fb30" [label="../../content/common/media/media_stream_options.cc"]
> > 
> > Can you point to the build log showing otherwise (apart from failure from
this
> > patch since it is the one introducing the dependency)?
> > 
> > > Could you explain more about the rationale and requirements to getting
this
> in
> > > to //media? I realized you offered conditional compilation, but I don't
> really
> > > think that represents a good solution.
> > > 
> > > This code needs to get out of //net, for sure. I'm trying to get a further
> > > picture for what our options are (please be as detailed as possible, given
> the
> > > round trip latencies)
> > 
> > I think there is a misunderstanding as I'm not suggesting moving the code
into
> > //media.
> > 
> > The CL created by servolk is trying to move the method from
> > //net/base/mime_util.{h,cc} to //media/mime_util.{h,cc}. I'm saying that
this
> > will break iOS as iOS uses //components/mime_util but does not currently
build
> > //media (and if possible we'd like to avoid building it).
> > 
> > I understand that conditional compilation is not possible (I've looked more
> into
> > the code). If the code cannot live in //net, why not move the method
> > IsSupportedMediaMimeType into the component then? //content already depends
on
> > //components/mime_util so it shouldn't be a problem.
> 
> I assumed that media was built and used on iOS, since I've noticed the comment
> at
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/media.gyp&l=16
> and noticed that iOS was explicitly handled in media.gyp in a few places. Now
> I've started digging deeper, and looks like only media_for_cast_ios target is
> used on iOS. But that's only a very small subset of media. Should we make a
> separate media subtarget for iOS that would contain only
media/base/mime_util.*
> for now? Or perhaps we could make media_base target build on iOS (shouldn't be
> too hard) and then make the media_base subtarget public, so that
> //component/media_base could depend only on it, but not on the larger media
> target (I've tried that in patchset #8 here, but had to revert that change,
> since media_base target is currently internal for //media).

I don't know why media.gyp is mentioning iOS. AFAIK, media_for_cast_ios is not
used on iOS downstream/upstream. jfroy@ added this target (in
https://codereview.chromium.org/581803003) so maybe he has more knowledge about
building //media on iOS. Maybe this is used by WebP project?

I don't really mind if a new target is added to //media, I still think it
strange to have a media_mime_util target that only export method related to mime
(especially after looking in //media/OWNERS and seeing that //media is mostly
interested in video/cast/audio).

Are those methods used in //media? Why not move them to //components/mime_util?
AFAICT the methods are only used by //content and html_viewer that are both
allowed to use that component so it should not be a problem. Am I missing
something?

[jfroy, 2015-05-12 17:25:26]

On 2015/05/12 17:12:21, sdefresne wrote:
> On 2015/05/12 16:49:03, servolk wrote:
> > On 2015/05/12 16:30:05, sdefresne wrote:
> > > On 2015/05/12 16:13:07, Ryan Sleevi wrote:
> > > > On 2015/05/12 08:45:02, sdefresne wrote:
> > > > > not lgtm as iOS depends on //components/mime_util and cannot depends
on
> > > > //media
> > > > > 
> > > > 
> > > > Where is that codified? //media already has build configurations for iOS
> and
> > > > does build there (as shown by build logs)
> > > 
> > > //media is not build on iOS neither downstream nor upstream (at least it
was
> > not
> > > building before this patch introduced the dependency). This can be
verified
> > > using ninja:
> > > $ ninja -C out/Debug-iphonesimulator -t graph|grep 'media/.*cc'
> > > "0x10414fb30" [label="../../content/common/media/media_stream_options.cc"]
> > > 
> > > Can you point to the build log showing otherwise (apart from failure from
> this
> > > patch since it is the one introducing the dependency)?
> > > 
> > > > Could you explain more about the rationale and requirements to getting
> this
> > in
> > > > to //media? I realized you offered conditional compilation, but I don't
> > really
> > > > think that represents a good solution.
> > > > 
> > > > This code needs to get out of //net, for sure. I'm trying to get a
further
> > > > picture for what our options are (please be as detailed as possible,
given
> > the
> > > > round trip latencies)
> > > 
> > > I think there is a misunderstanding as I'm not suggesting moving the code
> into
> > > //media.
> > > 
> > > The CL created by servolk is trying to move the method from
> > > //net/base/mime_util.{h,cc} to //media/mime_util.{h,cc}. I'm saying that
> this
> > > will break iOS as iOS uses //components/mime_util but does not currently
> build
> > > //media (and if possible we'd like to avoid building it).
> > > 
> > > I understand that conditional compilation is not possible (I've looked
more
> > into
> > > the code). If the code cannot live in //net, why not move the method
> > > IsSupportedMediaMimeType into the component then? //content already
depends
> on
> > > //components/mime_util so it shouldn't be a problem.
> > 
> > I assumed that media was built and used on iOS, since I've noticed the
comment
> > at
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/media.gyp&l=16
> > and noticed that iOS was explicitly handled in media.gyp in a few places.
Now
> > I've started digging deeper, and looks like only media_for_cast_ios target
is
> > used on iOS. But that's only a very small subset of media. Should we make a
> > separate media subtarget for iOS that would contain only
> media/base/mime_util.*
> > for now? Or perhaps we could make media_base target build on iOS (shouldn't
be
> > too hard) and then make the media_base subtarget public, so that
> > //component/media_base could depend only on it, but not on the larger media
> > target (I've tried that in patchset #8 here, but had to revert that change,
> > since media_base target is currently internal for //media).
> 
> I don't know why media.gyp is mentioning iOS. AFAIK, media_for_cast_ios is not
> used on iOS downstream/upstream. jfroy@ added this target (in
> https://codereview.chromium.org/581803003) so maybe he has more knowledge
about
> building //media on iOS. Maybe this is used by WebP project?
> 
> I don't really mind if a new target is added to //media, I still think it
> strange to have a media_mime_util target that only export method related to
mime
> (especially after looking in //media/OWNERS and seeing that //media is mostly
> interested in video/cast/audio).
> 
> Are those methods used in //media? Why not move them to
//components/mime_util?
> AFAICT the methods are only used by //content and html_viewer that are both
> allowed to use that component so it should not be a problem. Am I missing
> something?

media_for_cast_ios is used to build the Google Cast SDK for iOS. As was
mentioned already, //media doesn't build for iOS (mostly because gyp is not very
good at dealing with arch-specific assembly files and the duality of iOS using
Intel archs for the simulator and ARM archs for hardware). In addition, //media
is an overwhelming dependency for what //media/cast really needs. So I
determined the minimal dependency graph and create that media subset target.

tl;dr Don't make iOS depend on //media.

[DaleCurtis, 2015-05-12 17:25:39]

I'm unclear on the current state, but we definitely use to build media/ for iOS;
it's slowly been whittled away over the years though.

I don't see any reason why we couldn't go back to having a iOS only media target
which say only contains things like the mime type converts.

[jfroy, 2015-05-12 17:28:57]

On 2015/05/12 17:25:39, DaleCurtis wrote:
> I'm unclear on the current state, but we definitely use to build media/ for
iOS;
> it's slowly been whittled away over the years though.
> 
> I don't see any reason why we couldn't go back to having a iOS only media
target
> which say only contains things like the mime type converts.

My guess is that there is no plan on fixing the gyp build. All our future hopes
rest on gn doing a better job at conditioning the build for the strange world of
iOS sim / hardware builds. But yeah, defining a media subset for iOS seems like
a reasonable thing to do as well. I know that Chrome for iOS can't really use
//media anyways because they rely on the system web view, but they do use //net
(as do other Google apps for iOS).

[servolk, 2015-05-12 20:24:22]

On 2015/05/12 17:28:57, jfroy wrote:
> On 2015/05/12 17:25:39, DaleCurtis wrote:
> > I'm unclear on the current state, but we definitely use to build media/ for
> iOS;
> > it's slowly been whittled away over the years though.
> > 
> > I don't see any reason why we couldn't go back to having a iOS only media
> target
> > which say only contains things like the mime type converts.
> 
> My guess is that there is no plan on fixing the gyp build. All our future
hopes
> rest on gn doing a better job at conditioning the build for the strange world
of
> iOS sim / hardware builds. But yeah, defining a media subset for iOS seems
like
> a reasonable thing to do as well. I know that Chrome for iOS can't really use
> //media anyways because they rely on the system web view, but they do use
//net
> (as do other Google apps for iOS).

Introducing a new separate subtarget has some additional costs (e.g. we'd need
to make sure all the configuration settings, like 'proprietary_codecs' and
'media_use_ffmpeg' flags, from the main media target are properly
handled/applied in the new subtarget, since those affect the set of supported
containers and codecs). So how about we just fix media compilation on iOS? Turns
out this really simple change is sufficient for that:
https://codereview.chromium.org/1140553004/
I've tried it and it seems to be working fine on ios_rel_device_ninja trybot
even when the media target is built on iOS (which currently doesn't happen by
default), see https://codereview.chromium.org/1123343007/ where I applied CL
1140553004 on top of the latest patchset from here.

[servolk, 2015-05-12 20:31:19]

On 2015/05/12 20:24:22, servolk wrote:
> On 2015/05/12 17:28:57, jfroy wrote:
> > On 2015/05/12 17:25:39, DaleCurtis wrote:
> > > I'm unclear on the current state, but we definitely use to build media/
for
> > iOS;
> > > it's slowly been whittled away over the years though.
> > > 
> > > I don't see any reason why we couldn't go back to having a iOS only media
> > target
> > > which say only contains things like the mime type converts.
> > 
> > My guess is that there is no plan on fixing the gyp build. All our future
> hopes
> > rest on gn doing a better job at conditioning the build for the strange
world
> of
> > iOS sim / hardware builds. But yeah, defining a media subset for iOS seems
> like
> > a reasonable thing to do as well. I know that Chrome for iOS can't really
use
> > //media anyways because they rely on the system web view, but they do use
> //net
> > (as do other Google apps for iOS).
> 
> Introducing a new separate subtarget has some additional costs (e.g. we'd need
> to make sure all the configuration settings, like 'proprietary_codecs' and
> 'media_use_ffmpeg' flags, from the main media target are properly
> handled/applied in the new subtarget, since those affect the set of supported
> containers and codecs). So how about we just fix media compilation on iOS?
Turns
> out this really simple change is sufficient for that:
> https://codereview.chromium.org/1140553004/
> I've tried it and it seems to be working fine on ios_rel_device_ninja trybot
> even when the media target is built on iOS (which currently doesn't happen by
> default), see https://codereview.chromium.org/1123343007/ where I applied CL
> 1140553004 on top of the latest patchset from here.

Although, if we really want to avoid building //media on iOS then moving
mime_util to a separate component might be a better solution after all. But
could somebody more familiar with media (Dale?) comment on if we want to
eventually use media on iOS as well, or are we planning to never use any of the
media stuff on iOS?

[blundell, 2015-05-12 20:35:32]

On 2015/05/12 20:31:19, servolk wrote:
> On 2015/05/12 20:24:22, servolk wrote:
> > On 2015/05/12 17:28:57, jfroy wrote:
> > > On 2015/05/12 17:25:39, DaleCurtis wrote:
> > > > I'm unclear on the current state, but we definitely use to build media/
> for
> > > iOS;
> > > > it's slowly been whittled away over the years though.
> > > > 
> > > > I don't see any reason why we couldn't go back to having a iOS only
media
> > > target
> > > > which say only contains things like the mime type converts.
> > > 
> > > My guess is that there is no plan on fixing the gyp build. All our future
> > hopes
> > > rest on gn doing a better job at conditioning the build for the strange
> world
> > of
> > > iOS sim / hardware builds. But yeah, defining a media subset for iOS seems
> > like
> > > a reasonable thing to do as well. I know that Chrome for iOS can't really
> use
> > > //media anyways because they rely on the system web view, but they do use
> > //net
> > > (as do other Google apps for iOS).
> > 
> > Introducing a new separate subtarget has some additional costs (e.g. we'd
need
> > to make sure all the configuration settings, like 'proprietary_codecs' and
> > 'media_use_ffmpeg' flags, from the main media target are properly
> > handled/applied in the new subtarget, since those affect the set of
supported
> > containers and codecs). So how about we just fix media compilation on iOS?
> Turns
> > out this really simple change is sufficient for that:
> > https://codereview.chromium.org/1140553004/
> > I've tried it and it seems to be working fine on ios_rel_device_ninja trybot
> > even when the media target is built on iOS (which currently doesn't happen
by
> > default), see https://codereview.chromium.org/1123343007/ where I applied CL
> > 1140553004 on top of the latest patchset from here.
> 
> Although, if we really want to avoid building //media on iOS then moving
> mime_util to a separate component might be a better solution after all. But
> could somebody more familiar with media (Dale?) comment on if we want to
> eventually use media on iOS as well, or are we planning to never use any of
the
> media stuff on iOS?

//media was very explicitly removed from the iOS build:

https://chromiumcodereview.appspot.com/18873009

[servolk, 2015-05-12 20:38:33]

On 2015/05/12 20:35:32, blundell wrote:
> On 2015/05/12 20:31:19, servolk wrote:
> > On 2015/05/12 20:24:22, servolk wrote:
> > > On 2015/05/12 17:28:57, jfroy wrote:
> > > > On 2015/05/12 17:25:39, DaleCurtis wrote:
> > > > > I'm unclear on the current state, but we definitely use to build
media/
> > for
> > > > iOS;
> > > > > it's slowly been whittled away over the years though.
> > > > > 
> > > > > I don't see any reason why we couldn't go back to having a iOS only
> media
> > > > target
> > > > > which say only contains things like the mime type converts.
> > > > 
> > > > My guess is that there is no plan on fixing the gyp build. All our
future
> > > hopes
> > > > rest on gn doing a better job at conditioning the build for the strange
> > world
> > > of
> > > > iOS sim / hardware builds. But yeah, defining a media subset for iOS
seems
> > > like
> > > > a reasonable thing to do as well. I know that Chrome for iOS can't
really
> > use
> > > > //media anyways because they rely on the system web view, but they do
use
> > > //net
> > > > (as do other Google apps for iOS).
> > > 
> > > Introducing a new separate subtarget has some additional costs (e.g. we'd
> need
> > > to make sure all the configuration settings, like 'proprietary_codecs' and
> > > 'media_use_ffmpeg' flags, from the main media target are properly
> > > handled/applied in the new subtarget, since those affect the set of
> supported
> > > containers and codecs). So how about we just fix media compilation on iOS?
> > Turns
> > > out this really simple change is sufficient for that:
> > > https://codereview.chromium.org/1140553004/
> > > I've tried it and it seems to be working fine on ios_rel_device_ninja
trybot
> > > even when the media target is built on iOS (which currently doesn't happen
> by
> > > default), see https://codereview.chromium.org/1123343007/ where I applied
CL
> > > 1140553004 on top of the latest patchset from here.
> > 
> > Although, if we really want to avoid building //media on iOS then moving
> > mime_util to a separate component might be a better solution after all. But
> > could somebody more familiar with media (Dale?) comment on if we want to
> > eventually use media on iOS as well, or are we planning to never use any of
> the
> > media stuff on iOS?
> 
> //media was very explicitly removed from the iOS build:
> 
> https://chromiumcodereview.appspot.com/18873009

Ah, ok, in that case having a separate component is probably our only option.

[Ryan Sleevi, 2015-05-13 02:20:38]

On 2015/05/12 20:38:33, servolk wrote:
> Ah, ok, in that case having a separate component is probably our only option.

Or making the dependency conditional on !iOS.

For example, do any of the methods we're moving to //media even make sense in an
iOS world? Someone from the //media team would need to answer, but I suspect
"No"?

The behaviour for non-iOS embedders should be the same, since iOS doesn't use
//content (of course, this could also be a horrible misstatement), and the
behaviour for iOS would be... idempotent, since no iOS code was harmed in the
making of the previous CL?

[ddorwin, 2015-05-13 03:06:39]

What are the non-media (and non-content?) consumers of the callers of the media
code in components/?

The primary use of the media code is canPlayType()/MSE isTypeSupported()/EME,
but those are all provided by the WebView on iOS. Thus, it is those few other
uses that might be relevant. For example, is some portion used to deal with MIME
types in the network stack? Do we use that code on iOS?

[Ryan Sleevi, 2015-05-13 03:28:37]

On 2015/05/13 03:06:39, ddorwin wrote:
> What are the non-media (and non-content?) consumers of the callers of the
media
> code in components/?

That's why I asked - all the core consumers *are* in //content and used to
implement the Blink embedder APIs. Thus, it seems fine not to have this code on
iOS and not to call it on iOS.

> The primary use of the media code is canPlayType()/MSE isTypeSupported()/EME,
> but those are all provided by the WebView on iOS. Thus, it is those few other
> uses that might be relevant. For example, is some portion used to deal with
MIME
> types in the network stack? Do we use that code on iOS?

Not in the //net stack, not anymore.

The indirect dependencies are those that depend on either
mime_util::IsSupportedMimeType or mime_util::IsSupportedNonImageMimeType

mime_util::IsSupportedMimeType is only used by those implementing the Blink
bindings or //content (with the //content one being "weird")
mime_util::IsSupportedNonImageMimeType is the same

So that's why we should be fine if both of those return "false" for the //media
mime types on iOS, because they're never called on iOS.

[ddorwin, 2015-05-13 03:52:41]

On 2015/05/13 03:28:37, Ryan Sleevi wrote:
> On 2015/05/13 03:06:39, ddorwin wrote:
> > What are the non-media (and non-content?) consumers of the callers of the
> media
> > code in components/?
> 
> That's why I asked - all the core consumers *are* in //content and used to
> implement the Blink embedder APIs. Thus, it seems fine not to have this code
on
> iOS and not to call it on iOS.
> 
> > The primary use of the media code is canPlayType()/MSE
isTypeSupported()/EME,
> > but those are all provided by the WebView on iOS. Thus, it is those few
other
> > uses that might be relevant. For example, is some portion used to deal with
> MIME
> > types in the network stack? Do we use that code on iOS?
> 
> Not in the //net stack, not anymore.
> 
> The indirect dependencies are those that depend on either
> mime_util::IsSupportedMimeType or mime_util::IsSupportedNonImageMimeType
> 
> mime_util::IsSupportedMimeType is only used by those implementing the Blink
> bindings or //content (with the //content one being "weird")
> mime_util::IsSupportedNonImageMimeType is the same
> 
> So that's why we should be fine if both of those return "false" for the
//media
> mime types on iOS, because they're never called on iOS.

Thanks for clarifying. That SG.

It looks like IsSupportedMimeType() is called from several places in chrome/
(https://code.google.com/p/chromium/codesearch#search/&q=IsSupportedMimeType%2...),
but maybe those aren't compiled on iOS*. It sounds like maybe not from
sdefresne's original comment.

We can add NOTREACHED() for iOS to all places where we call media:: then only
add the build dependency for platforms other than iOS. I don't know what we can
do for the DEPS file other than add a comment that code that compiles on iOS
must not use media/.

* If they are compiled, I don't know what currently happens, but there should
probably be an iOS-specific path that matches the WebView capabilities. It's
probably the case that we're currently mostly correct for branding=Chrome. But
we probably report the wrong things for HLS, VP9, etc. to those callers in
chrome/.

[sdefresne, 2015-05-13 08:31:59]

On 2015/05/13 03:52:41, ddorwin wrote:
> On 2015/05/13 03:28:37, Ryan Sleevi wrote:
> > On 2015/05/13 03:06:39, ddorwin wrote:
> > > What are the non-media (and non-content?) consumers of the callers of the
> > media
> > > code in components/?
> > 
> > That's why I asked - all the core consumers *are* in //content and used to
> > implement the Blink embedder APIs. Thus, it seems fine not to have this code
> on
> > iOS and not to call it on iOS.
> > 
> > > The primary use of the media code is canPlayType()/MSE
> isTypeSupported()/EME,
> > > but those are all provided by the WebView on iOS. Thus, it is those few
> other
> > > uses that might be relevant. For example, is some portion used to deal
with
> > MIME
> > > types in the network stack? Do we use that code on iOS?
> > 
> > Not in the //net stack, not anymore.
> > 
> > The indirect dependencies are those that depend on either
> > mime_util::IsSupportedMimeType or mime_util::IsSupportedNonImageMimeType
> > 
> > mime_util::IsSupportedMimeType is only used by those implementing the Blink
> > bindings or //content (with the //content one being "weird")
> > mime_util::IsSupportedNonImageMimeType is the same
> > 
> > So that's why we should be fine if both of those return "false" for the
> //media
> > mime types on iOS, because they're never called on iOS.
> 
> Thanks for clarifying. That SG.

SGTM too.

> It looks like IsSupportedMimeType() is called from several places in chrome/
>
(https://code.google.com/p/chromium/codesearch#search/&q=IsSupportedMimeType%2...),
> but maybe those aren't compiled on iOS*. It sounds like maybe not from
> sdefresne's original comment.

Those call sites are not compiled on iOS. I think the only call site that is
currently calling mime_util on iOS downstream is
ResourcePrefetchPredictor::IsHandledSubresource() [this is in chrome/ and is
compiled in the downstream fork we are in the process of unforking but will take
some time]. AFAICT this code path is not exercised on iOS so I think we should
be fine if mime_util component were not even compiled on iOS.

So I think that marking as the dependency on //components/mime_util behind
OS!=ios should be enough. Marking as LGTM to unblock you provided you don't
bring a new dependency on //media on iOS. Thank you.

> We can add NOTREACHED() for iOS to all places where we call media:: then only
> add the build dependency for platforms other than iOS. I don't know what we
can
> do for the DEPS file other than add a comment that code that compiles on iOS
> must not use media/.
> 
> * If they are compiled, I don't know what currently happens, but there should
> probably be an iOS-specific path that matches the WebView capabilities. It's
> probably the case that we're currently mostly correct for branding=Chrome. But
> we probably report the wrong things for HLS, VP9, etc. to those callers in
> chrome/.

[servolk, 2015-05-13 18:34:06]

On 2015/05/13 08:31:59, sdefresne wrote:
> On 2015/05/13 03:52:41, ddorwin wrote:
> > On 2015/05/13 03:28:37, Ryan Sleevi wrote:
> > > On 2015/05/13 03:06:39, ddorwin wrote:
> > > > What are the non-media (and non-content?) consumers of the callers of
the
> > > media
> > > > code in components/?
> > > 
> > > That's why I asked - all the core consumers *are* in //content and used to
> > > implement the Blink embedder APIs. Thus, it seems fine not to have this
code
> > on
> > > iOS and not to call it on iOS.
> > > 
> > > > The primary use of the media code is canPlayType()/MSE
> > isTypeSupported()/EME,
> > > > but those are all provided by the WebView on iOS. Thus, it is those few
> > other
> > > > uses that might be relevant. For example, is some portion used to deal
> with
> > > MIME
> > > > types in the network stack? Do we use that code on iOS?
> > > 
> > > Not in the //net stack, not anymore.
> > > 
> > > The indirect dependencies are those that depend on either
> > > mime_util::IsSupportedMimeType or mime_util::IsSupportedNonImageMimeType
> > > 
> > > mime_util::IsSupportedMimeType is only used by those implementing the
Blink
> > > bindings or //content (with the //content one being "weird")
> > > mime_util::IsSupportedNonImageMimeType is the same
> > > 
> > > So that's why we should be fine if both of those return "false" for the
> > //media
> > > mime types on iOS, because they're never called on iOS.
> > 
> > Thanks for clarifying. That SG.
> 
> SGTM too.
> 
> > It looks like IsSupportedMimeType() is called from several places in chrome/
> >
>
(https://code.google.com/p/chromium/codesearch#search/&q=IsSupportedMimeType%2...),
> > but maybe those aren't compiled on iOS*. It sounds like maybe not from
> > sdefresne's original comment.
> 
> Those call sites are not compiled on iOS. I think the only call site that is
> currently calling mime_util on iOS downstream is
> ResourcePrefetchPredictor::IsHandledSubresource() [this is in chrome/ and is
> compiled in the downstream fork we are in the process of unforking but will
take
> some time]. AFAICT this code path is not exercised on iOS so I think we should
> be fine if mime_util component were not even compiled on iOS.
> 
> So I think that marking as the dependency on //components/mime_util behind
> OS!=ios should be enough. Marking as LGTM to unblock you provided you don't
> bring a new dependency on //media on iOS. Thank you.
> 
> > We can add NOTREACHED() for iOS to all places where we call media:: then
only
> > add the build dependency for platforms other than iOS. I don't know what we
> can
> > do for the DEPS file other than add a comment that code that compiles on iOS
> > must not use media/.
> > 
> > * If they are compiled, I don't know what currently happens, but there
should
> > probably be an iOS-specific path that matches the WebView capabilities. It's
> > probably the case that we're currently mostly correct for branding=Chrome.
But
> > we probably report the wrong things for HLS, VP9, etc. to those callers in
> > chrome/.

Ok, thanks patchset #14 ensures //component/mime_util won't call
media::IsSupportedMediaMimeType
on iOS and makes //component/mime_util depend on //media only on OSes other than
iOS. PTAL.

[servolk, 2015-05-13 18:35:58]

https://codereview.chromium.org/1129643002/diff/240001/components/mime_util/m...
File components/mime_util/mime_util.gyp (right):

https://codereview.chromium.org/1129643002/diff/240001/components/mime_util/m...
components/mime_util/mime_util.gyp:13: '../../media/media.gyp:media',
On 2015/05/12 08:45:02, sdefresne wrote:
> iOS currently builds mime_util.gyp but does not build //media so this
dependency
> will break compilation of iOS (as you've seen with your try bot failures).
> 
> I tried to remove the dependency and found that iOS do use
> mime_util::IsSupportedImageMimeType and
mime_util::IsSupportedJavascriptMimeType
> so please make this dependency conditional on OS!=ios (and protect the code
with
> #if !defined(OS_IOS)).
> 
> I've not looked at the rest of the CL to check whether this would be possible
or
> not.

Done.

[Ryan Sleevi, 2015-05-13 23:55:53]

https://codereview.chromium.org/1129643002/diff/290001/components/BUILD.gn
File components/BUILD.gn (right):

https://codereview.chromium.org/1129643002/diff/290001/components/BUILD.gn#ne...
components/BUILD.gn:299: "//media",
You need to update this for iOS

https://codereview.chromium.org/1129643002/diff/290001/components/mime_util/m...
File components/mime_util/mime_util.cc (right):

https://codereview.chromium.org/1129643002/diff/290001/components/mime_util/m...
components/mime_util/mime_util.cc:11: #if !defined(OS_IOS)
newline between 10 & 11

https://www.chromium.org/developers/coding-style#TOC-Platform-specific-code

https://codereview.chromium.org/1129643002/diff/290001/media/blink/key_system...
File media/blink/key_system_config_selector.cc (right):

https://codereview.chromium.org/1129643002/diff/290001/media/blink/key_system...
media/blink/key_system_config_selector.cc:11: #include "build/build_config.h"
Unclear why this is added? no ifdefs?

[servolk, 2015-05-14 00:13:23]

https://codereview.chromium.org/1129643002/diff/290001/components/BUILD.gn
File components/BUILD.gn (right):

https://codereview.chromium.org/1129643002/diff/290001/components/BUILD.gn#ne...
components/BUILD.gn:299: "//media",
On 2015/05/13 23:55:53, Ryan Sleevi wrote:
> You need to update this for iOS

Done.

https://codereview.chromium.org/1129643002/diff/290001/components/mime_util/m...
File components/mime_util/mime_util.cc (right):

https://codereview.chromium.org/1129643002/diff/290001/components/mime_util/m...
components/mime_util/mime_util.cc:11: #if !defined(OS_IOS)
On 2015/05/13 23:55:53, Ryan Sleevi wrote:
> newline between 10 & 11
> 
> https://www.chromium.org/developers/coding-style#TOC-Platform-specific-code

Done.

https://codereview.chromium.org/1129643002/diff/290001/media/blink/key_system...
File media/blink/key_system_config_selector.cc (right):

https://codereview.chromium.org/1129643002/diff/290001/media/blink/key_system...
media/blink/key_system_config_selector.cc:11: #include "build/build_config.h"
On 2015/05/13 23:55:53, Ryan Sleevi wrote:
> Unclear why this is added? no ifdefs?

There was a comment by ddorwin@ in patchset #9 to add build_config.h here
(https://codereview.chromium.org/1129643002/diff/200001/media/blink/key_system...)
since this file used to have a few OS_ANDROID references, so I've added this.
Then a recent CL https://codereview.chromium.org/1124863005 removed all
OS_ANDROID from this file. I have rebased my patch and haven't noticed that this
file no longer need build_config.h. Removed.

[ddorwin, 2015-05-14 18:10:00]

A few questions. LG otherwise.

https://codereview.chromium.org/1129643002/diff/290002/components/BUILD.gn
File components/BUILD.gn (right):

https://codereview.chromium.org/1129643002/diff/290002/components/BUILD.gn#ne...
components/BUILD.gn:306: deps += [ "//media" ]
Why do we need this dependency at all here? We have it in c/mime_util/ too.

https://codereview.chromium.org/1129643002/diff/290002/components/mime_util/DEPS
File components/mime_util/DEPS (right):

https://codereview.chromium.org/1129643002/diff/290002/components/mime_util/D...
components/mime_util/DEPS:6: "+media/base",
Should we add a comment that this must not be used on iOS?

https://codereview.chromium.org/1129643002/diff/290002/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/1129643002/diff/290002/media/media.gyp#newcod...
media/media.gyp:1041: ['OS != "ios" and (target_arch=="ia32" or
target_arch=="x64")', {
Why is this necessary if iOS doesn't use the media target?

[servolk, 2015-05-14 22:30:49]

https://codereview.chromium.org/1129643002/diff/290002/components/BUILD.gn
File components/BUILD.gn (right):

https://codereview.chromium.org/1129643002/diff/290002/components/BUILD.gn#ne...
components/BUILD.gn:306: deps += [ "//media" ]
On 2015/05/14 18:09:59, ddorwin wrote:
> Why do we need this dependency at all here? We have it in c/mime_util/ too.

Probably not anymore. I've added this dependency in patchset #10 and that fixed
compilation on ios_dbg_simulator_ninja trybot. But now that mime_util doesn't
depend on //media on iOS that's probably unnecessary, I'll remove it and will
rerun trybots.

https://codereview.chromium.org/1129643002/diff/290002/components/mime_util/DEPS
File components/mime_util/DEPS (right):

https://codereview.chromium.org/1129643002/diff/290002/components/mime_util/D...
components/mime_util/DEPS:6: "+media/base",
On 2015/05/14 18:09:59, ddorwin wrote:
> Should we add a comment that this must not be used on iOS?

Done.

https://codereview.chromium.org/1129643002/diff/290002/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/1129643002/diff/290002/media/media.gyp#newcod...
media/media.gyp:1041: ['OS != "ios" and (target_arch=="ia32" or
target_arch=="x64")', {
On 2015/05/14 18:09:59, ddorwin wrote:
> Why is this necessary if iOS doesn't use the media target?

Strictly speaking this is unnecessary for this CL, so I'll remove it for now.
But if we are interested in making media buildable on iOS, then we might still
consider this separate CL https://codereview.chromium.org/1140553004/

[servolk, 2015-05-15 19:53:36]

On 2015/05/14 22:30:49, servolk wrote:
> https://codereview.chromium.org/1129643002/diff/290002/components/BUILD.gn
> File components/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1129643002/diff/290002/components/BUILD.gn#ne...
> components/BUILD.gn:306: deps += [ "//media" ]
> On 2015/05/14 18:09:59, ddorwin wrote:
> > Why do we need this dependency at all here? We have it in c/mime_util/ too.
> 
> Probably not anymore. I've added this dependency in patchset #10 and that
fixed
> compilation on ios_dbg_simulator_ninja trybot. But now that mime_util doesn't
> depend on //media on iOS that's probably unnecessary, I'll remove it and will
> rerun trybots.
> 
>
https://codereview.chromium.org/1129643002/diff/290002/components/mime_util/DEPS
> File components/mime_util/DEPS (right):
> 
>
https://codereview.chromium.org/1129643002/diff/290002/components/mime_util/D...
> components/mime_util/DEPS:6: "+media/base",
> On 2015/05/14 18:09:59, ddorwin wrote:
> > Should we add a comment that this must not be used on iOS?
> 
> Done.
> 
> https://codereview.chromium.org/1129643002/diff/290002/media/media.gyp
> File media/media.gyp (right):
> 
>
https://codereview.chromium.org/1129643002/diff/290002/media/media.gyp#newcod...
> media/media.gyp:1041: ['OS != "ios" and (target_arch=="ia32" or
> target_arch=="x64")', {
> On 2015/05/14 18:09:59, ddorwin wrote:
> > Why is this necessary if iOS doesn't use the media target?
> 
> Strictly speaking this is unnecessary for this CL, so I'll remove it for now.
> But if we are interested in making media buildable on iOS, then we might still
> consider this separate CL https://codereview.chromium.org/1140553004/

Ping, I think I've addressed all CR feedback here. Ryan/David, do you want to
take one final look or is it ok to check in now?

[ddorwin, 2015-05-15 21:10:34]

LGTM. Thanks.

[Ryan Sleevi, 2015-05-15 21:30:32]

LGTM % 1 nit

https://codereview.chromium.org/1129643002/diff/310001/components/mime_util/DEPS
File components/mime_util/DEPS (right):

https://codereview.chromium.org/1129643002/diff/310001/components/mime_util/D...
components/mime_util/DEPS:6: "+media/base",  # Only for platforms other than iOS
Can't you make this a specific header file?

[servolk, 2015-05-15 22:10:31]

https://codereview.chromium.org/1129643002/diff/310001/components/mime_util/DEPS
File components/mime_util/DEPS (right):

https://codereview.chromium.org/1129643002/diff/310001/components/mime_util/D...
components/mime_util/DEPS:6: "+media/base",  # Only for platforms other than iOS
On 2015/05/15 21:30:32, Ryan Sleevi wrote:
> Can't you make this a specific header file?

Done.

[servolk, 2015-05-15 22:10:45]

The CQ bit was checked by servolk@chromium.org

[servolk, 2015-05-15 22:10:46]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
sdefresne@chromium.org, rsleevi@chromium.org, ddorwin@chromium.org
Link to the patchset: https://codereview.chromium.org/1129643002/#ps330001
(title: "Use media/base/mime_util.h in //components/mime_util/DEPS")

[commit-bot: I haz the power, 2015-05-15 22:12:46]

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

[commit-bot: I haz the power, 2015-05-16 00:02:19]

Committed patchset #19 (id:330001)

[commit-bot: I haz the power, 2015-05-18 11:29:30]

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