Componentize NativeAppWindowViews

Move native_app_window_views* to components/native_app_window and fixed up build files.

 This will be used in athena.

BUG=414968
TBR=ben@chromium.org

Committed: https://crrev.com/c6ff18dc322faf3938e9ca18ca1ba890a2060451
Cr-Commit-Position: refs/heads/master@{#295502}

[oshima, 2014-09-17 22:16:22]

Patchset #1 (id:1) has been deleted

[oshima, 2014-09-17 22:16:29]

Patchset #1 (id:20001) has been deleted

[oshima, 2014-09-17 22:18:05]

oshima@chromium.org changed reviewers:
+ jamescook@chromium.org, yoz@chromium.org

[oshima, 2014-09-17 22:18:33]

yoz, can you review this (ben is OOO until Oct 7)?

[James Cook, 2014-09-17 22:30:28]

I think this should be in namespace extensions instead of namespace apps, since
the NativeAppWindow base class is in namespace extensions.

[James Cook, 2014-09-17 22:36:45]

LGTM

[James Cook, 2014-09-17 22:37:35]

(after resolving the namespace, I mean :-)

[oshima, 2014-09-17 22:41:02]

On 2014/09/17 22:30:28, James Cook wrote:
> I think this should be in namespace extensions instead of namespace apps,
since
> the NativeAppWindow base class is in namespace extensions.

Done

[oshima, 2014-09-17 22:42:52]

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

[oshima, 2014-09-17 22:42:53]

switching owner's review from yoz@ to tapted@ as you're the owner of
c/b/ui/views/apps. Could you please review this?

[James Cook, 2014-09-17 22:48:24]

Also, you might note in the commit description that the motivation for this
change is to use NativeAppWindowViews in Athena.

https://codereview.chromium.org/576863003/diff/60001/components/native_app_wi...
File components/native_app_window/native_app_window_views.h (right):

https://codereview.chromium.org/576863003/diff/60001/components/native_app_wi...
components/native_app_window/native_app_window_views.h:40: namespace apps {
Were you going to change this to namespace extensions?

[oshima, 2014-09-17 22:51:30]

On 2014/09/17 22:48:24, James Cook wrote:
> Also, you might note in the commit description that the motivation for this
> change is to use NativeAppWindowViews in Athena.
> 
>
https://codereview.chromium.org/576863003/diff/60001/components/native_app_wi...
> File components/native_app_window/native_app_window_views.h (right):
> 
>
https://codereview.chromium.org/576863003/diff/60001/components/native_app_wi...
> components/native_app_window/native_app_window_views.h:40: namespace apps {
> Were you going to change this to namespace extensions?

Uploaded. (patch set 3)

[James Cook, 2014-09-17 23:41:49]

still LGTM

[tapted, 2014-09-18 00:29:33]

The apps stuff lgtm after some dependencies are added.

(also nit/typo in CL description/subject: Natieve -> Native)

I'd also consider a namespace like `native_app_window` instead of apps or
extensions - this seems to be the more common pattern amongst components, but a
components OWNER might know best (there's precedent for not following this
pattern for example).

https://codereview.chromium.org/576863003/diff/80001/components/native_app_wi...
File components/native_app_window.gypi (right):

https://codereview.chromium.org/576863003/diff/80001/components/native_app_wi...
components/native_app_window.gypi:12: '../ui/views/views.gyp:views',
I would add

        '../content/content.gyp:content_browser',
        '../extensions/extensions.gyp:extensions_browser',
        '../extensions/extensions.gyp:extensions_common',

They're the major ones it would be good to document.

I think if we could make this a real component instead of a static_library, it
would also need gfx.gyp:gfx and (conditionally) aura.gyp:aura, and probably some
other things. (pretty sure a proper component is still the goal, but it's
blocked on getting extensions a component first)

In the meantime you can run something like

gn gen out/gn_Debug && gn check out/gn_Debug //components/native_app_window

and it will list direct header dependencies that are missing. (and I've seen
patches going by to make the gn build `gn check` clean, so this will probably be
needed anyway unless you add `check_includes = false` to a BUILD.gn).

[oshima, 2014-09-18 07:13:50]

On 2014/09/18 00:29:33, tapted wrote:
> The apps stuff lgtm after some dependencies are added.
> 
> (also nit/typo in CL description/subject: Natieve -> Native)
> 
> I'd also consider a namespace like `native_app_window` instead of apps or
> extensions - this seems to be the more common pattern amongst components, but
a
> components OWNER might know best (there's precedent for not following this
> pattern for example).

Done

> 
>
https://codereview.chromium.org/576863003/diff/80001/components/native_app_wi...
> File components/native_app_window.gypi (right):
> 
>
https://codereview.chromium.org/576863003/diff/80001/components/native_app_wi...
> components/native_app_window.gypi:12: '../ui/views/views.gyp:views',
> I would add
> 
>         '../content/content.gyp:content_browser',
>         '../extensions/extensions.gyp:extensions_browser',
>         '../extensions/extensions.gyp:extensions_common',
> 
> They're the major ones it would be good to document.

Done. (they aren't in apps/apps.gypi, just fyi)

> 
> I think if we could make this a real component instead of a static_library, it
> would also need gfx.gyp:gfx and (conditionally) aura.gyp:aura, and probably
some
> other things. (pretty sure a proper component is still the goal, but it's
> blocked on getting extensions a component first)

My understanding is that components/ and shared_library are different things.
It's 'nice to have', but not required, and most of components are in fact
static_library.
And yes, extensions must be shared_library first.

> In the meantime you can run something like
> 
> gn gen out/gn_Debug && gn check out/gn_Debug //components/native_app_window
> 
> and it will list direct header dependencies that are missing. (and I've seen
> patches going by to make the gn build `gn check` clean, so this will probably
be
> needed anyway unless you add `check_includes = false` to a BUILD.gn).

[oshima, 2014-09-18 07:18:58]

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

[oshima, 2014-09-18 07:18:59]

tbr'ing ben for following dependency in  components/native_app_windows/DEPS

ui/ base/ and content/public/browser

new DPES is subset of apps/DEPS, which already has these dependencies.

[oshima, 2014-09-18 07:19:08]

The CQ bit was checked by oshima@chromium.org

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

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/576863003/80001

[commit-bot: I haz the power, 2014-09-18 07:45:08]

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

[commit-bot: I haz the power, 2014-09-18 07:45:09]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[James Cook, 2014-09-18 15:32:46]

https://codereview.chromium.org/576863003/diff/100001/components/native_app_w...
File components/native_app_window/native_app_window_views.cc (right):

https://codereview.chromium.org/576863003/diff/100001/components/native_app_w...
components/native_app_window/native_app_window_views.cc:443: }  // namespace
extensions
nit: namespace native_app_window

https://codereview.chromium.org/576863003/diff/100001/components/native_app_w...
File components/native_app_window/native_app_window_views.h (right):

https://codereview.chromium.org/576863003/diff/100001/components/native_app_w...
components/native_app_window/native_app_window_views.h:194: }  // namespace
extensions
nit: namespace native_app_window

[oshima, 2014-09-18 17:28:54]

https://codereview.chromium.org/576863003/diff/100001/components/native_app_w...
File components/native_app_window/native_app_window_views.cc (right):

https://codereview.chromium.org/576863003/diff/100001/components/native_app_w...
components/native_app_window/native_app_window_views.cc:443: }  // namespace
extensions
On 2014/09/18 15:32:46, James Cook wrote:
> nit: namespace native_app_window

Done.

https://codereview.chromium.org/576863003/diff/100001/components/native_app_w...
File components/native_app_window/native_app_window_views.h (right):

https://codereview.chromium.org/576863003/diff/100001/components/native_app_w...
components/native_app_window/native_app_window_views.h:194: }  // namespace
extensions
On 2014/09/18 15:32:46, James Cook wrote:
> nit: namespace native_app_window

Done.

[oshima, 2014-09-18 17:29:00]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-09-18 17:29:41]

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

[commit-bot: I haz the power, 2014-09-18 18:26:58]

Committed patchset #5 (id:120001) as 53302a230e452c6366fadfd44905bb21990f36fa

[commit-bot: I haz the power, 2014-09-18 18:27:34]

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