Move components/native_app_window to extensions/components/native_app_window

Move components/native_app_window to extensions/components/native_app_window

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)

renderer_context_menu has an "optional" extensions dependency, which can
be skipped when ENABLE_EXTENSIONS is not defined. Still, it is necessary
for renderer_context_menu.gyp to omit its extensions.gyp dependency and
build as a static library.

For native_app_window, since it is not compiled on Mac, the gyp circular
check is not performed on the bots.

This CL fixes the layering by adding a folder,
src/extensions/components. Things here may depend on extensions, but not
on other extensions/components folders in a way that creates a cycle.
This also allows the benefits of a component-architecture to make sense
of interdependencies between the ~1337 files under src/extensions.

This layout also makes it clear that by depending on one of these
components, a target is also depending on src/extensions. Currently,
this is not clear.

BUG=35878, 418455
TBR=reed@google.com

Committed: https://crrev.com/344272ddd4ca47fb56fd6a1ca6a4f1418df2c426
Cr-Commit-Position: refs/heads/master@{#297952}

[tapted, 2014-09-26 04:28:40]

Patchset #2 (id:20001) has been deleted

[tapted, 2014-09-29 02:38:45]

Patchset #2 (id:40001) has been deleted

[tapted, 2014-09-29 05:27:18]

tapted@chromium.org changed reviewers:
+ jamescook@chromium.org

[tapted, 2014-09-29 05:28:12]

tapted@chromium.org changed reviewers:
+ scheib@chromium.org

[tapted, 2014-09-29 05:29:00]

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

[tapted, 2014-09-29 05:29:19]

tapted@chromium.org changed reviewers:
+ oshima@chromium.org

[tapted, 2014-09-29 06:03:31]

Hi, please take a look. This is a follow-up to http://crrev.com/295502 which
added components/native_app_window for athena, but had a subtle layering issue.
(I propose `src/extensions/components` to resolve it)

 - jochen: src/components
 - scheib: src/extensions
 - oshima: src/athena
 - jamescook: */native_app_window

More details on the bugs.

Thanks!

https://codereview.chromium.org/606953002/diff/100001/apps/BUILD.gn
File apps/BUILD.gn (right):

https://codereview.chromium.org/606953002/diff/100001/apps/BUILD.gn#newcode71
apps/BUILD.gn:71: "//extensions/components/native_app_window",
this dep was previously missing from the gn file (apps.gypi had it)

https://codereview.chromium.org/606953002/diff/100001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (left):

https://codereview.chromium.org/606953002/diff/100001/chrome/browser/BUILD.gn...
chrome/browser/BUILD.gn:79: "//components/native_app_window",
this dep here is not needed at all, since it's pulled in via apps when
toolkit_views is set (likewise in chrome_browser.gypi)

https://codereview.chromium.org/606953002/diff/100001/extensions/components/n...
File extensions/components/native_app_window/BUILD.gn (right):

https://codereview.chromium.org/606953002/diff/100001/extensions/components/n...
extensions/components/native_app_window/BUILD.gn:15:
"//ui/views/controls/webview",
This was a missing dep found by `gn check` - also added to
native_app_window.gypi

[James Cook, 2014-09-29 16:36:18]

jochen/oshima, thoughts?

I believe the primary motivation in the original CL was to make these components
available to Athena, which depends on //extensions and //content but does not
depend on //chrome (and hence can't depend on //apps since //apps is not
separate from chrome).

I want to make sure that app_shell (from //extensions/shell) stays small, and in
particular does not pick up any UI library dependencies like //ui/views.  This
CL doesn't add UI dependencies to app_shell or extensions, but it would be nice
to find a way to express that we're trying to avoid using particular UI toolkits
in //extensions.

I think I'm OK with this, but I'd like to hear oshima/jochen's opinion.

[oshima, 2014-09-29 18:47:15]

I believe we were going to exclude views impl of app window on mac. Didn't it
work?

https://codereview.chromium.org/606953002/diff/100001/apps/BUILD.gn
File apps/BUILD.gn (right):

https://codereview.chromium.org/606953002/diff/100001/apps/BUILD.gn#newcode71
apps/BUILD.gn:71: "//extensions/components/native_app_window",
On 2014/09/29 06:03:31, tapted wrote:
> this dep was previously missing from the gn file (apps.gypi had it)

This should be "extensions/browser"

https://codereview.chromium.org/606953002/diff/100001/apps/apps.gypi
File apps/apps.gypi (right):

https://codereview.chromium.org/606953002/diff/100001/apps/apps.gypi#newcode82
apps/apps.gypi:82:
'../extensions/components/extensions_components.gyp:native_app_window',
this shuld be extensions/browser

[scheib, 2014-09-29 22:37:54]

Is extensions/components/BUILD.gn needed?

https://codereview.chromium.org/606953002/diff/100001/extensions/components/B...
File extensions/components/BUILD.gn (right):

https://codereview.chromium.org/606953002/diff/100001/extensions/components/B...
extensions/components/BUILD.gn:11: group("extensions_components") {
Where is this group referenced from?

https://codereview.chromium.org/606953002/diff/100001/extensions/components/B...
extensions/components/BUILD.gn:12: testonly = true  # You shouldn't actually
link to this.
This code should be linked to non-test targets. Perhaps I'm confused by
'testonly'.

https://codereview.chromium.org/606953002/diff/100001/extensions/components/B...
extensions/components/BUILD.gn:13: visibility = [ "//:*" ]  # Only for the root
targets to bring in.
Why?

[tapted, 2014-09-29 23:22:42]

On 2014/09/29 18:47:15, oshima wrote:
> I believe we were going to exclude views impl of app window on mac. Didn't it
> work?

I think it will just delay the problem. App windows are probably one of the
first things we can switch over after toolkit_views on Mac. There is some
interest in reusing some of the window-shape and transparency stuff that is
currently missing a Cocoa implementation.

There's also a desire to remove the `--no-circular-check` flag that's added to
./build/gyp_chromium when the platform is not Mac. It's good at finding
dependency creep ( http://crbug.com/35878 ). But here I think there's also an
underlying layering issue that should be straightened out.

On 2014/09/29 22:37:54, scheib wrote:
> Is extensions/components/BUILD.gn needed?

an excellent question! You're right - it's not needed for this at all. This is
just following the pattern from src/components but it's not serving a purpose
yet. Deleted.

https://codereview.chromium.org/606953002/diff/100001/apps/BUILD.gn
File apps/BUILD.gn (right):

https://codereview.chromium.org/606953002/diff/100001/apps/BUILD.gn#newcode71
apps/BUILD.gn:71: "//extensions/components/native_app_window",
On 2014/09/29 18:47:14, oshima wrote:
> On 2014/09/29 06:03:31, tapted wrote:
> > this dep was previously missing from the gn file (apps.gypi had it)
> 
> This should be "extensions/browser"

I think this one is handled by the dependencies in native_app_window itself.

i.e. '../extensions.gyp:extensions_browser', in native_app_window.gypi and
"//extensions/browser", in "//extensions/components/native_app_window"'s deps

[tapted, 2014-09-29 23:25:53]

https://codereview.chromium.org/606953002/diff/100001/apps/BUILD.gn
File apps/BUILD.gn (right):

https://codereview.chromium.org/606953002/diff/100001/apps/BUILD.gn#newcode71
apps/BUILD.gn:71: "//extensions/components/native_app_window",
On 2014/09/29 23:22:41, tapted wrote:
> On 2014/09/29 18:47:14, oshima wrote:
> > On 2014/09/29 06:03:31, tapted wrote:
> > > this dep was previously missing from the gn file (apps.gypi had it)
> > 
> > This should be "extensions/browser"
> 
> I think this one is handled by the dependencies in native_app_window itself.
> 
> i.e. '../extensions.gyp:extensions_browser', in native_app_window.gypi and
> "//extensions/browser", in "//extensions/components/native_app_window"'s deps

oh oops. I see. There's not actually a need for apps to depend on
native_app_window at all - let me fix that.

[oshima, 2014-09-30 00:06:28]

On 2014/09/29 23:22:42, tapted wrote:
> On 2014/09/29 18:47:15, oshima wrote:
> > I believe we were going to exclude views impl of app window on mac. Didn't
it
> > work?
> 
> I think it will just delay the problem. App windows are probably one of the
> first things we can switch over after toolkit_views on Mac. There is some
> interest in reusing some of the window-shape and transparency stuff that is
> currently missing a Cocoa implementation.
> 
> There's also a desire to remove the `--no-circular-check` flag that's added to
> ./build/gyp_chromium when the platform is not Mac. It's good at finding
> dependency creep ( http://crbug.com/35878 ). But here I think there's also an
> underlying layering issue that should be straightened out.

If that's the case, my preference is to have
components/native_app_windows/native_app_windows.gyp
What's really broken is that there is one components/components.gyp for
unrelated targets.
(I guess you can't really detect a circular dependency between targets in
comopnents/ this way?)


> On 2014/09/29 22:37:54, scheib wrote:
> > Is extensions/components/BUILD.gn needed?
> 
> an excellent question! You're right - it's not needed for this at all. This is
> just following the pattern from src/components but it's not serving a purpose
> yet. Deleted.
> 
> https://codereview.chromium.org/606953002/diff/100001/apps/BUILD.gn
> File apps/BUILD.gn (right):
> 
> https://codereview.chromium.org/606953002/diff/100001/apps/BUILD.gn#newcode71
> apps/BUILD.gn:71: "//extensions/components/native_app_window",
> On 2014/09/29 18:47:14, oshima wrote:
> > On 2014/09/29 06:03:31, tapted wrote:
> > > this dep was previously missing from the gn file (apps.gypi had it)
> > 
> > This should be "extensions/browser"
> 
> I think this one is handled by the dependencies in native_app_window itself.
> 
> i.e. '../extensions.gyp:extensions_browser', in native_app_window.gypi and
> "//extensions/browser", in "//extensions/components/native_app_window"'s deps

[tapted, 2014-09-30 02:58:54]

Patchset #6 (id:140001) has been deleted

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

On 2014/09/30 00:06:28, oshima wrote:
> On 2014/09/29 23:22:42, tapted wrote:
> > On 2014/09/29 18:47:15, oshima wrote:
> > > I believe we were going to exclude views impl of app window on mac. Didn't
> it
> > > work?
> > 
> > I think it will just delay the problem. App windows are probably one of the
> > first things we can switch over after toolkit_views on Mac. There is some
> > interest in reusing some of the window-shape and transparency stuff that is
> > currently missing a Cocoa implementation.
> > 
> > There's also a desire to remove the `--no-circular-check` flag that's added
to
> > ./build/gyp_chromium when the platform is not Mac. It's good at finding
> > dependency creep ( http://crbug.com/35878 ). But here I think there's also
an
> > underlying layering issue that should be straightened out.
> 
> If that's the case, my preference is to have
> components/native_app_windows/native_app_windows.gyp
> What's really broken is that there is one components/components.gyp for
> unrelated targets.
> (I guess you can't really detect a circular dependency between targets in
> comopnents/ this way?)

That's a pretty good point. The single components.gyp means that gyp can't
enforce that there are no cycles between the various components gypi files. It
also means it runs into the problem that this CL is trying to address.

I've explored converting native_app_window.gypi into a regular gyp in
http://crrev.com/617693002/ it seems to work, and it's simpler than this. I'll
publish it out.

https://codereview.chromium.org/606953002/diff/100001/apps/BUILD.gn
File apps/BUILD.gn (right):

https://codereview.chromium.org/606953002/diff/100001/apps/BUILD.gn#newcode71
apps/BUILD.gn:71: "//extensions/components/native_app_window",
On 2014/09/29 18:47:14, oshima wrote:
> On 2014/09/29 06:03:31, tapted wrote:
> > this dep was previously missing from the gn file (apps.gypi had it)
> 
> This should be "extensions/browser"

Done.

https://codereview.chromium.org/606953002/diff/100001/apps/apps.gypi
File apps/apps.gypi (right):

https://codereview.chromium.org/606953002/diff/100001/apps/apps.gypi#newcode82
apps/apps.gypi:82:
'../extensions/components/extensions_components.gyp:native_app_window',
On 2014/09/29 18:47:14, oshima wrote:
> this shuld be extensions/browser

Done. Of course I need to add back the native_app_window dep for chrome/browser
(see below)

https://codereview.chromium.org/606953002/diff/160001/chrome/browser/ui/BUILD.gn
File chrome/browser/ui/BUILD.gn (right):

https://codereview.chromium.org/606953002/diff/160001/chrome/browser/ui/BUILD...
chrome/browser/ui/BUILD.gn:208: deps += [
"//extensions/components/native_app_window" ]
.. pretty sure this is the where the native_app_window dependency belongs for
Chrome/browser. (It is views code, so belongs next to the c/b/ui/views stuff).
This also ensures it's not pulled in on MacViews until it's ready.

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

deleting code from //components lgtm

[scheib, 2014-09-30 21:50:53]

extensions/DEPS can disallow use of components.

extensions/components/README to explain the components concept. (e.g. see
src/components/README).

extensions/components/native_app_window/README to explain that component.

[James Cook, 2014-09-30 22:27:32]

native_app_window* lgtm

https://codereview.chromium.org/606953002/diff/160001/chrome/chrome_browser_u...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/606953002/diff/160001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:2741: 'dependencies': [
'<(DEPTH)/extensions/components/extensions_components.gyp:native_app_window' ],
nit: I would wrap this, it's kind of long

[Ken Rockot(use gerrit already), 2014-09-30 22:53:52]

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

[Ken Rockot(use gerrit already), 2014-09-30 22:53:53]

Somewhat impromptu drive-by commentary here, so take with a grain of salt and I
hope I'm not bikeshedding too much.

The introduction of //extensions/components makes me uncomfortable, especially
when it's just to avoid a shortcoming in GYP or the fact that all component
rules are rooted in a single GYP file (why is this, anyway?)

I think this could safely live in a new //extensions/ui/views with appropriate
DEPS boundaries in place, but I'd much rather see a larger //components
component that provides a views-based implementation of extensions UI concepts
as needed, rather than slicing each UI feature implementation into its own
separate //extensions/component.

If you really must do this, at least call the component native_app_window_views,
because currently the name confuses the views-specific implementation with the
generic extensions concept of a native app window.

OR, you could move the entire native_app_window abstraction into //components,
stripped of its //extensions dependencies, and provide a hierarchy of
implementations there (e.g. //components/native_app_window/{views,whatever}).
Then extensions can depend on this component and embedders who want to use the
views implementation can depend additionally on the views implementation of the
component.

[oshima, 2014-09-30 23:28:30]

On 2014/09/30 22:53:53, Ken Rockot wrote:
> Somewhat impromptu drive-by commentary here, so take with a grain of salt and
I
> hope I'm not bikeshedding too much.
> 
> The introduction of //extensions/components makes me uncomfortable, especially
> when it's just to avoid a shortcoming in GYP or the fact that all component
> rules are rooted in a single GYP file (why is this, anyway?)

I agree your point here (that having all targets in single gyp),
although //components owners wanted to avoid //extensions dependency there.

> 
> I think this could safely live in a new //extensions/ui/views with appropriate
> DEPS boundaries in place, but I'd much rather see a larger //components
> component that provides a views-based implementation of extensions UI concepts
> as needed, rather than slicing each UI feature implementation into its own
> separate //extensions/component.

A component can also depends on contents, aura and other parts as well as views,
so IMHO, categorizing based on what it uses doesn't really work well.

I'd also expect there will be more components that depends on //extensions.
(there is renderer_context_menu which I added ;-). With these assumption,
it sounds to me that //extensions/components is better option.

> 
> If you really must do this, at least call the component
native_app_window_views,
> because currently the name confuses the views-specific implementation with the
> generic extensions concept of a native app window.

I named native_app_window in original CL so that if we ever had mac, we can add
subdirectories views/cocoa rather than having separate native_app_window_cocoa.
I don't have strong opinion though.

> 
> OR, you could move the entire native_app_window abstraction into //components,
> stripped of its //extensions dependencies, and provide a hierarchy of
> implementations there (e.g. //components/native_app_window/{views,whatever}).
> Then extensions can depend on this component and embedders who want to use the
> views implementation can depend additionally on the views implementation of
the
> component.

sounds like a good alternative (not sure how easy/hard to do so).

[tapted, 2014-10-01 00:01:33]

On 2014/09/30 21:50:53, scheib wrote:
> extensions/DEPS can disallow use of components.

Done.

> extensions/components/README to explain the components concept. (e.g. see
> src/components/README).

Done.

> extensions/components/native_app_window/README to explain that component.

Done.

On 2014/09/30 22:53:53, Ken Rockot wrote:
> The introduction of //extensions/components makes me uncomfortable, especially
> when it's just to avoid a shortcoming in GYP or the fact that all component
> rules are rooted in a single GYP file (why is this, anyway?)

Can you expand a bit on why it makes you uncomfortable? I admit.. I thought it
felt a bit strange too, and it took a while to grow on me, but I think
extensions/components is a sound idea.  E.g. (not suggesting we need this, but
hypothetically), we could have particular APIs become extensions/components; if
you make an embedder that can't support an API (or just doesn't want to), it's
easy to drop that component from your dependencies.

For the gyp-file cycle thing. I'm focusing on that because it gives a concrete
error I can point at. But really, there is the fuzzier layering problem.
src/extensions and src/components can't currrently sit in layers since they have
a cycle between them. Adding src/extensions/components sets up layering as:

src/components
src/extensions
src/extensions/components

On 2014/09/30 23:28:30, oshima wrote:
> On 2014/09/30 22:53:53, Ken Rockot wrote:
> > OR, you could move the entire native_app_window abstraction into
//components,
> > stripped of its //extensions dependencies, and provide a hierarchy of
> > implementations there (e.g.
//components/native_app_window/{views,whatever}).
> > Then extensions can depend on this component and embedders who want to use
the
> > views implementation can depend additionally on the views implementation of
> the
> > component.
> 
> sounds like a good alternative (not sure how easy/hard to do so).

I think this is worth trying - there should be something for you all to look at
when you wake up to see how it looks :)

At a cursory look, I'd need to
 - move extensions::NativeAppWindow into components/native_app_window [feels
"nice"]
 - move other dependencies (SizeConstraints, DraggableRegion)
 - add a NativeAppWindowDelegate, which extensions::AppWindow inherits from

only that last part looks a bit curly, but there's precedent for this under
ui/views/widget already.

https://codereview.chromium.org/606953002/diff/160001/chrome/chrome_browser_u...
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/606953002/diff/160001/chrome/chrome_browser_u...
chrome/chrome_browser_ui.gypi:2741: 'dependencies': [
'<(DEPTH)/extensions/components/extensions_components.gyp:native_app_window' ],
On 2014/09/30 22:27:32, James Cook wrote:
> nit: I would wrap this, it's kind of long

Done.

[tapted, 2014-10-01 08:47:58]

On 2014/10/01 00:01:33, tapted wrote:
> On 2014/09/30 23:28:30, oshima wrote:
> > On 2014/09/30 22:53:53, Ken Rockot wrote:
> > > OR, you could move the entire native_app_window abstraction into
> //components,
> > > stripped of its //extensions dependencies, and provide a hierarchy of
> > > implementations there (e.g.
> //components/native_app_window/{views,whatever}).
> > > Then extensions can depend on this component and embedders who want to use
> the
> > > views implementation can depend additionally on the views implementation
of
> > the
> > > component.
> > 
> > sounds like a good alternative (not sure how easy/hard to do so).
> 
> I think this is worth trying - there should be something for you all to look
at
> when you wake up to see how it looks :)
> 
> At a cursory look, I'd need to
>  - move extensions::NativeAppWindow into components/native_app_window [feels
> "nice"]
>  - move other dependencies (SizeConstraints, DraggableRegion)
>  - add a NativeAppWindowDelegate, which extensions::AppWindow inherits from
> 
> only that last part looks a bit curly, but there's precedent for this under
> ui/views/widget already.

I've explored this in http://crrev.com/616253002/ . I haven't published it out
yet -- there's a few things still to fix -- but linux has been spotted green.
And perhaps chromeos also.

It works. It fixes DEPS for native_app_window, and the decoupling of
native_app_window and extensions seems "nice". NativeAppWindowDelegate is not
even that big. But I don't know if we want this.

Please take a look at http://crrev.com/616253002/ and see how it feels - there
are forwarding declarations to keep the diff bloat down so you can concentrate
on the interesting stuff (but DEPS rules are all updated already assuming no
forwarding). If it's preferable to this CL, let me know. I'll keep going there
and close this one. Otherwise, I think this one is as ready as it can get.

However note http://crrev.com/616253002/ is really just solving
native_app_window. The same technique might not apply to renderer_context_menu
or anything else that needs to be componentized in this way.

[Ken Rockot(use gerrit already), 2014-10-01 18:22:33]

First, thank you for trying that suggestion. I really like that
native_app_window
can indeed be componentized (http://crrev.com/616253002/) without any
dependencies
on extensions. Its usage within extensions is IMO not much better or worse
looking
with that approach than any other, but it seems more in line with the spirit of
componentization. [side note: I know that CL is incomplete, but I would expect
the 
Cocoa impl to move as well in that case, yes?]

Just to elaborate a bit on my position here after some more thought: I am
solidly in favor of decomposing //extensions even further, but I don't think it
should be done by adding another components subtree which implies dependency on
the entire extensions module. Rather I think we should continue to chip away
pieces that could be reusable (even if not immediately needed) outside of
extensions. native_app_window seems like a perfectly good example of such a
case,
as does renderer_context_menu.

[rambling_digression]
I could even see API implementations going this way as they're ported to mojo.
Maybe
we build a generic extension_api component which defines a common service
interface for components to implement, and then all extension API
implementations
move to their own component with only a lightweight dependency on extension_api.
Bindings set up by //extensions code could make use of this service interface,
but it would become possible for other code to consume extension APIs
without any dependency on //extensions concepts (which would be relegated to
managing things like process lifetime and feature access control within various
contexts).
[/rambling_digression]

Anyway, I may be biased, but I do prefer the approach which fully componentizes
NativeAppWindow.

[oshima, 2014-10-01 18:47:03]

On 2014/10/01 18:22:33, Ken Rockot wrote:
> First, thank you for trying that suggestion. I really like that
> native_app_window
> can indeed be componentized (http://crrev.com/616253002/) without any
> dependencies
> on extensions. Its usage within extensions is IMO not much better or worse
> looking
> with that approach than any other, but it seems more in line with the spirit
of
> componentization. [side note: I know that CL is incomplete, but I would expect
> the 
> Cocoa impl to move as well in that case, yes?]
> 
> Just to elaborate a bit on my position here after some more thought: I am
> solidly in favor of decomposing //extensions even further, but I don't think
it
> should be done by adding another components subtree which implies dependency
on
> the entire extensions module. Rather I think we should continue to chip away
> pieces that could be reusable (even if not immediately needed) outside of
> extensions. native_app_window seems like a perfectly good example of such a
> case,
> as does renderer_context_menu.
> 

Are you suggesting components should not depends on //extensions or you just
don't want to
have a sub directory under //extensions? I don't mind to have
//components/extensions
or something else for components that depends on //extensions. 

> [rambling_digression]
> I could even see API implementations going this way as they're ported to mojo.
> Maybe
> we build a generic extension_api component which defines a common service
> interface for components to implement, and then all extension API
> implementations
> move to their own component with only a lightweight dependency on
extension_api.
> Bindings set up by //extensions code could make use of this service interface,
> but it would become possible for other code to consume extension APIs
> without any dependency on //extensions concepts (which would be relegated to
> managing things like process lifetime and feature access control within
various
> contexts).
> [/rambling_digression]
> 
> Anyway, I may be biased, but I do prefer the approach which fully
componentizes
> NativeAppWindow.

My understanding of the goal of componentization is to remove unnecessary
dependency so
that embedder can use them in their context. Since NativeAppWindow does not make
sense
without extension, I feel it's ok to have extension dependency there. (chrome
w/o extensions, such as ios/android will never use them)

Extension system is one of key component in chromium, so it seems to be
perfectly
fine to have components that depends //extensions.

Just my 2c.

[tapted, 2014-10-01 23:35:00]

I think I'm inclined to go with this CL (src/extensions/components). See
rationale below. scheib: WDYT?

On 2014/10/01 18:47:03, oshima wrote:
> On 2014/10/01 18:22:33, Ken Rockot wrote:
> > First, thank you for trying that suggestion. I really like that
> > native_app_window
> > can indeed be componentized (http://crrev.com/616253002/) without any
> > dependencies
> > on extensions. Its usage within extensions is IMO not much better or worse
> > looking
> > with that approach than any other, but it seems more in line with the spirit
> of
> > componentization. [side note: I know that CL is incomplete, but I would
expect
> > the 
> > Cocoa impl to move as well in that case, yes?]

There's currently no demand for anything between the Cocoa
"Chrome"NativeAppWindow implementation and extensions::NativeAppWindow, so no
way to move the cocoa window without first untangling it from Chrome. Hopefully
I can get this MacViews thing going and we will never need it :) (or, more
likely, it will be a thin OS-integration layer like
ChromeNativeAppWindowViewsWin)

> > Just to elaborate a bit on my position here after some more thought: I am
> > solidly in favor of decomposing //extensions even further, but I don't think
> it
> > should be done by adding another components subtree which implies dependency
> on
> > the entire extensions module. Rather I think we should continue to chip away
> > pieces that could be reusable (even if not immediately needed) outside of
> > extensions. native_app_window seems like a perfectly good example of such a
> > case,
> > as does renderer_context_menu.

After some time to reflect, I'm worried it may become unsustainable. Or would
end up adding complexity for no tangible gain. Example:
ChromeNativeAppWindowViews still needs access to the Extension*, and it
currently gets that through extensions::NativeAppWindow. To allow that to work,
I added a constructor argument to ChromeNativeAppWindowViews so that it could
have access to a strongly typed 'NativeAppWindowDelegate'. But now there are two
copies of that delegate in data members.

I think what I liked about http://crrev.com/616253002/ was the clearer
separation from (the pretty big and complex) AppWindow class and the things that
the {Chrome,etc.}NativeAppWindow* classes need (i.e. NativeAppWindowDelegate).
But I can't point to a good reason why this can't be done in src/extensions
(e.g. similar to ui/views/widget). Then it could full encapsulate the
dependencies (which currently include an Extension*).


> Are you suggesting components should not depends on //extensions or you just
> don't want to
> have a sub directory under //extensions? I don't mind to have
> //components/extensions
> or something else for components that depends on //extensions. 
> 
> > [rambling_digression]
> > I could even see API implementations going this way as they're ported to
mojo.
> > Maybe
> > we build a generic extension_api component which defines a common service
> > interface for components to implement, and then all extension API
> > implementations
> > move to their own component with only a lightweight dependency on
> extension_api.
> > Bindings set up by //extensions code could make use of this service
interface,
> > but it would become possible for other code to consume extension APIs
> > without any dependency on //extensions concepts (which would be relegated to
> > managing things like process lifetime and feature access control within
> various
> > contexts).
> > [/rambling_digression]
> > 
> > Anyway, I may be biased, but I do prefer the approach which fully
> componentizes
> > NativeAppWindow.
> 
> My understanding of the goal of componentization is to remove unnecessary
> dependency so
> that embedder can use them in their context. Since NativeAppWindow does not
make
> sense
> without extension, I feel it's ok to have extension dependency there. (chrome
> w/o extensions, such as ios/android will never use them)

^ this :). It currently feels a bit contrived to remove the Extension*
dependency, since the Chrome* versions of NativeAppWindow still want it, and
nothing currently needs a version without an Extension* dependency.

> 
> Extension system is one of key component in chromium, so it seems to be
> perfectly
> fine to have components that depends //extensions.
> 
> Just my 2c.

[James Cook, 2014-10-01 23:48:40]

On 2014/10/01 23:35:00, tapted wrote:
> I think I'm inclined to go with this CL (src/extensions/components). See
> rationale below. scheib: WDYT?
> 
> On 2014/10/01 18:47:03, oshima wrote:
> > On 2014/10/01 18:22:33, Ken Rockot wrote:
> > > First, thank you for trying that suggestion. I really like that
> > > native_app_window
> > > can indeed be componentized (http://crrev.com/616253002/) without any
> > > dependencies
> > > on extensions. Its usage within extensions is IMO not much better or worse
> > > looking
> > > with that approach than any other, but it seems more in line with the
spirit
> > of
> > > componentization. [side note: I know that CL is incomplete, but I would
> expect
> > > the 
> > > Cocoa impl to move as well in that case, yes?]
> 
> There's currently no demand for anything between the Cocoa
> "Chrome"NativeAppWindow implementation and extensions::NativeAppWindow, so no
> way to move the cocoa window without first untangling it from Chrome.
Hopefully
> I can get this MacViews thing going and we will never need it :) (or, more
> likely, it will be a thin OS-integration layer like
> ChromeNativeAppWindowViewsWin)
> 
> > > Just to elaborate a bit on my position here after some more thought: I am
> > > solidly in favor of decomposing //extensions even further, but I don't
think
> > it
> > > should be done by adding another components subtree which implies
dependency
> > on
> > > the entire extensions module. Rather I think we should continue to chip
away
> > > pieces that could be reusable (even if not immediately needed) outside of
> > > extensions. native_app_window seems like a perfectly good example of such
a
> > > case,
> > > as does renderer_context_menu.
> 
> After some time to reflect, I'm worried it may become unsustainable. Or would
> end up adding complexity for no tangible gain. Example:
> ChromeNativeAppWindowViews still needs access to the Extension*, and it
> currently gets that through extensions::NativeAppWindow. To allow that to
work,
> I added a constructor argument to ChromeNativeAppWindowViews so that it could
> have access to a strongly typed 'NativeAppWindowDelegate'. But now there are
two
> copies of that delegate in data members.
> 
> I think what I liked about http://crrev.com/616253002/ was the clearer
> separation from (the pretty big and complex) AppWindow class and the things
that
> the {Chrome,etc.}NativeAppWindow* classes need (i.e. NativeAppWindowDelegate).
> But I can't point to a good reason why this can't be done in src/extensions
> (e.g. similar to ui/views/widget). Then it could full encapsulate the
> dependencies (which currently include an Extension*).
> 
> 
> > Are you suggesting components should not depends on //extensions or you just
> > don't want to
> > have a sub directory under //extensions? I don't mind to have
> > //components/extensions
> > or something else for components that depends on //extensions. 
> > 
> > > [rambling_digression]
> > > I could even see API implementations going this way as they're ported to
> mojo.
> > > Maybe
> > > we build a generic extension_api component which defines a common service
> > > interface for components to implement, and then all extension API
> > > implementations
> > > move to their own component with only a lightweight dependency on
> > extension_api.
> > > Bindings set up by //extensions code could make use of this service
> interface,
> > > but it would become possible for other code to consume extension APIs
> > > without any dependency on //extensions concepts (which would be relegated
to
> > > managing things like process lifetime and feature access control within
> > various
> > > contexts).
> > > [/rambling_digression]
> > > 
> > > Anyway, I may be biased, but I do prefer the approach which fully
> > componentizes
> > > NativeAppWindow.
> > 
> > My understanding of the goal of componentization is to remove unnecessary
> > dependency so
> > that embedder can use them in their context. Since NativeAppWindow does not
> make
> > sense
> > without extension, I feel it's ok to have extension dependency there.
(chrome
> > w/o extensions, such as ios/android will never use them)
> 
> ^ this :). It currently feels a bit contrived to remove the Extension*
> dependency, since the Chrome* versions of NativeAppWindow still want it, and
> nothing currently needs a version without an Extension* dependency.
> 
> > 
> > Extension system is one of key component in chromium, so it seems to be
> > perfectly
> > fine to have components that depends //extensions.
> > 
> > Just my 2c.

I am not scheib, but FWIW I'm OK with this approach. I agree it feels artificial
to remove the Extension* dependency from NativeAppWindow, since it will never be
used without AppWindow.

[scheib, 2014-10-01 23:51:06]

Ken and James have done most of the refactoring, so their opinions
out-weigh mine. I'm indifferent for this to be componentized in
src/components or src/extensions/components. Either makes sense if there is
a dependency on extensions. However, I do like the direction
https://codereview.chromium.org/616253002/ is going to cleanly seperate out
that dependency into an indirect delegate
https://codereview.chromium.org/616253002/diff/100001/components/native_app_w...
which... but I don't like it much, because it adds even more Chrome
layering and indirection, and if it's not warranted by having use cases of
multiple implementations or testing modularity I prefer not to have the
code complexity.

Summary: extensions/components seems simplest and easiest to understand to
me.

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

[Ken Rockot(use gerrit already), 2014-10-01 23:54:45]

I concede that my concerns aren't strong enough to warrant blocking this
work. I can live with extensions/components for now. :)

On Wed, Oct 1, 2014 at 4:50 PM, Vincent Scheib <scheib@chromium.org> wrote:

> Ken and James have done most of the refactoring, so their opinions
> out-weigh mine. I'm indifferent for this to be componentized in
> src/components or src/extensions/components. Either makes sense if there is
> a dependency on extensions. However, I do like the direction
> https://codereview.chromium.org/616253002/ is going to cleanly seperate
> out that dependency into an indirect delegate
>
https://codereview.chromium.org/616253002/diff/100001/components/native_app_w...
> which... but I don't like it much, because it adds even more Chrome
> layering and indirection, and if it's not warranted by having use cases of
> multiple implementations or testing modularity I prefer not to have the
> code complexity.
>
> Summary: extensions/components seems simplest and easiest to understand to
> me.
>
>
>

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

[scheib, 2014-10-02 15:25:45]

extensions/* lgtm

[scheib, 2014-10-02 15:27:45]

Remaining l g t m  approval needed:
 - oshima: src/athena

[oshima, 2014-10-02 16:58:16]

On 2014/10/02 15:27:45, scheib wrote:
> Remaining l g t m  approval needed:
>  - oshima: src/athena

athena lgtm

[tapted, 2014-10-02 23:40:01]

tapted@chromium.org changed reviewers:
+ reed@google.com

[tapted, 2014-10-02 23:40:02]

Thanks all!

+ reed@google.com TBR for

Missing LGTM from OWNERS of dependencies added to DEPS:
    '+third_party/skia/include/core/SkRegion.h',

(this is just moving an existing DEPS to a new folder)

[tapted, 2014-10-02 23:40:20]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2014-10-02 23:41:19]

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

[commit-bot: I haz the power, 2014-10-02 23:46:01]

Committed patchset #7 (id:180001) as 98f56f380d9218c2229d8d231f59f807f47b5d3a

[commit-bot: I haz the power, 2014-10-02 23:46:43]

Patchset 7 (id:??) landed as
https://crrev.com/344272ddd4ca47fb56fd6a1ca6a4f1418df2c426
Cr-Commit-Position: refs/heads/master@{#297952}