Convert components/native_app_window.gypi to a gyp to fix a cycle

Convert components/native_app_window.gypi to a gyp to fix a cycle

There's currently a dependency cycle between gyp files: components.gyp
and extensions.gyp. extensions.gyp depends on a number of components,
and two components depend on exetensions.gyp. These are:
  - components/renderer_context_menu/
  - components/native_app_window/ (more recently)

For native_app_window, since it is not compiled on Mac, the gyp circular
check is not performed on the bots. However, we soon want to compile it
on Mac.

This CL fixes the cycle between extensions.gyp and components.gyp by
converting components/native_app_window.gypi to a gyp.

BUG=35878, 418455

[tapted, 2014-09-30 03:32:15]

Patchset #4 (id:60001) has been deleted

[tapted, 2014-09-30 03:32:23]

Patchset #3 (id:40001) has been deleted

[tapted, 2014-09-30 04:04:09]

tapted@chromium.org changed reviewers:
+ jamescook@chromium.org, jochen@chromium.org, oshima@chromium.org

[tapted, 2014-09-30 04:10:32]

**alternative to http://crrev.com/606953002 **

jochen: is this still a correct way to use src/components?

I think I prefer it over the other CL

(oshima/jamescook: WDYT?)

[jochen (gone - plz use gerrit), 2014-09-30 08:41:17]

jochen@chromium.org changed reviewers:
+ blundell@chromium.org, caitkp@chromium.org, erikwright@chromium.org,
kalman@chromium.org

[jochen (gone - plz use gerrit), 2014-09-30 08:41:18]

+other components owners
+kalman for extensions

hum, what's the layering relating between components and extensions?

I'd prefer if there was a clear separation, i.e. only one may depend on the
other but not the other way round

[blundell, 2014-09-30 09:02:43]

Yeah, I don't think this is the right solution. I think that //extensions should
live strictly above //components.

[tapted, 2014-09-30 09:11:24]

On 2014/09/30 08:41:18, jochen (slow for reviews) wrote:
> +other components owners
> +kalman for extensions
> 
> hum, what's the layering relating between components and extensions?

extensions depends on many components [1]. In fact I think a number of
components were motivated by the desire to extract a dependency from
chrome/browser into a component so that extension's dependencies on
chrome/browser could be untangled.

Going the other way, there are two components with extensions dependencies:
native_app_window and renderer_context_menu. renderer_context_menu lists
extensions in DEPS but not in gypi, which allows it to avoid the gypfile-cycle
check. However, between the targets themselves, there is not a cycle.

[1] extensions -> components dependencies: crx_file keyed_service_content
keyed_service_core pref_registry sessions storage_monitor url_matcher user_prefs
web_cache_browser web_modal

> I'd prefer if there was a clear separation, i.e. only one may depend on the
> other but not the other way round

I think this could look like the alternative I have up in
http://crrev.com/606953002/ . It introduces a "src/extensions/components" folder
to solve layering on a folder basis. However, there are questions about whether
we want a second "components" folder in Chrome, in order to provide something
that can depend on extensions.

[blundell, 2014-09-30 09:53:06]

On 2014/09/30 09:11:24, tapted wrote:
> On 2014/09/30 08:41:18, jochen (slow for reviews) wrote:
> > +other components owners
> > +kalman for extensions
> > 
> > hum, what's the layering relating between components and extensions?
> 
> extensions depends on many components [1]. In fact I think a number of
> components were motivated by the desire to extract a dependency from
> chrome/browser into a component so that extension's dependencies on
> chrome/browser could be untangled.
> 
> Going the other way, there are two components with extensions dependencies:
> native_app_window and renderer_context_menu. renderer_context_menu lists
> extensions in DEPS but not in gypi, which allows it to avoid the gypfile-cycle
> check. However, between the targets themselves, there is not a cycle.
> 
> [1] extensions -> components dependencies: crx_file keyed_service_content
> keyed_service_core pref_registry sessions storage_monitor url_matcher
user_prefs
> web_cache_browser web_modal
> 
> > I'd prefer if there was a clear separation, i.e. only one may depend on the
> > other but not the other way round
> 
> I think this could look like the alternative I have up in
> http://crrev.com/606953002/ . It introduces a "src/extensions/components"
folder
> to solve layering on a folder basis. However, there are questions about
whether
> we want a second "components" folder in Chrome, in order to provide something
> that can depend on extensions.

Naive question: Why not just in //extensions/native_app_window and
//extensions/renderer_context_menu, or something like that?

[tapted, 2014-09-30 11:21:43]

On 2014/09/30 09:53:06, blundell wrote:
> Naive question: Why not just in //extensions/native_app_window and
> //extensions/renderer_context_menu, or something like that?

I guess the goal in making extensions/components would be to make it clear that
it's not "part" of extensions. E.g. native_app_window introduces a dependency on
a particular UI toolkit, which shouldn't belong in extensions. For
renderer_context_menu, it actually compiles in two modes based on whether
enable_extensions is set (i.e. it sometimes doesn't have an extensions
dependency at all).

[blundell, 2014-09-30 11:32:36]

On 2014/09/30 11:21:43, tapted wrote:
> On 2014/09/30 09:53:06, blundell wrote:
> > Naive question: Why not just in //extensions/native_app_window and
> > //extensions/renderer_context_menu, or something like that?
> 
> I guess the goal in making extensions/components would be to make it clear
that
> it's not "part" of extensions. E.g. native_app_window introduces a dependency
on
> a particular UI toolkit, which shouldn't belong in extensions.

Ah! It looks like this is an implementation of an interface in //extensions? So
maybe something like //extensions/views/native_app_window_views.* ?

For
> renderer_context_menu, it actually compiles in two modes based on whether
> enable_extensions is set (i.e. it sometimes doesn't have an extensions
> dependency at all).

The //extensions dependency in this component seems really thin. Would it make
sense to just have a GetContextMenuExtensionForWebContents(WebContents*) [or
whatever name is appropriate] that lives somewhere in //extensions instead?

It would be really nice to avoid //extensions/components for sure.

[erikwright (departed), 2014-09-30 13:58:09]

On 2014/09/30 11:32:36, blundell wrote:
> On 2014/09/30 11:21:43, tapted wrote:
> > On 2014/09/30 09:53:06, blundell wrote:
> > > Naive question: Why not just in //extensions/native_app_window and
> > > //extensions/renderer_context_menu, or something like that?
> > 
> > I guess the goal in making extensions/components would be to make it clear
> that
> > it's not "part" of extensions. E.g. native_app_window introduces a
dependency
> on
> > a particular UI toolkit, which shouldn't belong in extensions.
> 
> Ah! It looks like this is an implementation of an interface in //extensions?
So
> maybe something like //extensions/views/native_app_window_views.* ?
> 
> For
> > renderer_context_menu, it actually compiles in two modes based on whether
> > enable_extensions is set (i.e. it sometimes doesn't have an extensions
> > dependency at all).
> 
> The //extensions dependency in this component seems really thin. Would it make
> sense to just have a GetContextMenuExtensionForWebContents(WebContents*) [or
> whatever name is appropriate] that lives somewhere in //extensions instead?
> 
> It would be really nice to avoid //extensions/components for sure.

I have only skimmed the comments and CL description, but my naive approach to
removing a dependency from this component towards extensions would be, as Colin
suggests, to define some kind of delegate API or other means of indirection
allowing the embedder (Chrome) to connect the component to its dependencies.
This is how every other component dependency is satisfied.

[oshima, 2014-09-30 14:05:39]

On 2014/09/30 11:32:36, blundell wrote:
> On 2014/09/30 11:21:43, tapted wrote:
> > On 2014/09/30 09:53:06, blundell wrote:
> > > Naive question: Why not just in //extensions/native_app_window and
> > > //extensions/renderer_context_menu, or something like that?
> > 
> > I guess the goal in making extensions/components would be to make it clear
> that
> > it's not "part" of extensions. E.g. native_app_window introduces a
dependency
> on
> > a particular UI toolkit, which shouldn't belong in extensions.
> 
> Ah! It looks like this is an implementation of an interface in //extensions?
So
> maybe something like //extensions/views/native_app_window_views.* ?

Extension folks wanted to avoid having views dependency under extensions.
If there is no other way, maybe we have to do this.

> 
> For
> > renderer_context_menu, it actually compiles in two modes based on whether
> > enable_extensions is set (i.e. it sometimes doesn't have an extensions
> > dependency at all).
> 
> The //extensions dependency in this component seems really thin. Would it make
> sense to just have a GetContextMenuExtensionForWebContents(WebContents*) [or
> whatever name is appropriate] that lives somewhere in //extensions instead?
> 
> It would be really nice to avoid //extensions/components for sure.

I guess we can put these code in //extensions/util ? I'm not sure if extensions
folks like it as it will add code that is not directly related to extensions
itself.

I understand now that we don't want //extensions in //components, although
It's unfortunate that there is no common place for a component that requires
//extensions.

[blundell, 2014-09-30 14:07:41]

On 2014/09/30 14:05:39, oshima wrote:
> On 2014/09/30 11:32:36, blundell wrote:
> > On 2014/09/30 11:21:43, tapted wrote:
> > > On 2014/09/30 09:53:06, blundell wrote:
> > > > Naive question: Why not just in //extensions/native_app_window and
> > > > //extensions/renderer_context_menu, or something like that?
> > > 
> > > I guess the goal in making extensions/components would be to make it clear
> > that
> > > it's not "part" of extensions. E.g. native_app_window introduces a
> dependency
> > on
> > > a particular UI toolkit, which shouldn't belong in extensions.
> > 
> > Ah! It looks like this is an implementation of an interface in //extensions?
> So
> > maybe something like //extensions/views/native_app_window_views.* ?
> 
> Extension folks wanted to avoid having views dependency under extensions.
> If there is no other way, maybe we have to do this.
> 
> > 
> > For
> > > renderer_context_menu, it actually compiles in two modes based on whether
> > > enable_extensions is set (i.e. it sometimes doesn't have an extensions
> > > dependency at all).
> > 
> > The //extensions dependency in this component seems really thin. Would it
make
> > sense to just have a GetContextMenuExtensionForWebContents(WebContents*) [or
> > whatever name is appropriate] that lives somewhere in //extensions instead?
> > 
> > It would be really nice to avoid //extensions/components for sure.
> 
> I guess we can put these code in //extensions/util ? I'm not sure if
extensions
> folks like it as it will add code that is not directly related to extensions
> itself.
> 
> I understand now that we don't want //extensions in //components, although
> It's unfortunate that there is no common place for a component that requires
> //extensions.

Which embedders use these components? //chrome and //athena?

[oshima, 2014-09-30 14:53:11]

On 2014/09/30 14:07:41, blundell wrote:
> On 2014/09/30 14:05:39, oshima wrote:
> > On 2014/09/30 11:32:36, blundell wrote:
> > > On 2014/09/30 11:21:43, tapted wrote:
> > > > On 2014/09/30 09:53:06, blundell wrote:
> > > > > Naive question: Why not just in //extensions/native_app_window and
> > > > > //extensions/renderer_context_menu, or something like that?
> > > > 
> > > > I guess the goal in making extensions/components would be to make it
clear
> > > that
> > > > it's not "part" of extensions. E.g. native_app_window introduces a
> > dependency
> > > on
> > > > a particular UI toolkit, which shouldn't belong in extensions.
> > > 
> > > Ah! It looks like this is an implementation of an interface in
//extensions?
> > So
> > > maybe something like //extensions/views/native_app_window_views.* ?
> > 
> > Extension folks wanted to avoid having views dependency under extensions.
> > If there is no other way, maybe we have to do this.
> > 
> > > 
> > > For
> > > > renderer_context_menu, it actually compiles in two modes based on
whether
> > > > enable_extensions is set (i.e. it sometimes doesn't have an extensions
> > > > dependency at all).
> > > 
> > > The //extensions dependency in this component seems really thin. Would it
> make
> > > sense to just have a GetContextMenuExtensionForWebContents(WebContents*)
[or
> > > whatever name is appropriate] that lives somewhere in //extensions
instead?
> > > 
> > > It would be really nice to avoid //extensions/components for sure.
> > 
> > I guess we can put these code in //extensions/util ? I'm not sure if
> extensions
> > folks like it as it will add code that is not directly related to extensions
> > itself.
> > 
> > I understand now that we don't want //extensions in //components, although
> > It's unfortunate that there is no common place for a component that requires
> > //extensions.
> 
> Which embedders use these components? //chrome and //athena?

Both.

[James Cook, 2014-09-30 15:56:05]

I would be OK with //extensions/views or //extensions/browser/views or
//extensions/browser/ui/views as long as we had DEPS rules in place that forbid
dependencies on these directories from other //extensions code.  Likewise a
README in that directory could help.

Athena and app_shell and Chrome all depend on //extensions.

app_shell in particular wants to avoid picking up dependencies on views -- we're
concerned about size.

Sorry for the yak shave, but I'm glad you're sorting this out.

https://codereview.chromium.org/617693002/diff/80001/chrome/chrome_browser_ui...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/617693002/diff/80001/chrome/chrome_browser_ui...
chrome/chrome_browser_ui.gypi:2741: 'dependencies': [
'<(DEPTH)/components/native_app_window/native_app_window.gyp:native_app_window'
],
nit: This line is a little long, I might wrap it

[James Cook, 2014-09-30 16:28:20]

On 2014/09/30 15:56:05, James Cook wrote:
> I would be OK with //extensions/views or //extensions/browser/views or
> //extensions/browser/ui/views as long as we had DEPS rules in place that
forbid
> dependencies on these directories from other //extensions code.  Likewise a
> README in that directory could help.
> 
> Athena and app_shell and Chrome all depend on //extensions.
> 
> app_shell in particular wants to avoid picking up dependencies on views --
we're
> concerned about size.
> 
> Sorry for the yak shave, but I'm glad you're sorting this out.
> 
>
https://codereview.chromium.org/617693002/diff/80001/chrome/chrome_browser_ui...
> File chrome/chrome_browser_ui.gypi (right):
> 
>
https://codereview.chromium.org/617693002/diff/80001/chrome/chrome_browser_ui...
> chrome/chrome_browser_ui.gypi:2741: 'dependencies': [
>
'<(DEPTH)/components/native_app_window/native_app_window.gyp:native_app_window'
> ],
> nit: This line is a little long, I might wrap it

Talked to oshima, I think //extensions/components is better than
//extensions/browser/views because it makes very clear that these are modules of
code to be used by embedders *outside* the extensions module.

[tapted, 2014-09-30 21:35:53]

On 2014/09/30 16:28:20, James Cook wrote:
> Talked to oshima, I think //extensions/components is better than
> //extensions/browser/views because it makes very clear that these are modules
of
> code to be used by embedders *outside* the extensions module.

Sweet - I think //extensions/components is most of the way there in
http://crrev.com/606953002 . I'll make sure I have buy-in from extensions
OWNERS. If anyone else would like to see how it feels and/or comment, please
have a look in http://crrev.com/606953002

I'll close this CL.