V2 app support step2 : Use NativeAppWindow for Activity's window.

V2 app support step2 : Use NativeAppWindow for Activity's window.
 This CL removes redundant widget created by Activity, and use NativeAppWindow instead.

Activity APIs needs to some cleanup and I'll look into it once hashimoto-san finished app_window transition.

step3 will replace ChromeAppNativeWindowViews with athena's impl.

BUG=410448
Committed: https://crrev.com/be9f162893a0ed54ee4c8f5fac3b8e2a113bea02
Cr-Commit-Position: refs/heads/master@{#294297}

[oshima, 2014-09-10 20:51:11]

Patchset #2 (id:20001) has been deleted

[oshima, 2014-09-10 20:51:16]

Patchset #1 (id:1) has been deleted

[oshima, 2014-09-10 21:04:49]

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

[oshima, 2014-09-10 21:14:07]

Patchset #1 (id:40001) has been deleted

[oshima, 2014-09-10 21:14:11]

Patchset #1 (id:60001) has been deleted

[oshima, 2014-09-10 21:19:25]

Patchset #1 (id:80001) has been deleted

[oshima, 2014-09-10 21:19:40]

Patchset #1 (id:100001) has been deleted

[oshima, 2014-09-10 21:20:54]

https://codereview.chromium.org/558243002/diff/120001/athena/athena.gyp
File athena/athena.gyp (right):

https://codereview.chromium.org/558243002/diff/120001/athena/athena.gyp#newco...
athena/athena.gyp:190: '../skia/skia.gyp:skia',
this is for webview include (which brings skia header)

[Jun Mukai, 2014-09-10 23:35:38]

I am wondering how these V2 apps will look like in the overview mode.

Because this CL provides its own widget for v2 apps, that means those widget
doesn't have Athena's non-client frame views, right?

Then, it won't change the title bar / favicon in the overview mode, which might
be slightly different from our expectation. Better to chat with kuscher about
this.

https://codereview.chromium.org/558243002/diff/120001/athena/activity/activit...
File athena/activity/activity_view_manager_impl.cc (left):

https://codereview.chromium.org/558243002/diff/120001/athena/activity/activit...
athena/activity/activity_view_manager_impl.cc:30: params.activatable =
views::Widget::InitParams::ACTIVATABLE_YES;
why this removed?  is it a default value?

https://codereview.chromium.org/558243002/diff/120001/athena/content/chrome/c...
File athena/content/chrome/content_activity_factory.cc (right):

https://codereview.chromium.org/558243002/diff/120001/athena/content/chrome/c...
athena/content/chrome/content_activity_factory.cc:22:
Observe(app_window_->web_contents());
It is a bit unclear to me that app_window_->web_contents() ==
web_view->web_contents().  Is that true?
Might be good to add a DCHECK here.

[Jun Mukai, 2014-09-10 23:53:14]

https://codereview.chromium.org/558243002/diff/120001/athena/extensions/chrom...
File athena/extensions/chrome/athena_apps_client.cc (right):

https://codereview.chromium.org/558243002/diff/120001/athena/extensions/chrom...
athena/extensions/chrome/athena_apps_client.cc:24: class
AthenaNativeAppWindowViews : public ChromeNativeAppWindowViews {
For the follow-ups of outlined comment:
Noticed that ChromeNativeAppWindowViews is a WidgetDelegate subclass. Therefore,
you may want to override CreateNonClientFrameView here, and use the Athena
activity's frame view.

[Jun Mukai, 2014-09-10 23:58:11]

https://codereview.chromium.org/558243002/diff/120001/athena/extensions/chrom...
File athena/extensions/chrome/athena_apps_client.cc (right):

https://codereview.chromium.org/558243002/diff/120001/athena/extensions/chrom...
athena/extensions/chrome/athena_apps_client.cc:29: views::WebView* GetWebView()
{
Do you need to expose this?
You may want to pass apps::NativeAppWindowViews* to AppActivity, then
AppActivity will have the access to the widget(), instead of calling
web_view_->GetWidget() ?

[oshima, 2014-09-11 00:12:34]

https://codereview.chromium.org/558243002/diff/120001/athena/activity/activit...
File athena/activity/activity_view_manager_impl.cc (left):

https://codereview.chromium.org/558243002/diff/120001/athena/activity/activit...
athena/activity/activity_view_manager_impl.cc:30: params.activatable =
views::Widget::InitParams::ACTIVATABLE_YES;
On 2014/09/10 23:35:38, Jun Mukai wrote:
> why this removed?  is it a default value?

This was necessary when window type was TYPE_WINDOW_FRAMELESS. TYPE_WINDOW is
activatable.

https://codereview.chromium.org/558243002/diff/120001/athena/content/chrome/c...
File athena/content/chrome/content_activity_factory.cc (right):

https://codereview.chromium.org/558243002/diff/120001/athena/content/chrome/c...
athena/content/chrome/content_activity_factory.cc:22:
Observe(app_window_->web_contents());
On 2014/09/10 23:35:38, Jun Mukai wrote:
> It is a bit unclear to me that app_window_->web_contents() ==
> web_view->web_contents().  Is that true?
> Might be good to add a DCHECK here.

Good point. Done.

[oshima, 2014-09-11 00:19:59]

On 2014/09/10 23:35:38, Jun Mukai wrote:
> I am wondering how these V2 apps will look like in the overview mode.
> 
> Because this CL provides its own widget for v2 apps, that means those widget
> doesn't have Athena's non-client frame views, right?
> 
> Then, it won't change the title bar / favicon in the overview mode, which
might
> be slightly different from our expectation. Better to chat with kuscher about
> this.

ChromeAppNativeView will be replaced. See the issue description.

> 
>
https://codereview.chromium.org/558243002/diff/120001/athena/activity/activit...
> File athena/activity/activity_view_manager_impl.cc (left):
> 
>
https://codereview.chromium.org/558243002/diff/120001/athena/activity/activit...
> athena/activity/activity_view_manager_impl.cc:30: params.activatable =
> views::Widget::InitParams::ACTIVATABLE_YES;
> why this removed?  is it a default value?
> 
>
https://codereview.chromium.org/558243002/diff/120001/athena/content/chrome/c...
> File athena/content/chrome/content_activity_factory.cc (right):
> 
>
https://codereview.chromium.org/558243002/diff/120001/athena/content/chrome/c...
> athena/content/chrome/content_activity_factory.cc:22:
> Observe(app_window_->web_contents());
> It is a bit unclear to me that app_window_->web_contents() ==
> web_view->web_contents().  Is that true?
> Might be good to add a DCHECK here.

[oshima, 2014-09-11 00:20:56]

https://codereview.chromium.org/558243002/diff/120001/athena/extensions/chrom...
File athena/extensions/chrome/athena_apps_client.cc (right):

https://codereview.chromium.org/558243002/diff/120001/athena/extensions/chrom...
athena/extensions/chrome/athena_apps_client.cc:24: class
AthenaNativeAppWindowViews : public ChromeNativeAppWindowViews {
On 2014/09/10 23:53:14, Jun Mukai wrote:
> For the follow-ups of outlined comment:
> Noticed that ChromeNativeAppWindowViews is a WidgetDelegate subclass.
Therefore,
> you may want to override CreateNonClientFrameView here, and use the Athena
> activity's frame view.

ActivityFrame view is in athena/activity, and require some refactoring just to
use it. Since this has to be changed anyway,
I prefer to wait until we can refactor. (it's step3 in the description)

https://codereview.chromium.org/558243002/diff/120001/athena/extensions/chrom...
athena/extensions/chrome/athena_apps_client.cc:29: views::WebView* GetWebView()
{
On 2014/09/10 23:58:11, Jun Mukai wrote:
> Do you need to expose this?
> You may want to pass apps::NativeAppWindowViews* to AppActivity, then
> AppActivity will have the access to the widget(), instead of calling
> web_view_->GetWidget() ?

I tried that first, but it added extra dependency to NAWV.
What AppActivity needs is WebView, not NAWV.

I also want to change APIs a bit once hashimoto-san's appwindow
transition is completed.

[Jun Mukai, 2014-09-11 00:24:15]

Okay, then it lgtm as a step 2.  Waiting for step 3!
# by the way... how many steps are there? :)

[oshima, 2014-09-11 00:28:28]

On 2014/09/11 00:24:15, Jun Mukai wrote:
> Okay, then it lgtm as a step 2.  Waiting for step 3!
> # by the way... how many steps are there? :)

Step 3 will be the last one for this bug. There will be bugs or improvements
for sure and they'll be tracked separately.

[oshima, 2014-09-11 00:28:38]

The CQ bit was checked by oshima@chromium.org

[oshima, 2014-09-11 00:29:00]

The CQ bit was unchecked by oshima@chromium.org

[oshima, 2014-09-11 00:31:16]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-09-11 00:37:33]

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

[commit-bot: I haz the power, 2014-09-11 02:24:40]

Committed patchset #2 (id:140001) as 9f4bf07da7ab86013b1a75ec4a905ebde0346723

[commit-bot: I haz the power, 2014-09-11 03:09:17]

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