Do not show the activity widget in ActivityManager::AddActivity

Do not show the activity widget in ActivityManager::AddActivity
 * Instead, show it explicitly on caller side. This is necessary as the extensions system controls the app window's visibility.

Remove ActivityViewManager
 Instead of keeping activity -> widget map, use views::Widget::GetWidgetForNativeWindow

BUG=417017
TEST=ActivityManagerTest.Basic

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

[oshima, 2014-09-23 21:57:11]

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

[oshima, 2014-09-23 22:09:34]

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

[oshima, 2014-09-23 22:43:18]

Patchset #1 (id:1) has been deleted

[oshima, 2014-09-23 22:43:24]

Patchset #3 (id:60001) has been deleted

[oshima, 2014-09-23 22:43:54]

Patchset #1 (id:20001) has been deleted

[Mr4D (OOO till 08-26), 2014-09-23 23:14:33]

I like the changelist in general since I did never see a reason for having the
activity_view_manager in the first place.

However - it does not address 2 problems:
1 - The user will be irritated when he starts an app, the overview mode goes
away, but he still sees the old activity before the new activity becomes
visible. This might be a flash - or a multiple second wait in which he/she does
not know what is going on. As such I would recommend showing something. A white
window is much better then nothing.
2 - The resource manager will not be happy that we have content which is
invisible but "Active". This will cause problems when the memory gets squeezy.
[yes, once I have created the browser testing framework you will have failing
tests..]

So I am not sure that we should give the visibility control to the extension
system. Is there something I am missing?

https://codereview.chromium.org/596623005/diff/80001/athena/activity/activity...
File athena/activity/activity_view_manager_impl.cc (left):

https://codereview.chromium.org/596623005/diff/80001/athena/activity/activity...
athena/activity/activity_view_manager_impl.cc:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
Yay! I found this class odd from the beginning...

[oshima, 2014-09-24 00:57:29]

On 2014/09/23 23:14:33, Mr4D wrote:
> I like the changelist in general since I did never see a reason for having the
> activity_view_manager in the first place.
> 
> However - it does not address 2 problems:
> 1 - The user will be irritated when he starts an app, the overview mode goes
> away, but he still sees the old activity before the new activity becomes
> visible. This might be a flash - or a multiple second wait in which he/she
does
> not know what is going on. As such I would recommend showing something. A
white
> window is much better then nothing.

I actually was wrong about why app is controlling visibility. Extension controls
visibilty
because app has control it (it can start with hidden state, or active /
inactive).

The above issue shouldn't be an issue.

> 2 - The resource manager will not be happy that we have content which is
> invisible but "Active". This will cause problems when the memory gets squeezy.
> [yes, once I have created the browser testing framework you will have failing
> tests..]

Where do you see this is happening?

> 
> So I am not sure that we should give the visibility control to the extension
> system. Is there something I am missing?

It should be fine for them to make decision the first visibility state, as it
largely depends on the application's context.
Once it's shown, user and RM can have control.

>
>
https://codereview.chromium.org/596623005/diff/80001/athena/activity/activity...
> File athena/activity/activity_view_manager_impl.cc (left):
> 
>
https://codereview.chromium.org/596623005/diff/80001/athena/activity/activity...
> athena/activity/activity_view_manager_impl.cc:1: // Copyright 2014 The
Chromium
> Authors. All rights reserved.
> Yay! I found this class odd from the beginning...

[Mr4D (OOO till 08-26), 2014-09-24 20:25:51]

The initial state of the window should still be discussed with UX since having
no visual action showing on the screen for a while after clicking a button (and
there is no time limit, even starting the file manager took ~ a second) is not
correct. [As such I would suggest to show a window - even if it is empty, but
some apps might show something where is nothing, so that might not be ideal
either, but the latter is probably less often happening then the former].

For the ResourceManager firing on Focus and order shifting problem I will
investigate once this has landed.

lgtm

[oshima, 2014-09-24 20:28:31]

here is the summary of the offline discussion and my findings.
let me know if I'm missing something.

chrome.app.window API allows apps to create and window in hidden state (
https://developer.chrome.com/apps/app_window#type-CreateWindowOptions),
and show later, or hide later.

The problem is that current WM/Overviewmode code doesn't seem to take
visibility into account, and that's one of reason why you were seeing the issue.
I filed a bug for this issue and we should address it separately
(crbug.com/417412).

There is another issue, reported by skuhne@ that the exiting overview mode can
screw up
the activity order as it may activate the window that should not be activated.
skuhne@ will file a bug for it.

This CL is still necessary as the current code causes unnecessary visibility
switch
when an application window is created in hidden state.

On 2014/09/24 00:57:29, oshima wrote:
> On 2014/09/23 23:14:33, Mr4D wrote:
> > I like the changelist in general since I did never see a reason for having
the
> > activity_view_manager in the first place.
> > 
> > However - it does not address 2 problems:
> > 1 - The user will be irritated when he starts an app, the overview mode goes
> > away, but he still sees the old activity before the new activity becomes
> > visible. This might be a flash - or a multiple second wait in which he/she
> does
> > not know what is going on. As such I would recommend showing something. A
> white
> > window is much better then nothing.
> 
> I actually was wrong about why app is controlling visibility. Extension
controls
> visibilty
> because app has control it (it can start with hidden state, or active /
> inactive).
> 
> The above issue shouldn't be an issue.
> 
> > 2 - The resource manager will not be happy that we have content which is
> > invisible but "Active". This will cause problems when the memory gets
squeezy.
> > [yes, once I have created the browser testing framework you will have
failing
> > tests..]
> 
> Where do you see this is happening?
> 
> > 
> > So I am not sure that we should give the visibility control to the extension
> > system. Is there something I am missing?
> 
> It should be fine for them to make decision the first visibility state, as it
> largely depends on the application's context.
> Once it's shown, user and RM can have control.
> 
> >
> >
>
https://codereview.chromium.org/596623005/diff/80001/athena/activity/activity...
> > File athena/activity/activity_view_manager_impl.cc (left):
> > 
> >
>
https://codereview.chromium.org/596623005/diff/80001/athena/activity/activity...
> > athena/activity/activity_view_manager_impl.cc:1: // Copyright 2014 The
> Chromium
> > Authors. All rights reserved.
> > Yay! I found this class odd from the beginning...

[oshima, 2014-09-24 20:42:15]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-09-24 20:43:30]

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

[commit-bot: I haz the power, 2014-09-24 21:28:59]

Committed patchset #3 (id:100001) as 554dfa327d33d28b5f878faf38d4c2379dfde489

[commit-bot: I haz the power, 2014-09-24 21:29:42]

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