Move c/b/media/router/mojo/*.mojom to chrome/common/media_router/mojo/*.mojom

Move c/b/media/router/mojo/*.mojom to chrome/common/media_router/mojo/*.mojom

We have a file in extensions/ [1] that depends on Media Router mojo definitions
from chrome/browser/. To remove this dependency from extensions/ to
chrome/browser/, this CL moves that file to chrome/renderer/, and Media Router
mojo interfaces to chrome/common/media_router/mojo/. Media Router typemaps and
related C++ structs are also moved to chrome/common/media_router/.

[1] extensions/renderer/resources/media_router_bindings.js

BUG=704958
Review-Url: https://codereview.chromium.org/2771413003
Cr-Commit-Position: refs/heads/master@{#465734}
Committed: https://chromium.googlesource.com/chromium/src/+/a34e5193d3994da1e822b7259462ab3c67a14467

[takumif, 2017-03-27 22:23:53]

Description was changed from

==========
Move c/b/media/router/mojo/*.mojom to media/mojo/interfaces/*.mojom

BUG=704958
==========

to

==========
Move c/b/media/router/mojo/*.mojom to media/mojo/interfaces/*.mojom

We have a file in extensions/ [1] that depends on Media Router mojo definitions
from chrome/browser/. To remove this dependency from extensions/ to
chrome/browser/, this CL moves Media Router mojo interfaces to
media/mojo/interfaces/.

In a later CL, Media Router typemaps and related C++ structs will also be moved
to media/mojo/router/, given they don't depend on chrome/ or content/.

[1] extensions/renderer/resources/media_router_bindings.js

BUG=704958
==========

[takumif, 2017-03-27 22:24:43]

Description was changed from

==========
Move c/b/media/router/mojo/*.mojom to media/mojo/interfaces/*.mojom

We have a file in extensions/ [1] that depends on Media Router mojo definitions
from chrome/browser/. To remove this dependency from extensions/ to
chrome/browser/, this CL moves Media Router mojo interfaces to
media/mojo/interfaces/.

In a later CL, Media Router typemaps and related C++ structs will also be moved
to media/mojo/router/, given they don't depend on chrome/ or content/.

[1] extensions/renderer/resources/media_router_bindings.js

BUG=704958
==========

to

==========
Move c/b/media/router/mojo/*.mojom to media/mojo/interfaces/*.mojom

We have a file in extensions/ [1] that depends on Media Router mojo definitions
from chrome/browser/. To remove this dependency from extensions/ to
chrome/browser/, this CL moves Media Router mojo interfaces to
media/mojo/interfaces/.

In a later CL, Media Router typemaps and related C++ structs will also be moved
to media/mojo/router/, given they don't depend on chrome/ or content/.

[1] extensions/renderer/resources/media_router_bindings.js

BUG=704958
==========

[takumif, 2017-03-27 22:25:12]

Description was changed from

==========
Move c/b/media/router/mojo/*.mojom to media/mojo/interfaces/*.mojom

We have a file in extensions/ [1] that depends on Media Router mojo definitions
from chrome/browser/. To remove this dependency from extensions/ to
chrome/browser/, this CL moves Media Router mojo interfaces to
media/mojo/interfaces/.

In a later CL, Media Router typemaps and related C++ structs will also be moved
to media/mojo/router/, given they don't depend on chrome/ or content/.

[1] extensions/renderer/resources/media_router_bindings.js

BUG=704958
==========

to

==========
Move c/b/media/router/mojo/*.mojom to media/mojo/interfaces/*.mojom

We have a file in extensions/ [1] that depends on Media Router mojo definitions
from chrome/browser/. To remove this dependency from extensions/ to
chrome/browser/, this CL moves Media Router mojo interfaces to
media/mojo/interfaces/.

In a later CL, Media Router typemaps and related C++ structs will also be moved
to media/router/, given they don't depend on chrome/ or content/.

[1] extensions/renderer/resources/media_router_bindings.js

BUG=704958
==========

[takumif, 2017-03-27 22:35:35]

takumif@chromium.org changed reviewers:
+ dcheng@chromium.org, imcheng@chromium.org, mfoltz@chromium.org,
rdevlin.cronin@chromium.org, xhwang@chromium.org

[takumif, 2017-03-27 22:35:36]

Please take a look at:

Daniel:  *.mojom, *.typemap, *struct_traits*
Derek:   overall
Devlin:  extensions/
Mark:    overall
Xiaohan: media/

Thank you!

[mark a. foltz, 2017-03-27 22:40:45]

https://codereview.chromium.org/2771413003/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/mojo/media_router.typemap (right):

https://codereview.chromium.org/2771413003/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router.typemap:5: mojom =
"//media/mojo/interfaces/media_router.mojom"
This typemap should move here as well.

https://codereview.chromium.org/2771413003/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/mojo/media_router_struct_traits.h (right):

https://codereview.chromium.org/2771413003/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router_struct_traits.h:13: #include
"media/mojo/interfaces/media_router.mojom.h"
The struct_traits for these interfaces is an implicit dependency of the mojom
and should move to media/ as well IMO.  I am not sure where the typemaps for
other media/ interfaces are kept.

[xhwang, 2017-03-27 23:02:51]

Thanks!

After looking at this CL, I wonder whether it makes sense to put everything
under media/router folder, including the mojom interfaces. There are two reasons
I feel that's more reasonable:
1. Names like MediaController and MediaStatus are pretty generic. It's okay
since they are under the "media_router" module. But if the files are located
under media/mojo/interfaces, it would looks like it's generic media interfaces
and could confuse people.
2. To make DEPS easier. Some mojom files depends on //net, while traditionally
we don't want to depend on net [1]. It'll be much easier to specify media_router
specific DEPS rules if everything is in one folder.

In the future, probably media/remoting and media/gpu should follow the same
model.

[1]
https://cs.chromium.org/chromium/src/media/DEPS?rcl=4616044b6d5b9ef6bf8b77c9f...

[Devlin, 2017-03-28 01:26:12]

extensions stuff seems reasonable, but I'll wait for a MR expert to sign off
just in case any significant changes are needed.

[mark a. foltz, 2017-03-28 16:04:52]

On 2017/03/27 at 23:02:51, xhwang wrote:
> Thanks!
> 
> After looking at this CL, I wonder whether it makes sense to put everything
under media/router folder, including the mojom interfaces. There are two reasons
I feel that's more reasonable:
> 1. Names like MediaController and MediaStatus are pretty generic. It's okay
since they are under the "media_router" module. But if the files are located
under media/mojo/interfaces, it would looks like it's generic media interfaces
and could confuse people.
> 2. To make DEPS easier. Some mojom files depends on //net, while traditionally
we don't want to depend on net [1]. It'll be much easier to specify media_router
specific DEPS rules if everything is in one folder.
> 
> In the future, probably media/remoting and media/gpu should follow the same
model.
> 
> [1]
https://cs.chromium.org/chromium/src/media/DEPS?rcl=4616044b6d5b9ef6bf8b77c9f...

There seem to be a wide variety of interfaces in this folder, and it's not
obvious how they are grouped.  Would it make sense to split up all the mojoms in
media/mojo/interfaces into sub-folders in a separate patch, rather than forking
off a specific folder here?  Otherwise the organization wouldn't seem logical
(in terms of when to put things in one folder or another).

[xhwang, 2017-03-28 16:40:48]

On 2017/03/28 16:04:52, mark a. foltz wrote:
> On 2017/03/27 at 23:02:51, xhwang wrote:
> > Thanks!
> > 
> > After looking at this CL, I wonder whether it makes sense to put everything
> under media/router folder, including the mojom interfaces. There are two
reasons
> I feel that's more reasonable:
> > 1. Names like MediaController and MediaStatus are pretty generic. It's okay
> since they are under the "media_router" module. But if the files are located
> under media/mojo/interfaces, it would looks like it's generic media interfaces
> and could confuse people.
> > 2. To make DEPS easier. Some mojom files depends on //net, while
traditionally
> we don't want to depend on net [1]. It'll be much easier to specify
media_router
> specific DEPS rules if everything is in one folder.
> > 
> > In the future, probably media/remoting and media/gpu should follow the same
> model.
> > 
> > [1]
>
https://cs.chromium.org/chromium/src/media/DEPS?rcl=4616044b6d5b9ef6bf8b77c9f...
> 
> There seem to be a wide variety of interfaces in this folder, and it's not
> obvious how they are grouped.  Would it make sense to split up all the mojoms
in
> media/mojo/interfaces into sub-folders in a separate patch, rather than
forking
> off a specific folder here?  Otherwise the organization wouldn't seem logical
> (in terms of when to put things in one folder or another).

Yes, currently media/mojo/interfaces has all mojo interfaces used under media/.
We debated about this before :) The new model I am proposing is that each
module/feature has everything in it's own folder under media/, including mojo
interfaces. So we'll have something like:

media/foo/mojo/interfaces
media/foo/mojo/clients
media/foo/bar/...

media/remoting and media/gpu should probably all follow this model. (Today their
mojo interfaces are directly in media/mojo/interfaces).

Logically media/foo will be a separate target from media and the majority of
media/mojo. Mixing them together will make things harder to understand. Also, it
could cause unclear DEPS rules as described above.

[mark a. foltz, 2017-03-28 16:44:09]

On 2017/03/28 at 16:40:48, xhwang wrote:
> On 2017/03/28 16:04:52, mark a. foltz wrote:
> > On 2017/03/27 at 23:02:51, xhwang wrote:
> > > Thanks!
> > > 
> > > After looking at this CL, I wonder whether it makes sense to put
everything
> > under media/router folder, including the mojom interfaces. There are two
reasons
> > I feel that's more reasonable:
> > > 1. Names like MediaController and MediaStatus are pretty generic. It's
okay
> > since they are under the "media_router" module. But if the files are located
> > under media/mojo/interfaces, it would looks like it's generic media
interfaces
> > and could confuse people.
> > > 2. To make DEPS easier. Some mojom files depends on //net, while
traditionally
> > we don't want to depend on net [1]. It'll be much easier to specify
media_router
> > specific DEPS rules if everything is in one folder.
> > > 
> > > In the future, probably media/remoting and media/gpu should follow the
same
> > model.
> > > 
> > > [1]
> >
https://cs.chromium.org/chromium/src/media/DEPS?rcl=4616044b6d5b9ef6bf8b77c9f...
> > 
> > There seem to be a wide variety of interfaces in this folder, and it's not
> > obvious how they are grouped.  Would it make sense to split up all the
mojoms in
> > media/mojo/interfaces into sub-folders in a separate patch, rather than
forking
> > off a specific folder here?  Otherwise the organization wouldn't seem
logical
> > (in terms of when to put things in one folder or another).
> 
> Yes, currently media/mojo/interfaces has all mojo interfaces used under
media/. We debated about this before :) The new model I am proposing is that
each module/feature has everything in it's own folder under media/, including
mojo interfaces. So we'll have something like:
> 
> media/foo/mojo/interfaces
> media/foo/mojo/clients
> media/foo/bar/...
> 
> media/remoting and media/gpu should probably all follow this model. (Today
their mojo interfaces are directly in media/mojo/interfaces).
> 
> Logically media/foo will be a separate target from media and the majority of
media/mojo. Mixing them together will make things harder to understand. Also, it
could cause unclear DEPS rules as described above.

Will media/mojo/interfaces go away?  If not, what remains in
media/mojo/interfaces?

[takumif, 2017-03-28 16:52:22]

https://codereview.chromium.org/2771413003/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/mojo/media_router.typemap (right):

https://codereview.chromium.org/2771413003/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router.typemap:5: mojom =
"//media/mojo/interfaces/media_router.mojom"
On 2017/03/27 22:40:45, mark a. foltz wrote:
> This typemap should move here as well.

Moving this typemap would require also moving Issue, MediaRoute, etc. to media/,
so I was thinking we could do that in another patch.

Also, this typemap depends on content/, which we shouldn't from media/.
Can/should we move PresentationConnectionCloseReason and
PresentationConnectionState to media/?

https://codereview.chromium.org/2771413003/diff/40001/chrome/browser/media/ro...
File chrome/browser/media/router/mojo/media_router_struct_traits.h (right):

https://codereview.chromium.org/2771413003/diff/40001/chrome/browser/media/ro...
chrome/browser/media/router/mojo/media_router_struct_traits.h:13: #include
"media/mojo/interfaces/media_router.mojom.h"
On 2017/03/27 22:40:45, mark a. foltz wrote:
> The struct_traits for these interfaces is an implicit dependency of the mojom
> and should move to media/ as well IMO.  I am not sure where the typemaps for
> other media/ interfaces are kept.

All the typemaps for media/ interfaces are in media/ so it'd make sense to move
the Media Router typemaps there as well. I didn't do that yet since that'd be a
pretty big change.

[xhwang, 2017-03-28 16:54:36]

On 2017/03/28 16:44:09, mark a. foltz wrote:
> On 2017/03/28 at 16:40:48, xhwang wrote:
> > On 2017/03/28 16:04:52, mark a. foltz wrote:
> > > On 2017/03/27 at 23:02:51, xhwang wrote:
> > > > Thanks!
> > > > 
> > > > After looking at this CL, I wonder whether it makes sense to put
> everything
> > > under media/router folder, including the mojom interfaces. There are two
> reasons
> > > I feel that's more reasonable:
> > > > 1. Names like MediaController and MediaStatus are pretty generic. It's
> okay
> > > since they are under the "media_router" module. But if the files are
located
> > > under media/mojo/interfaces, it would looks like it's generic media
> interfaces
> > > and could confuse people.
> > > > 2. To make DEPS easier. Some mojom files depends on //net, while
> traditionally
> > > we don't want to depend on net [1]. It'll be much easier to specify
> media_router
> > > specific DEPS rules if everything is in one folder.
> > > > 
> > > > In the future, probably media/remoting and media/gpu should follow the
> same
> > > model.
> > > > 
> > > > [1]
> > >
>
https://cs.chromium.org/chromium/src/media/DEPS?rcl=4616044b6d5b9ef6bf8b77c9f...
> > > 
> > > There seem to be a wide variety of interfaces in this folder, and it's not
> > > obvious how they are grouped.  Would it make sense to split up all the
> mojoms in
> > > media/mojo/interfaces into sub-folders in a separate patch, rather than
> forking
> > > off a specific folder here?  Otherwise the organization wouldn't seem
> logical
> > > (in terms of when to put things in one folder or another).
> > 
> > Yes, currently media/mojo/interfaces has all mojo interfaces used under
> media/. We debated about this before :) The new model I am proposing is that
> each module/feature has everything in it's own folder under media/, including
> mojo interfaces. So we'll have something like:
> > 
> > media/foo/mojo/interfaces
> > media/foo/mojo/clients
> > media/foo/bar/...
> > 
> > media/remoting and media/gpu should probably all follow this model. (Today
> their mojo interfaces are directly in media/mojo/interfaces).
> > 
> > Logically media/foo will be a separate target from media and the majority of
> media/mojo. Mixing them together will make things harder to understand. Also,
it
> could cause unclear DEPS rules as described above.
> 
> Will media/mojo/interfaces go away?  If not, what remains in
> media/mojo/interfaces?

It will not go away. media/mojo/interfaces will contain mojo interfaces used to
interact with the core media pipeline (the "media" target), and common mojo
types corresponding to base madia types like media_types.mojom.

[mark a. foltz, 2017-03-28 17:07:58]

On 2017/03/28 at 16:54:36, xhwang wrote:
> On 2017/03/28 16:44:09, mark a. foltz wrote:
> > On 2017/03/28 at 16:40:48, xhwang wrote:
> > > On 2017/03/28 16:04:52, mark a. foltz wrote:
> > > > On 2017/03/27 at 23:02:51, xhwang wrote:
> > > > > Thanks!
> > > > > 
> > > > > After looking at this CL, I wonder whether it makes sense to put
> > everything
> > > > under media/router folder, including the mojom interfaces. There are two
> > reasons
> > > > I feel that's more reasonable:
> > > > > 1. Names like MediaController and MediaStatus are pretty generic. It's
> > okay
> > > > since they are under the "media_router" module. But if the files are
located
> > > > under media/mojo/interfaces, it would looks like it's generic media
> > interfaces
> > > > and could confuse people.
> > > > > 2. To make DEPS easier. Some mojom files depends on //net, while
> > traditionally
> > > > we don't want to depend on net [1]. It'll be much easier to specify
> > media_router
> > > > specific DEPS rules if everything is in one folder.
> > > > > 
> > > > > In the future, probably media/remoting and media/gpu should follow the
> > same
> > > > model.
> > > > > 
> > > > > [1]
> > > >
> >
https://cs.chromium.org/chromium/src/media/DEPS?rcl=4616044b6d5b9ef6bf8b77c9f...
> > > > 
> > > > There seem to be a wide variety of interfaces in this folder, and it's
not
> > > > obvious how they are grouped.  Would it make sense to split up all the
> > mojoms in
> > > > media/mojo/interfaces into sub-folders in a separate patch, rather than
> > forking
> > > > off a specific folder here?  Otherwise the organization wouldn't seem
> > logical
> > > > (in terms of when to put things in one folder or another).
> > > 
> > > Yes, currently media/mojo/interfaces has all mojo interfaces used under
> > media/. We debated about this before :) The new model I am proposing is that
> > each module/feature has everything in it's own folder under media/,
including
> > mojo interfaces. So we'll have something like:
> > > 
> > > media/foo/mojo/interfaces
> > > media/foo/mojo/clients
> > > media/foo/bar/...
> > > 
> > > media/remoting and media/gpu should probably all follow this model. (Today
> > their mojo interfaces are directly in media/mojo/interfaces).
> > > 
> > > Logically media/foo will be a separate target from media and the majority
of
> > media/mojo. Mixing them together will make things harder to understand.
Also, it
> > could cause unclear DEPS rules as described above.
> > 
> > Will media/mojo/interfaces go away?  If not, what remains in
> > media/mojo/interfaces?
> 
> It will not go away. media/mojo/interfaces will contain mojo interfaces used
to interact with the core media pipeline (the "media" target), and common mojo
types corresponding to base madia types like media_types.mojom.

I don't think that completely solves the problem then, unless it's obvious what
is the "core media pipeline" - to me that's just another media feature :)

Or is this a layering distinction - are features outside of core media allowed
to depend on core media? Maybe as part of splitting up the interfaces we should
have a README describing these rules.  It would help teams like us adding new
features into the media/ library.

[xhwang, 2017-03-28 17:58:24]

On 2017/03/28 17:07:58, mark a. foltz wrote:
> On 2017/03/28 at 16:54:36, xhwang wrote:
> > On 2017/03/28 16:44:09, mark a. foltz wrote:
> > > On 2017/03/28 at 16:40:48, xhwang wrote:
> > > > On 2017/03/28 16:04:52, mark a. foltz wrote:
> > > > > On 2017/03/27 at 23:02:51, xhwang wrote:
> > > > > > Thanks!
> > > > > > 
> > > > > > After looking at this CL, I wonder whether it makes sense to put
> > > everything
> > > > > under media/router folder, including the mojom interfaces. There are
two
> > > reasons
> > > > > I feel that's more reasonable:
> > > > > > 1. Names like MediaController and MediaStatus are pretty generic.
It's
> > > okay
> > > > > since they are under the "media_router" module. But if the files are
> located
> > > > > under media/mojo/interfaces, it would looks like it's generic media
> > > interfaces
> > > > > and could confuse people.
> > > > > > 2. To make DEPS easier. Some mojom files depends on //net, while
> > > traditionally
> > > > > we don't want to depend on net [1]. It'll be much easier to specify
> > > media_router
> > > > > specific DEPS rules if everything is in one folder.
> > > > > > 
> > > > > > In the future, probably media/remoting and media/gpu should follow
the
> > > same
> > > > > model.
> > > > > > 
> > > > > > [1]
> > > > >
> > >
>
https://cs.chromium.org/chromium/src/media/DEPS?rcl=4616044b6d5b9ef6bf8b77c9f...
> > > > > 
> > > > > There seem to be a wide variety of interfaces in this folder, and it's
> not
> > > > > obvious how they are grouped.  Would it make sense to split up all the
> > > mojoms in
> > > > > media/mojo/interfaces into sub-folders in a separate patch, rather
than
> > > forking
> > > > > off a specific folder here?  Otherwise the organization wouldn't seem
> > > logical
> > > > > (in terms of when to put things in one folder or another).
> > > > 
> > > > Yes, currently media/mojo/interfaces has all mojo interfaces used under
> > > media/. We debated about this before :) The new model I am proposing is
that
> > > each module/feature has everything in it's own folder under media/,
> including
> > > mojo interfaces. So we'll have something like:
> > > > 
> > > > media/foo/mojo/interfaces
> > > > media/foo/mojo/clients
> > > > media/foo/bar/...
> > > > 
> > > > media/remoting and media/gpu should probably all follow this model.
(Today
> > > their mojo interfaces are directly in media/mojo/interfaces).
> > > > 
> > > > Logically media/foo will be a separate target from media and the
majority
> of
> > > media/mojo. Mixing them together will make things harder to understand.
> Also, it
> > > could cause unclear DEPS rules as described above.
> > > 
> > > Will media/mojo/interfaces go away?  If not, what remains in
> > > media/mojo/interfaces?
> > 
> > It will not go away. media/mojo/interfaces will contain mojo interfaces used
> to interact with the core media pipeline (the "media" target), and common mojo
> types corresponding to base madia types like media_types.mojom.
> 
> I don't think that completely solves the problem then, unless it's obvious
what
> is the "core media pipeline" - to me that's just another media feature :)
> 
> Or is this a layering distinction - are features outside of core media allowed
> to depend on core media? Maybe as part of splitting up the interfaces we
should
> have a README describing these rules.  It would help teams like us adding new
> features into the media/ library.

I agree. The folder structure of media/ and the associated gn targets definitely
need a lot of refactoring and reorganization, with proper documentation. We
talked about this before but it's  lower priority :(

For now, let's move everything (including mojo interfaces) about media router
into a folder under media. We'll fix media/ later. Thanks!

[dcheng, 2017-03-28 18:45:37]

rs lgtm for the mojo security review, as this is just moving some stuff around

[mark a. foltz, 2017-03-28 19:49:08]

On 2017/03/28 at 17:58:24, xhwang wrote:
> On 2017/03/28 17:07:58, mark a. foltz wrote:
> > On 2017/03/28 at 16:54:36, xhwang wrote:
> > > On 2017/03/28 16:44:09, mark a. foltz wrote:
> > > > On 2017/03/28 at 16:40:48, xhwang wrote:
> > > > > On 2017/03/28 16:04:52, mark a. foltz wrote:
> > > > > > On 2017/03/27 at 23:02:51, xhwang wrote:
> > > > > > > Thanks!
> > > > > > > 
> > > > > > > After looking at this CL, I wonder whether it makes sense to put
> > > > everything
> > > > > > under media/router folder, including the mojom interfaces. There are
two
> > > > reasons
> > > > > > I feel that's more reasonable:
> > > > > > > 1. Names like MediaController and MediaStatus are pretty generic.
It's
> > > > okay
> > > > > > since they are under the "media_router" module. But if the files are
> > located
> > > > > > under media/mojo/interfaces, it would looks like it's generic media
> > > > interfaces
> > > > > > and could confuse people.
> > > > > > > 2. To make DEPS easier. Some mojom files depends on //net, while
> > > > traditionally
> > > > > > we don't want to depend on net [1]. It'll be much easier to specify
> > > > media_router
> > > > > > specific DEPS rules if everything is in one folder.
> > > > > > > 
> > > > > > > In the future, probably media/remoting and media/gpu should follow
the
> > > > same
> > > > > > model.
> > > > > > > 
> > > > > > > [1]
> > > > > >
> > > >
> >
https://cs.chromium.org/chromium/src/media/DEPS?rcl=4616044b6d5b9ef6bf8b77c9f...
> > > > > > 
> > > > > > There seem to be a wide variety of interfaces in this folder, and
it's
> > not
> > > > > > obvious how they are grouped.  Would it make sense to split up all
the
> > > > mojoms in
> > > > > > media/mojo/interfaces into sub-folders in a separate patch, rather
than
> > > > forking
> > > > > > off a specific folder here?  Otherwise the organization wouldn't
seem
> > > > logical
> > > > > > (in terms of when to put things in one folder or another).
> > > > > 
> > > > > Yes, currently media/mojo/interfaces has all mojo interfaces used
under
> > > > media/. We debated about this before :) The new model I am proposing is
that
> > > > each module/feature has everything in it's own folder under media/,
> > including
> > > > mojo interfaces. So we'll have something like:
> > > > > 
> > > > > media/foo/mojo/interfaces
> > > > > media/foo/mojo/clients
> > > > > media/foo/bar/...
> > > > > 
> > > > > media/remoting and media/gpu should probably all follow this model.
(Today
> > > > their mojo interfaces are directly in media/mojo/interfaces).
> > > > > 
> > > > > Logically media/foo will be a separate target from media and the
majority
> > of
> > > > media/mojo. Mixing them together will make things harder to understand.
> > Also, it
> > > > could cause unclear DEPS rules as described above.
> > > > 
> > > > Will media/mojo/interfaces go away?  If not, what remains in
> > > > media/mojo/interfaces?
> > > 
> > > It will not go away. media/mojo/interfaces will contain mojo interfaces
used
> > to interact with the core media pipeline (the "media" target), and common
mojo
> > types corresponding to base madia types like media_types.mojom.
> > 
> > I don't think that completely solves the problem then, unless it's obvious
what
> > is the "core media pipeline" - to me that's just another media feature :)
> > 
> > Or is this a layering distinction - are features outside of core media
allowed
> > to depend on core media? Maybe as part of splitting up the interfaces we
should
> > have a README describing these rules.  It would help teams like us adding
new
> > features into the media/ library.
> 
> I agree. The folder structure of media/ and the associated gn targets
definitely need a lot of refactoring and reorganization, with proper
documentation. We talked about this before but it's  lower priority :(
> 
> For now, let's move everything (including mojo interfaces) about media router
into a folder under media. We'll fix media/ later. Thanks!

SGTM.  I filed crbug.com/706115 to capture this conversation.  Feel free to add
further ideas there.

[takumif, 2017-03-28 22:06:43]

takumif@chromium.org changed reviewers:
+ thakis@chromium.org

[takumif, 2017-03-28 22:06:44]

+thakis for c/b/DEPS

I've moved the mojo files to media/router/. Please take a look, thanks.

media/router/ doesn't have any files aside from OWNERS and the mojo/ directory
for now, but the plan is to move typemapped classes/structs there.

[imcheng, 2017-03-28 23:47:41]

lgtm

https://codereview.chromium.org/2771413003/diff/60001/media/router/mojo/media...
File media/router/mojo/media_controller.mojom (right):

https://codereview.chromium.org/2771413003/diff/60001/media/router/mojo/media...
media/router/mojo/media_controller.mojom:1: // Copyright 2017 The Chromium
Authors. All rights reserved.
It seems this and other .mojom files should be in the interfaces folder to be
consistent with the directory structure in media/. Our C++ classes that uses
generated mojo code could then be placed in a separate folder.

[xhwang, 2017-03-29 00:04:33]

Thanks for doing this!

media/ LGTM

I'll leave you guys to figure out what's the best folder structure you want to
follow.

[takumif, 2017-03-29 00:17:26]

Patchset #5 (id:80001) has been deleted

[takumif, 2017-03-29 01:03:40]

takumif@chromium.org changed reviewers:
- thakis@chromium.org

[takumif, 2017-03-29 01:03:41]

https://codereview.chromium.org/2771413003/diff/60001/media/router/mojo/media...
File media/router/mojo/media_controller.mojom (right):

https://codereview.chromium.org/2771413003/diff/60001/media/router/mojo/media...
media/router/mojo/media_controller.mojom:1: // Copyright 2017 The Chromium
Authors. All rights reserved.
On 2017/03/28 23:47:41, imcheng wrote:
> It seems this and other .mojom files should be in the interfaces folder to be
> consistent with the directory structure in media/. Our C++ classes that uses
> generated mojo code could then be placed in a separate folder.

Actually, media/capture/mojo/ doesn't have an interface/ directory, and I was
thinking that we could mirror where files are currently located in
c/b/media/router/ and c/b/media/router/mojo/, since we wouldn't need
subdirectories other than interface/.

[imcheng, 2017-03-29 01:23:57]

On 2017/03/29 01:03:41, takumif wrote:
>
https://codereview.chromium.org/2771413003/diff/60001/media/router/mojo/media...
> File media/router/mojo/media_controller.mojom (right):
> 
>
https://codereview.chromium.org/2771413003/diff/60001/media/router/mojo/media...
> media/router/mojo/media_controller.mojom:1: // Copyright 2017 The Chromium
> Authors. All rights reserved.
> On 2017/03/28 23:47:41, imcheng wrote:
> > It seems this and other .mojom files should be in the interfaces folder to
be
> > consistent with the directory structure in media/. Our C++ classes that uses
> > generated mojo code could then be placed in a separate folder.
> 
> Actually, media/capture/mojo/ doesn't have an interface/ directory, and I was
> thinking that we could mirror where files are currently located in
> c/b/media/router/ and c/b/media/router/mojo/, since we wouldn't need
> subdirectories other than interface/.

Sure, that's fine.

[Nico, 2017-03-29 12:51:03]

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

[Nico, 2017-03-29 12:51:04]

lgtm

Nice, thanks for doing this!

[imcheng, 2017-03-29 17:11:21]

One more thing: could you please update the patch title and description?

[takumif, 2017-03-29 17:14:46]

Description was changed from

==========
Move c/b/media/router/mojo/*.mojom to media/mojo/interfaces/*.mojom

We have a file in extensions/ [1] that depends on Media Router mojo definitions
from chrome/browser/. To remove this dependency from extensions/ to
chrome/browser/, this CL moves Media Router mojo interfaces to
media/mojo/interfaces/.

In a later CL, Media Router typemaps and related C++ structs will also be moved
to media/router/, given they don't depend on chrome/ or content/.

[1] extensions/renderer/resources/media_router_bindings.js

BUG=704958
==========

to

==========
Move c/b/media/router/mojo/*.mojom to media/router/mojo/*.mojom

We have a file in extensions/ [1] that depends on Media Router mojo definitions
from chrome/browser/. To remove this dependency from extensions/ to
chrome/browser/, this CL moves Media Router mojo interfaces to
media/router/mojo/.

In a later CL, Media Router typemaps and related C++ structs will also be moved
to media/router/, given they don't depend on chrome/ or content/.

[1] extensions/renderer/resources/media_router_bindings.js

BUG=704958
==========

[takumif, 2017-03-29 17:15:58]

On 2017/03/29 17:11:21, imcheng wrote:
> One more thing: could you please update the patch title and description?

Done.

[takumif, 2017-03-29 20:11:21]

The CQ bit was checked by takumif@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 20:12:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 21:59:59]

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

[commit-bot: I haz the power, 2017-03-29 22:00:00]

Dry run: This issue passed the CQ dry run.

[mark a. foltz, 2017-03-30 21:46:43]

LGTM

There will still be an implicit dependency on chrome/browser because of the mojo
typemaps.  Please work out a plan to eventually migrate typemaps + traits files
into media/router (which will require figuring out some layering inside the MR
implementation).

Also we will probably want to add media/router/DEPS as part of that change.

[Devlin, 2017-03-30 22:04:26]

extensions lgtm

[takumif, 2017-03-30 23:06:58]

Description was changed from

==========
Move c/b/media/router/mojo/*.mojom to media/router/mojo/*.mojom

We have a file in extensions/ [1] that depends on Media Router mojo definitions
from chrome/browser/. To remove this dependency from extensions/ to
chrome/browser/, this CL moves Media Router mojo interfaces to
media/router/mojo/.

In a later CL, Media Router typemaps and related C++ structs will also be moved
to media/router/, given they don't depend on chrome/ or content/.

[1] extensions/renderer/resources/media_router_bindings.js

BUG=704958
==========

to

==========
Move c/b/media/router/mojo/*.mojom to components/media_router/mojo/*.mojom

We have a file in extensions/ [1] that depends on Media Router mojo definitions
from chrome/browser/. To remove this dependency from extensions/ to
chrome/browser/, this CL moves Media Router mojo interfaces to
components/media_router/mojo/.

In a later CL, Media Router typemaps and related C++ structs will also be moved
to components/media_router/.

[1] extensions/renderer/resources/media_router_bindings.js

BUG=704958
==========

[takumif, 2017-03-30 23:16:26]

takumif@chromium.org changed reviewers:
+ sdefresne@chromium.org
- thakis@chromium.org

[takumif, 2017-03-30 23:16:27]

After an offline discussion with Xiaohan and Derek, we've decided to move the
mojo files to components/media_router/ instead, since the typemaps for them
depend on content/, and we don't want to depend on content/ from media/.

+Sylvain for components/

Please take a look, thanks!

[sdefresne, 2017-03-31 09:35:05]

On 2017/03/30 23:16:27, takumif wrote:
> After an offline discussion with Xiaohan and Derek, we've decided to move the
> mojo files to components/media_router/ instead, since the typemaps for them
> depend on content/, and we don't want to depend on content/ from media/.
> 
> +Sylvain for components/
> 
> Please take a look, thanks!

It looks like media/ already depends on content/:

$ git grep //content -- media
media/base/BUILD.gn:    "//content/browser",
media/base/BUILD.gn:    "//content/public/common:common_sources",
media/base/android/BUILD.gn:     
"//content/public/android:content_java_resources",
media/gpu/BUILD.gn:    "//content/gpu:*",
media/gpu/BUILD.gn:    "//content/renderer:*",
media/gpu/BUILD.gn:    visibility += [ "//content/common" ]
media/gpu/ipc/service/BUILD.gn:    "//content/gpu/*",
media/gpu/ipc/service/BUILD.gn:    "//content/public/gpu/*",
media/mojo/clients/BUILD.gn:    "//content/renderer",

Given that, do you really still want to move the code to
components/media_router?

[xhwang, 2017-03-31 16:32:23]

On 2017/03/31 09:35:05, sdefresne wrote:
> On 2017/03/30 23:16:27, takumif wrote:
> > After an offline discussion with Xiaohan and Derek, we've decided to move
the
> > mojo files to components/media_router/ instead, since the typemaps for them
> > depend on content/, and we don't want to depend on content/ from media/.
> > 
> > +Sylvain for components/
> > 
> > Please take a look, thanks!
> 
> It looks like media/ already depends on content/:
> 
> $ git grep //content -- media
> media/base/BUILD.gn:    "//content/browser",
> media/base/BUILD.gn:    "//content/public/common:common_sources",
> media/base/android/BUILD.gn:     
> "//content/public/android:content_java_resources",
> media/gpu/BUILD.gn:    "//content/gpu:*",
> media/gpu/BUILD.gn:    "//content/renderer:*",
> media/gpu/BUILD.gn:    visibility += [ "//content/common" ]
> media/gpu/ipc/service/BUILD.gn:    "//content/gpu/*",
> media/gpu/ipc/service/BUILD.gn:    "//content/public/gpu/*",
> media/mojo/clients/BUILD.gn:    "//content/renderer",
> 
> Given that, do you really still want to move the code to
> components/media_router?

Those are mostly "visibility" rules, not "deps" rules, with the exception of 
 media/base/android/BUILD.gn:     
   "//content/public/android:content_java_resources",
which I think is a bug and should be fixed.

[xhwang, 2017-03-31 16:53:55]

On 2017/03/31 16:32:23, xhwang_slow wrote:
> On 2017/03/31 09:35:05, sdefresne wrote:
> > On 2017/03/30 23:16:27, takumif wrote:
> > > After an offline discussion with Xiaohan and Derek, we've decided to move
> the
> > > mojo files to components/media_router/ instead, since the typemaps for
them
> > > depend on content/, and we don't want to depend on content/ from media/.
> > > 
> > > +Sylvain for components/
> > > 
> > > Please take a look, thanks!
> > 
> > It looks like media/ already depends on content/:
> > 
> > $ git grep //content -- media
> > media/base/BUILD.gn:    "//content/browser",
> > media/base/BUILD.gn:    "//content/public/common:common_sources",
> > media/base/android/BUILD.gn:     
> > "//content/public/android:content_java_resources",
> > media/gpu/BUILD.gn:    "//content/gpu:*",
> > media/gpu/BUILD.gn:    "//content/renderer:*",
> > media/gpu/BUILD.gn:    visibility += [ "//content/common" ]
> > media/gpu/ipc/service/BUILD.gn:    "//content/gpu/*",
> > media/gpu/ipc/service/BUILD.gn:    "//content/public/gpu/*",
> > media/mojo/clients/BUILD.gn:    "//content/renderer",
> > 
> > Given that, do you really still want to move the code to
> > components/media_router?
> 
> Those are mostly "visibility" rules, not "deps" rules, with the exception of 
>  media/base/android/BUILD.gn:     
>    "//content/public/android:content_java_resources",
> which I think is a bug and should be fixed.

filed https://bugs.chromium.org/p/chromium/issues/detail?id=707265 to track
this.

[takumif, 2017-03-31 17:44:31]

The CQ bit was checked by takumif@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 17:45:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 17:51:06]

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

[commit-bot: I haz the power, 2017-03-31 17:51:07]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[takumif, 2017-03-31 21:18:39]

The CQ bit was checked by takumif@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 21:19:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 21:25:04]

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

[commit-bot: I haz the power, 2017-03-31 21:25:08]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[takumif, 2017-03-31 21:27:26]

Patchset #7 (id:140001) has been deleted

[takumif, 2017-03-31 22:10:04]

The CQ bit was checked by takumif@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 22:10:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 22:18:00]

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

[commit-bot: I haz the power, 2017-03-31 22:18:02]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[takumif, 2017-03-31 22:47:09]

The CQ bit was checked by takumif@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 22:47:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 22:55:27]

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

[commit-bot: I haz the power, 2017-03-31 22:55:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[takumif, 2017-03-31 23:04:05]

The CQ bit was checked by takumif@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 23:04:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 23:11:58]

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

[commit-bot: I haz the power, 2017-03-31 23:12:00]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[takumif, 2017-03-31 23:20:12]

The CQ bit was checked by takumif@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 23:21:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 23:27:43]

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

[commit-bot: I haz the power, 2017-03-31 23:27:44]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[takumif, 2017-03-31 23:37:33]

The CQ bit was checked by takumif@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 23:38:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 23:42:56]

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

[commit-bot: I haz the power, 2017-03-31 23:42:58]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[takumif, 2017-04-01 00:16:59]

Patchset #11 (id:240001) has been deleted

[takumif, 2017-04-01 00:17:12]

Patchset #10 (id:220001) has been deleted

[takumif, 2017-04-01 00:33:32]

The CQ bit was checked by takumif@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-01 00:34:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-01 00:38:50]

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

[commit-bot: I haz the power, 2017-04-01 00:38:52]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[takumif, 2017-04-01 00:51:04]

The CQ bit was checked by takumif@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-01 00:51:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-01 00:56:24]

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

[commit-bot: I haz the power, 2017-04-01 00:56:26]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[takumif, 2017-04-01 00:59:52]

The CQ bit was checked by takumif@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-01 01:00:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-01 01:04:58]

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

[commit-bot: I haz the power, 2017-04-01 01:05:00]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[sdefresne, 2017-04-03 13:34:07]

lgtm

https://codereview.chromium.org/2771413003/diff/300001/components/media_route...
File components/media_router/discovery/media_sink_internal_unittest.cc (right):

https://codereview.chromium.org/2771413003/diff/300001/components/media_route...
components/media_router/discovery/media_sink_internal_unittest.cc:6: #include
"testing/gmock/include/gmock/gmock.h"
nit: blank line above

https://codereview.chromium.org/2771413003/diff/300001/components/media_route...
File components/media_router/issue_unittest.cc (right):

https://codereview.chromium.org/2771413003/diff/300001/components/media_route...
components/media_router/issue_unittest.cc:6: #include
"testing/gmock/include/gmock/gmock.h"
nit: blank line above

https://codereview.chromium.org/2771413003/diff/300001/components/media_route...
File components/media_router/media_route_unittest.cc (right):

https://codereview.chromium.org/2771413003/diff/300001/components/media_route...
components/media_router/media_route_unittest.cc:6: #include
"components/media_router/media_sink.h"
nit: blank line above

https://codereview.chromium.org/2771413003/diff/300001/components/media_route...
File components/media_router/media_sink.cc (right):

https://codereview.chromium.org/2771413003/diff/300001/components/media_route...
components/media_router/media_sink.cc:6: #include "base/i18n/string_compare.h"
nit: blank line above

https://codereview.chromium.org/2771413003/diff/300001/components/media_route...
File components/media_router/media_sink_unittest.cc (right):

https://codereview.chromium.org/2771413003/diff/300001/components/media_route...
components/media_router/media_sink_unittest.cc:6: #include
"testing/gmock/include/gmock/gmock.h"
nit: blank line above

https://codereview.chromium.org/2771413003/diff/300001/components/media_route...
File components/media_router/media_source_helper_unittest.cc (right):

https://codereview.chromium.org/2771413003/diff/300001/components/media_route...
components/media_router/media_source_helper_unittest.cc:6: #include
"components/media_router/media_source.h"
nit: blank line above

https://codereview.chromium.org/2771413003/diff/300001/components/media_route...
File components/media_router/media_source_unittest.cc (right):

https://codereview.chromium.org/2771413003/diff/300001/components/media_route...
components/media_router/media_source_unittest.cc:6: #include
"testing/gtest/include/gtest/gtest.h"
nit: blank line above

[sdefresne, 2017-04-03 13:34:13]

On 2017/03/31 16:53:55, xhwang_slow wrote:
> On 2017/03/31 16:32:23, xhwang_slow wrote:
> > On 2017/03/31 09:35:05, sdefresne wrote:
> > > On 2017/03/30 23:16:27, takumif wrote:
> > > > After an offline discussion with Xiaohan and Derek, we've decided to
move
> > the
> > > > mojo files to components/media_router/ instead, since the typemaps for
> them
> > > > depend on content/, and we don't want to depend on content/ from media/.
> > > > 
> > > > +Sylvain for components/
> > > > 
> > > > Please take a look, thanks!
> > > 
> > > It looks like media/ already depends on content/:
> > > 
> > > $ git grep //content -- media
> > > media/base/BUILD.gn:    "//content/browser",
> > > media/base/BUILD.gn:    "//content/public/common:common_sources",
> > > media/base/android/BUILD.gn:     
> > > "//content/public/android:content_java_resources",
> > > media/gpu/BUILD.gn:    "//content/gpu:*",
> > > media/gpu/BUILD.gn:    "//content/renderer:*",
> > > media/gpu/BUILD.gn:    visibility += [ "//content/common" ]
> > > media/gpu/ipc/service/BUILD.gn:    "//content/gpu/*",
> > > media/gpu/ipc/service/BUILD.gn:    "//content/public/gpu/*",
> > > media/mojo/clients/BUILD.gn:    "//content/renderer",
> > > 
> > > Given that, do you really still want to move the code to
> > > components/media_router?
> > 
> > Those are mostly "visibility" rules, not "deps" rules, with the exception of

> >  media/base/android/BUILD.gn:     
> >    "//content/public/android:content_java_resources",
> > which I think is a bug and should be fixed.
> 
> filed https://bugs.chromium.org/p/chromium/issues/detail?id=707265 to track
> this.

Ack, thank you for checking.

[takumif, 2017-04-03 17:17:57]

The CQ bit was checked by takumif@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 17:18:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-03 17:24:28]

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

[commit-bot: I haz the power, 2017-04-03 17:24:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[takumif, 2017-04-03 17:51:21]

The CQ bit was checked by takumif@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 17:52:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-03 18:00:08]

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

[commit-bot: I haz the power, 2017-04-03 18:00:10]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[takumif, 2017-04-03 18:10:58]

Patchset #14 (id:340001) has been deleted

[takumif, 2017-04-03 18:11:14]

Patchset #13 (id:320001) has been deleted

[takumif, 2017-04-03 18:12:46]

The CQ bit was checked by takumif@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 18:13:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[takumif, 2017-04-03 19:03:41]

Patchset #13 (id:360001) has been deleted

[takumif, 2017-04-03 19:06:01]

The CQ bit was checked by takumif@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 19:07:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-03 20:18:16]

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

[commit-bot: I haz the power, 2017-04-03 20:18:18]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[takumif, 2017-04-03 21:03:22]

The CQ bit was checked by takumif@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 21:03:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-03 22:39:34]

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

[commit-bot: I haz the power, 2017-04-03 22:39:38]

Dry run: This issue passed the CQ dry run.

[mark a. foltz, 2017-04-03 22:50:13]

mfoltz@chromium.org changed reviewers:
+ thakis@chromium.org

[mark a. foltz, 2017-04-03 22:50:16]

Still LGTM with a couple of inline comments.

Probably a good idea to include a README.md here as well describing what code is
contained inside.  Also explain that the Media Router API and its
implementations is going to live in chrome/browser/media/router for now.

https://codereview.chromium.org/2771413003/diff/380001/chrome/browser/media/r...
File chrome/browser/media/router/presentation_request.h (right):

https://codereview.chromium.org/2771413003/diff/380001/chrome/browser/media/r...
chrome/browser/media/router/presentation_request.h:17: // Represents a
presentation request made from a render frame. Contains the
I feel like this belongs in content/browser, along with
create_presentation_connection_request.  The method to convert URLs to
MediaSources could be moved to media_source.h to avoid a circular dependency
between components/ and content/.

https://codereview.chromium.org/2771413003/diff/380001/components/media_route...
File components/media_router/DEPS (right):

https://codereview.chromium.org/2771413003/diff/380001/components/media_route...
components/media_router/DEPS:1: include_rules = [
Can we include a "-chrome" here to avoid circularity?

https://codereview.chromium.org/2771413003/diff/380001/components/media_route...
File components/media_router/OWNERS (right):

https://codereview.chromium.org/2771413003/diff/380001/components/media_route...
components/media_router/OWNERS:1: imcheng@chromium.org
Please include the existing OWNERS file using file:// instead.

[mark a. foltz, 2017-04-03 22:51:07]

https://codereview.chromium.org/2771413003/diff/380001/chrome/browser/media/r...
File chrome/browser/media/router/presentation_request.h (right):

https://codereview.chromium.org/2771413003/diff/380001/chrome/browser/media/r...
chrome/browser/media/router/presentation_request.h:17: // Represents a
presentation request made from a render frame. Contains the
On 2017/04/03 at 22:50:15, mark a. foltz wrote:
> I feel like this belongs in content/browser, along with
create_presentation_connection_request.  The method to convert URLs to
MediaSources could be moved to media_source.h to avoid a circular dependency
between components/ and content/.

Note: not implying this is to be done in this patch.  If you agree, then feel
free to add a TODO.

[takumif, 2017-04-04 16:54:18]

Description was changed from

==========
Move c/b/media/router/mojo/*.mojom to components/media_router/mojo/*.mojom

We have a file in extensions/ [1] that depends on Media Router mojo definitions
from chrome/browser/. To remove this dependency from extensions/ to
chrome/browser/, this CL moves Media Router mojo interfaces to
components/media_router/mojo/.

In a later CL, Media Router typemaps and related C++ structs will also be moved
to components/media_router/.

[1] extensions/renderer/resources/media_router_bindings.js

BUG=704958
==========

to

==========
Move c/b/media/router/mojo/*.mojom to components/media_router/mojo/*.mojom

We have a file in extensions/ [1] that depends on Media Router mojo definitions
from chrome/browser/. To remove this dependency from extensions/ to
chrome/browser/, this CL moves Media Router mojo interfaces to
components/media_router/mojo/. Media Router typemaps and related C++ structs are
also moved to components/media_router/.

[1] extensions/renderer/resources/media_router_bindings.js

BUG=704958
==========

[takumif, 2017-04-04 17:25:41]

The CQ bit was checked by takumif@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 17:26:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[takumif, 2017-04-04 17:34:47]

takumif@chromium.org changed reviewers:
+ jam@chromium.org

[takumif, 2017-04-04 17:34:51]

+jam for the dependencies in components/media_router/DEPS, and
chromium_bindings_configuration.gni. Please take a look, thanks.

https://codereview.chromium.org/2771413003/diff/300001/components/media_route...
File components/media_router/discovery/media_sink_internal_unittest.cc (right):

https://codereview.chromium.org/2771413003/diff/300001/components/media_route...
components/media_router/discovery/media_sink_internal_unittest.cc:6: #include
"testing/gmock/include/gmock/gmock.h"
On 2017/04/03 13:34:06, sdefresne wrote:
> nit: blank line above

Done.

https://codereview.chromium.org/2771413003/diff/300001/components/media_route...
File components/media_router/issue_unittest.cc (right):

https://codereview.chromium.org/2771413003/diff/300001/components/media_route...
components/media_router/issue_unittest.cc:6: #include
"testing/gmock/include/gmock/gmock.h"
On 2017/04/03 13:34:06, sdefresne wrote:
> nit: blank line above

Done.

https://codereview.chromium.org/2771413003/diff/300001/components/media_route...
File components/media_router/media_route_unittest.cc (right):

https://codereview.chromium.org/2771413003/diff/300001/components/media_route...
components/media_router/media_route_unittest.cc:6: #include
"components/media_router/media_sink.h"
On 2017/04/03 13:34:06, sdefresne wrote:
> nit: blank line above

Done.

https://codereview.chromium.org/2771413003/diff/300001/components/media_route...
File components/media_router/media_sink.cc (right):

https://codereview.chromium.org/2771413003/diff/300001/components/media_route...
components/media_router/media_sink.cc:6: #include "base/i18n/string_compare.h"
On 2017/04/03 13:34:07, sdefresne wrote:
> nit: blank line above

Done.

https://codereview.chromium.org/2771413003/diff/300001/components/media_route...
File components/media_router/media_sink_unittest.cc (right):

https://codereview.chromium.org/2771413003/diff/300001/components/media_route...
components/media_router/media_sink_unittest.cc:6: #include
"testing/gmock/include/gmock/gmock.h"
On 2017/04/03 13:34:07, sdefresne wrote:
> nit: blank line above

Done.

https://codereview.chromium.org/2771413003/diff/300001/components/media_route...
File components/media_router/media_source_helper_unittest.cc (right):

https://codereview.chromium.org/2771413003/diff/300001/components/media_route...
components/media_router/media_source_helper_unittest.cc:6: #include
"components/media_router/media_source.h"
On 2017/04/03 13:34:07, sdefresne wrote:
> nit: blank line above

Done.

https://codereview.chromium.org/2771413003/diff/300001/components/media_route...
File components/media_router/media_source_unittest.cc (right):

https://codereview.chromium.org/2771413003/diff/300001/components/media_route...
components/media_router/media_source_unittest.cc:6: #include
"testing/gtest/include/gtest/gtest.h"
On 2017/04/03 13:34:07, sdefresne wrote:
> nit: blank line above

Done.

https://codereview.chromium.org/2771413003/diff/380001/chrome/browser/media/r...
File chrome/browser/media/router/presentation_request.h (right):

https://codereview.chromium.org/2771413003/diff/380001/chrome/browser/media/r...
chrome/browser/media/router/presentation_request.h:17: // Represents a
presentation request made from a render frame. Contains the
On 2017/04/03 22:50:15, mfoltz_ooo_until_4_10 wrote:
> I feel like this belongs in content/browser, along with
> create_presentation_connection_request.  The method to convert URLs to
> MediaSources could be moved to media_source.h to avoid a circular dependency
> between components/ and content/.

Adding a TODO to move presentation_request. I'm not sure if
create_presentation_connection_request can be moved, given its dependency on
MediaRoute.

https://codereview.chromium.org/2771413003/diff/380001/components/media_route...
File components/media_router/DEPS (right):

https://codereview.chromium.org/2771413003/diff/380001/components/media_route...
components/media_router/DEPS:1: include_rules = [
On 2017/04/03 22:50:15, mfoltz_ooo_until_4_10 wrote:
> Can we include a "-chrome" here to avoid circularity?

Done.

https://codereview.chromium.org/2771413003/diff/380001/components/media_route...
File components/media_router/OWNERS (right):

https://codereview.chromium.org/2771413003/diff/380001/components/media_route...
components/media_router/OWNERS:1: imcheng@chromium.org
On 2017/04/03 22:50:15, mfoltz_ooo_until_4_10 wrote:
> Please include the existing OWNERS file using file:// instead.

Done.

[commit-bot: I haz the power, 2017-04-04 17:41:23]

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

[commit-bot: I haz the power, 2017-04-04 17:41:25]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[jam, 2017-04-04 17:58:52]

did you consider just moving the file in extensions/ up to chrome? I personally
find the chrome vs extensions split a bit odd sometimes, since it seems like
it's in a halfway state.

anyways, looking at  extensions/renderer/resources/media_router_bindings.js:
https://cs.chromium.org/chromium/src/extensions/renderer/resources/media_rout...

it already lists a bunch of mojoms in chrome/browser/media. this suggests that
this file doesn't really belong in extensions/

[takumif, 2017-04-04 18:08:33]

On 2017/04/04 17:58:52, jam wrote:
> did you consider just moving the file in extensions/ up to chrome? I
personally
> find the chrome vs extensions split a bit odd sometimes, since it seems like
> it's in a halfway state.
> 
> anyways, looking at  extensions/renderer/resources/media_router_bindings.js:
>
https://cs.chromium.org/chromium/src/extensions/renderer/resources/media_rout...
> 
> it already lists a bunch of mojoms in chrome/browser/media. this suggests that
> this file doesn't really belong in extensions/

If we moved media_router_bindings.js to chrome/, we'd now have to depend on that
from extensions/.
This CL moves all the mojoms the file depends on to components/media_router/.

[takumif, 2017-04-04 18:24:28]

The CQ bit was checked by takumif@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 18:25:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-04 19:18:36]

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

[commit-bot: I haz the power, 2017-04-04 19:18:37]

Dry run: This issue passed the CQ dry run.

[jam, 2017-04-04 19:28:55]

On 2017/04/04 18:08:33, takumif wrote:
> On 2017/04/04 17:58:52, jam wrote:
> > did you consider just moving the file in extensions/ up to chrome? I
> personally
> > find the chrome vs extensions split a bit odd sometimes, since it seems like
> > it's in a halfway state.
> > 
> > anyways, looking at  extensions/renderer/resources/media_router_bindings.js:
> >
>
https://cs.chromium.org/chromium/src/extensions/renderer/resources/media_rout...
> > 
> > it already lists a bunch of mojoms in chrome/browser/media. this suggests
that
> > this file doesn't really belong in extensions/
> 
> If we moved media_router_bindings.js to chrome/, we'd now have to depend on
that
> from extensions/.

can you explain why?

> This CL moves all the mojoms the file depends on to components/media_router/.

[takumif, 2017-04-04 20:31:29]

On 2017/04/04 19:28:55, jam wrote:
> On 2017/04/04 18:08:33, takumif wrote:
> > On 2017/04/04 17:58:52, jam wrote:
> > > did you consider just moving the file in extensions/ up to chrome? I
> > personally
> > > find the chrome vs extensions split a bit odd sometimes, since it seems
like
> > > it's in a halfway state.
> > > 
> > > anyways, looking at 
extensions/renderer/resources/media_router_bindings.js:
> > >
> >
>
https://cs.chromium.org/chromium/src/extensions/renderer/resources/media_rout...
> > > 
> > > it already lists a bunch of mojoms in chrome/browser/media. this suggests
> that
> > > this file doesn't really belong in extensions/
> > 
> > If we moved media_router_bindings.js to chrome/, we'd now have to depend on
> that
> > from extensions/.
> 
> can you explain why?
> 
> > This CL moves all the mojoms the file depends on to
components/media_router/.

We expose the mojo interfaces to the Media Router component extension so that it
can talk to the browser-side Media Router implementation via mojo. We do that
through extensions/renderer/dispatcher.cc so we cannot move out of extensions/
entirely.

[jam, 2017-04-05 16:40:22]

On 2017/04/04 20:31:29, takumif wrote:
> On 2017/04/04 19:28:55, jam wrote:
> > On 2017/04/04 18:08:33, takumif wrote:
> > > On 2017/04/04 17:58:52, jam wrote:
> > > > did you consider just moving the file in extensions/ up to chrome? I
> > > personally
> > > > find the chrome vs extensions split a bit odd sometimes, since it seems
> like
> > > > it's in a halfway state.
> > > > 
> > > > anyways, looking at 
> extensions/renderer/resources/media_router_bindings.js:
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/extensions/renderer/resources/media_rout...
> > > > 
> > > > it already lists a bunch of mojoms in chrome/browser/media. this
suggests
> > that
> > > > this file doesn't really belong in extensions/
> > > 
> > > If we moved media_router_bindings.js to chrome/, we'd now have to depend
on
> > that
> > > from extensions/.
> > 
> > can you explain why?
> > 
> > > This CL moves all the mojoms the file depends on to
> components/media_router/.
> 
> We expose the mojo interfaces to the Media Router component extension so that
it
> can talk to the browser-side Media Router implementation via mojo. We do that
> through extensions/renderer/dispatcher.cc so we cannot move out of extensions/
> entirely.

yes, but the dispatcher also gives its delegate in chrome a chance to add extra
interfaces, so why not move the media_router stuff there?
https://cs.chromium.org/chromium/src/chrome/renderer/extensions/chrome_extens...

[imcheng, 2017-04-05 17:11:46]

On 2017/04/05 16:40:22, jam (busy till April 10) wrote:
> On 2017/04/04 20:31:29, takumif wrote:
> > On 2017/04/04 19:28:55, jam wrote:
> > > On 2017/04/04 18:08:33, takumif wrote:
> > > > On 2017/04/04 17:58:52, jam wrote:
> > > > > did you consider just moving the file in extensions/ up to chrome? I
> > > > personally
> > > > > find the chrome vs extensions split a bit odd sometimes, since it
seems
> > like
> > > > > it's in a halfway state.
> > > > > 
> > > > > anyways, looking at 
> > extensions/renderer/resources/media_router_bindings.js:
> > > > >
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/extensions/renderer/resources/media_rout...
> > > > > 
> > > > > it already lists a bunch of mojoms in chrome/browser/media. this
> suggests
> > > that
> > > > > this file doesn't really belong in extensions/
> > > > 
> > > > If we moved media_router_bindings.js to chrome/, we'd now have to depend
> on
> > > that
> > > > from extensions/.
> > > 
> > > can you explain why?
> > > 
> > > > This CL moves all the mojoms the file depends on to
> > components/media_router/.
> > 
> > We expose the mojo interfaces to the Media Router component extension so
that
> it
> > can talk to the browser-side Media Router implementation via mojo. We do
that
> > through extensions/renderer/dispatcher.cc so we cannot move out of
extensions/
> > entirely.
> 
> yes, but the dispatcher also gives its delegate in chrome a chance to add
extra
> interfaces, so why not move the media_router stuff there?
>
https://cs.chromium.org/chromium/src/chrome/renderer/extensions/chrome_extens...

jam@, good question. 

My thought originally was to move everything to components/media_router but it
looks like there are some chrome/ dependencies that we can't quite get rid of
for now and will require rearchitecting some of our APIs which may or may not
happen. It's also more work to shuffle many files around and maintain code in
both components/ and chrome. 

We can certainly move media_router_bindings.js to chrome/extensions. In
particular, chrome/renderer/resources/renderer_resources.grd (thanks Devlin for
the info) and the dispatcher delegate file you mentioned.
At the same time, we should also move the Media Router related mojom files out
of chrome/browser. chrome/common/media_router is a good place for this as we
already have a folder with a mojom file there.

Let me know what you think.

Takumi, sorry for going back and forth about this, I think we all want the
simplest way that is correct. :)

[jam, 2017-04-10 15:43:07]

On 2017/04/05 17:11:46, imcheng wrote:
> On 2017/04/05 16:40:22, jam (busy till April 10) wrote:
> > On 2017/04/04 20:31:29, takumif wrote:
> > > On 2017/04/04 19:28:55, jam wrote:
> > > > On 2017/04/04 18:08:33, takumif wrote:
> > > > > On 2017/04/04 17:58:52, jam wrote:
> > > > > > did you consider just moving the file in extensions/ up to chrome? I
> > > > > personally
> > > > > > find the chrome vs extensions split a bit odd sometimes, since it
> seems
> > > like
> > > > > > it's in a halfway state.
> > > > > > 
> > > > > > anyways, looking at 
> > > extensions/renderer/resources/media_router_bindings.js:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/extensions/renderer/resources/media_rout...
> > > > > > 
> > > > > > it already lists a bunch of mojoms in chrome/browser/media. this
> > suggests
> > > > that
> > > > > > this file doesn't really belong in extensions/
> > > > > 
> > > > > If we moved media_router_bindings.js to chrome/, we'd now have to
depend
> > on
> > > > that
> > > > > from extensions/.
> > > > 
> > > > can you explain why?
> > > > 
> > > > > This CL moves all the mojoms the file depends on to
> > > components/media_router/.
> > > 
> > > We expose the mojo interfaces to the Media Router component extension so
> that
> > it
> > > can talk to the browser-side Media Router implementation via mojo. We do
> that
> > > through extensions/renderer/dispatcher.cc so we cannot move out of
> extensions/
> > > entirely.
> > 
> > yes, but the dispatcher also gives its delegate in chrome a chance to add
> extra
> > interfaces, so why not move the media_router stuff there?
> >
>
https://cs.chromium.org/chromium/src/chrome/renderer/extensions/chrome_extens...
> 
> jam@, good question. 
> 
> My thought originally was to move everything to components/media_router but it
> looks like there are some chrome/ dependencies that we can't quite get rid of
> for now and will require rearchitecting some of our APIs which may or may not
> happen. It's also more work to shuffle many files around and maintain code in
> both components/ and chrome. 
> 
> We can certainly move media_router_bindings.js to chrome/extensions. In
> particular, chrome/renderer/resources/renderer_resources.grd (thanks Devlin
for
> the info) and the dispatcher delegate file you mentioned.
> At the same time, we should also move the Media Router related mojom files out
> of chrome/browser. chrome/common/media_router is a good place for this as we
> already have a folder with a mojom file there.
> 
> Let me know what you think.
> 
> Takumi, sorry for going back and forth about this, I think we all want the
> simplest way that is correct. :)

[jam, 2017-04-10 15:43:13]

On 2017/04/05 17:11:46, imcheng wrote:
> On 2017/04/05 16:40:22, jam (busy till April 10) wrote:
> > On 2017/04/04 20:31:29, takumif wrote:
> > > On 2017/04/04 19:28:55, jam wrote:
> > > > On 2017/04/04 18:08:33, takumif wrote:
> > > > > On 2017/04/04 17:58:52, jam wrote:
> > > > > > did you consider just moving the file in extensions/ up to chrome? I
> > > > > personally
> > > > > > find the chrome vs extensions split a bit odd sometimes, since it
> seems
> > > like
> > > > > > it's in a halfway state.
> > > > > > 
> > > > > > anyways, looking at 
> > > extensions/renderer/resources/media_router_bindings.js:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/extensions/renderer/resources/media_rout...
> > > > > > 
> > > > > > it already lists a bunch of mojoms in chrome/browser/media. this
> > suggests
> > > > that
> > > > > > this file doesn't really belong in extensions/
> > > > > 
> > > > > If we moved media_router_bindings.js to chrome/, we'd now have to
depend
> > on
> > > > that
> > > > > from extensions/.
> > > > 
> > > > can you explain why?
> > > > 
> > > > > This CL moves all the mojoms the file depends on to
> > > components/media_router/.
> > > 
> > > We expose the mojo interfaces to the Media Router component extension so
> that
> > it
> > > can talk to the browser-side Media Router implementation via mojo. We do
> that
> > > through extensions/renderer/dispatcher.cc so we cannot move out of
> extensions/
> > > entirely.
> > 
> > yes, but the dispatcher also gives its delegate in chrome a chance to add
> extra
> > interfaces, so why not move the media_router stuff there?
> >
>
https://cs.chromium.org/chromium/src/chrome/renderer/extensions/chrome_extens...
> 
> jam@, good question. 
> 
> My thought originally was to move everything to components/media_router but it
> looks like there are some chrome/ dependencies that we can't quite get rid of
> for now and will require rearchitecting some of our APIs which may or may not
> happen. It's also more work to shuffle many files around and maintain code in
> both components/ and chrome. 
> 
> We can certainly move media_router_bindings.js to chrome/extensions. In
> particular, chrome/renderer/resources/renderer_resources.grd (thanks Devlin
for
> the info) and the dispatcher delegate file you mentioned.
> At the same time, we should also move the Media Router related mojom files out
> of chrome/browser. chrome/common/media_router is a good place for this as we
> already have a folder with a mojom file there.
> 
> Let me know what you think.

Yep this is what I meant :)
> 
> Takumi, sorry for going back and forth about this, I think we all want the
> simplest way that is correct. :)

[takumif, 2017-04-12 00:33:26]

On 2017/04/10 15:43:13, jam wrote:
> On 2017/04/05 17:11:46, imcheng wrote:
> > On 2017/04/05 16:40:22, jam (busy till April 10) wrote:
> > > On 2017/04/04 20:31:29, takumif wrote:
> > > > On 2017/04/04 19:28:55, jam wrote:
> > > > > On 2017/04/04 18:08:33, takumif wrote:
> > > > > > On 2017/04/04 17:58:52, jam wrote:
> > > > > > > did you consider just moving the file in extensions/ up to chrome?
I
> > > > > > personally
> > > > > > > find the chrome vs extensions split a bit odd sometimes, since it
> > seems
> > > > like
> > > > > > > it's in a halfway state.
> > > > > > > 
> > > > > > > anyways, looking at 
> > > > extensions/renderer/resources/media_router_bindings.js:
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://cs.chromium.org/chromium/src/extensions/renderer/resources/media_rout...
> > > > > > > 
> > > > > > > it already lists a bunch of mojoms in chrome/browser/media. this
> > > suggests
> > > > > that
> > > > > > > this file doesn't really belong in extensions/
> > > > > > 
> > > > > > If we moved media_router_bindings.js to chrome/, we'd now have to
> depend
> > > on
> > > > > that
> > > > > > from extensions/.
> > > > > 
> > > > > can you explain why?
> > > > > 
> > > > > > This CL moves all the mojoms the file depends on to
> > > > components/media_router/.
> > > > 
> > > > We expose the mojo interfaces to the Media Router component extension so
> > that
> > > it
> > > > can talk to the browser-side Media Router implementation via mojo. We do
> > that
> > > > through extensions/renderer/dispatcher.cc so we cannot move out of
> > extensions/
> > > > entirely.
> > > 
> > > yes, but the dispatcher also gives its delegate in chrome a chance to add
> > extra
> > > interfaces, so why not move the media_router stuff there?
> > >
> >
>
https://cs.chromium.org/chromium/src/chrome/renderer/extensions/chrome_extens...
> > 
> > jam@, good question. 
> > 
> > My thought originally was to move everything to components/media_router but
it
> > looks like there are some chrome/ dependencies that we can't quite get rid
of
> > for now and will require rearchitecting some of our APIs which may or may
not
> > happen. It's also more work to shuffle many files around and maintain code
in
> > both components/ and chrome. 
> > 
> > We can certainly move media_router_bindings.js to chrome/extensions. In
> > particular, chrome/renderer/resources/renderer_resources.grd (thanks Devlin
> for
> > the info) and the dispatcher delegate file you mentioned.
> > At the same time, we should also move the Media Router related mojom files
out
> > of chrome/browser. chrome/common/media_router is a good place for this as we
> > already have a folder with a mojom file there.
> > 
> > Let me know what you think.
> 
> Yep this is what I meant :)
> > 
> > Takumi, sorry for going back and forth about this, I think we all want the
> > simplest way that is correct. :)

Discussed this with Derek offline. If we move media_router_bindings.js to
chrome/renderer/ and the .mojom files to chrome/common/, we'd still have to move
all the files above there because we wouldn't be able to depend on
chrome/browser/ from chrome/common/. That said, it'd still be preferable to have
the code split between chrome/common/ and chrome/browser/ rather than between
components/ and chrome/browser/, so I'll be updating this CL to move the files
to chrome/common/.

[takumif, 2017-04-13 17:48:04]

The CQ bit was checked by takumif@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 17:49:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[takumif, 2017-04-13 17:54:22]

Description was changed from

==========
Move c/b/media/router/mojo/*.mojom to components/media_router/mojo/*.mojom

We have a file in extensions/ [1] that depends on Media Router mojo definitions
from chrome/browser/. To remove this dependency from extensions/ to
chrome/browser/, this CL moves Media Router mojo interfaces to
components/media_router/mojo/. Media Router typemaps and related C++ structs are
also moved to components/media_router/.

[1] extensions/renderer/resources/media_router_bindings.js

BUG=704958
==========

to

==========
Move c/b/media/router/mojo/*.mojom to chrome/common/media_router/mojo/*.mojom

We have a file in extensions/ [1] that depends on Media Router mojo definitions
from chrome/browser/. To remove this dependency from extensions/ to
chrome/browser/, this CL moves that file to chrome/renderer/, and Media Router
mojo interfaces to chrome/common/media_router/mojo/. Media Router typemaps and
related C++ structs are also moved to chrome/common/media_router/.

[1] extensions/renderer/resources/media_router_bindings.js

BUG=704958
==========

[takumif, 2017-04-13 18:00:04]

As explained in the above comment, I've moved the files to
chrome/common/media_router/. Please take a look at:
imcheng: overall
jam: chromium_bindings_configuration.gni
rdcronin: chrome/renderer/*/extensions/

Thanks!

[commit-bot: I haz the power, 2017-04-13 18:05:22]

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

[commit-bot: I haz the power, 2017-04-13 18:05:24]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[takumif, 2017-04-13 18:20:25]

The CQ bit was checked by takumif@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 18:21:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-13 18:49:20]

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

[commit-bot: I haz the power, 2017-04-13 18:49:23]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[takumif, 2017-04-13 18:56:44]

Patchset #16 (id:440001) has been deleted

[takumif, 2017-04-13 18:57:03]

The CQ bit was checked by takumif@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 18:57:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-13 20:03:53]

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

[commit-bot: I haz the power, 2017-04-13 20:03:56]

Dry run: This issue passed the CQ dry run.

[imcheng, 2017-04-13 23:44:32]

lgtm

I looked through the .gn/.gni/extensions files and skimmed through a few .h/.cc
that were moved to chrome/common/media_router.

https://codereview.chromium.org/2771413003/diff/460001/mojo/public/tools/bind...
File mojo/public/tools/bindings/chromium_bindings_configuration.gni (right):

https://codereview.chromium.org/2771413003/diff/460001/mojo/public/tools/bind...
mojo/public/tools/bindings/chromium_bindings_configuration.gni:12:
"//chrome/common/media_router/mojo/typemaps.gni",
nit: please fix ordering

[takumif, 2017-04-14 19:05:40]

The CQ bit was checked by takumif@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-14 19:06:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[takumif, 2017-04-14 19:09:54]

https://codereview.chromium.org/2771413003/diff/460001/mojo/public/tools/bind...
File mojo/public/tools/bindings/chromium_bindings_configuration.gni (right):

https://codereview.chromium.org/2771413003/diff/460001/mojo/public/tools/bind...
mojo/public/tools/bindings/chromium_bindings_configuration.gni:12:
"//chrome/common/media_router/mojo/typemaps.gni",
On 2017/04/13 23:44:32, imcheng wrote:
> nit: please fix ordering

Done.

[commit-bot: I haz the power, 2017-04-14 21:13:59]

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

[commit-bot: I haz the power, 2017-04-14 21:14:02]

Dry run: This issue passed the CQ dry run.

[jam, 2017-04-17 15:16:16]

lgtm

[takumif, 2017-04-17 18:56:28]

The CQ bit was checked by takumif@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-17 18:56:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-17 20:06:28]

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

[commit-bot: I haz the power, 2017-04-17 20:06:30]

Dry run: This issue passed the CQ dry run.

[Nico, 2017-04-18 13:50:43]

The CQ bit was checked by thakis@chromium.org

[Nico, 2017-04-18 13:50:44]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org,
xhwang@chromium.org, rdevlin.cronin@chromium.org, thakis@chromium.org,
sdefresne@chromium.org, mfoltz@chromium.org, imcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2771413003/#ps480001
(title: "Address Derek's comment")

[commit-bot: I haz the power, 2017-04-18 13:51:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-18 14:00:58]

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

[commit-bot: I haz the power, 2017-04-18 14:01:01]

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

[takumif, 2017-04-18 22:43:17]

Devlin, I just wanted to confirm that it's okay to move media_router_bindings.js
to chrome/renderer/resources/extensions/. Thanks!

[Devlin, 2017-04-18 23:04:06]

On 2017/04/18 22:43:17, takumif wrote:
> Devlin, I just wanted to confirm that it's okay to move
media_router_bindings.js
> to chrome/renderer/resources/extensions/. Thanks!

Yes, it's much better to have these in c/r/resources/extensions than in
extensions/renderer/resources.  Going forward, I might prefer it yet more for
them to be separate from extensions, but that need not hold up this CL, since
this is an improvement.

LGTM

[takumif, 2017-04-18 23:43:59]

The CQ bit was checked by takumif@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-18 23:44:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 00:48:56]

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

[commit-bot: I haz the power, 2017-04-19 00:48:58]

Dry run: This issue passed the CQ dry run.

[Nico, 2017-04-19 20:26:47]

The CQ bit was checked by thakis@chromium.org

[Nico, 2017-04-19 20:26:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, mfoltz@chromium.org, jam@chromium.org,
dcheng@chromium.org, thakis@chromium.org, sdefresne@chromium.org,
imcheng@chromium.org, xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/2771413003/#ps500001
(title: "Rebase")

[commit-bot: I haz the power, 2017-04-19 20:27:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 20:38:07]

CQ is committing da patch.

Bot data: {"patchset_id": 500001, "attempt_start_ts": 1492633607307830,
"parent_rev": "cb7c7859e8e84236d85aca12005c22ce707ebee4", "commit_rev":
"a34e5193d3994da1e822b7259462ab3c67a14467"}

[commit-bot: I haz the power, 2017-04-19 20:38:31]

Description was changed from

==========
Move c/b/media/router/mojo/*.mojom to chrome/common/media_router/mojo/*.mojom

We have a file in extensions/ [1] that depends on Media Router mojo definitions
from chrome/browser/. To remove this dependency from extensions/ to
chrome/browser/, this CL moves that file to chrome/renderer/, and Media Router
mojo interfaces to chrome/common/media_router/mojo/. Media Router typemaps and
related C++ structs are also moved to chrome/common/media_router/.

[1] extensions/renderer/resources/media_router_bindings.js

BUG=704958
==========

to

==========
Move c/b/media/router/mojo/*.mojom to chrome/common/media_router/mojo/*.mojom

We have a file in extensions/ [1] that depends on Media Router mojo definitions
from chrome/browser/. To remove this dependency from extensions/ to
chrome/browser/, this CL moves that file to chrome/renderer/, and Media Router
mojo interfaces to chrome/common/media_router/mojo/. Media Router typemaps and
related C++ structs are also moved to chrome/common/media_router/.

[1] extensions/renderer/resources/media_router_bindings.js

BUG=704958

Review-Url: https://codereview.chromium.org/2771413003
Cr-Commit-Position: refs/heads/master@{#465734}
Committed:
https://chromium.googlesource.com/chromium/src/+/a34e5193d3994da1e822b7259462...
==========

[commit-bot: I haz the power, 2017-04-19 20:38:34]

Committed patchset #18 (id:500001) as
https://chromium.googlesource.com/chromium/src/+/a34e5193d3994da1e822b7259462...

[Nico, 2017-04-20 14:57:50]

https://codereview.chromium.org/2771413003/diff/500001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/2771413003/diff/500001/chrome/test/BUILD.gn#n...
chrome/test/BUILD.gn:4446: "../common/media_router/media_source_unittest.cc",
These 6 files are listed both here and in
https://codereview.chromium.org/2771413003/diff/500001/chrome/common/media_ro...
-- is that intentional?

[Nico, 2017-04-20 15:00:57]

https://codereview.chromium.org/2771413003/diff/500001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/2771413003/diff/500001/chrome/test/BUILD.gn#n...
chrome/test/BUILD.gn:4475:
"../common/media_router/mojo/media_router_struct_traits_unittest.cc",
This one too is listed in both places.

[takumif, 2017-04-20 18:15:36]

https://codereview.chromium.org/2771413003/diff/500001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/2771413003/diff/500001/chrome/test/BUILD.gn#n...
chrome/test/BUILD.gn:4446: "../common/media_router/media_source_unittest.cc",
On 2017/04/20 14:57:49, Nico wrote:
> These 6 files are listed both here and in
>
https://codereview.chromium.org/2771413003/diff/500001/chrome/common/media_ro...
> -- is that intentional?

No, I'll remove them from the other file.

https://codereview.chromium.org/2771413003/diff/500001/chrome/test/BUILD.gn#n...
chrome/test/BUILD.gn:4475:
"../common/media_router/mojo/media_router_struct_traits_unittest.cc",
On 2017/04/20 15:00:57, Nico wrote:
> This one too is listed in both places.

Acknowledged.

[takumif, 2017-04-20 21:04:45]

https://codereview.chromium.org/2771413003/diff/500001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/2771413003/diff/500001/chrome/test/BUILD.gn#n...
chrome/test/BUILD.gn:4446: "../common/media_router/media_source_unittest.cc",
On 2017/04/20 18:15:35, takumif wrote:
> On 2017/04/20 14:57:49, Nico wrote:
> > These 6 files are listed both here and in
> >
>
https://codereview.chromium.org/2771413003/diff/500001/chrome/common/media_ro...
> > -- is that intentional?
> 
> No, I'll remove them from the other file.

Landed a fix:
https://codereview.chromium.org/2834573003/