arc: Support Arc window showing/hiding on a user's profile switch.

arc: Support Arc window showing/hiding on a user's profile switch.

This fixes several cases when multi-user manager and shelf controller
is unable to restore Arc window correctly when the user changes his
profile.

BUG=b/31493349
TEST=Manually on a device. Arc app windows automatically restored when
     the user switch backs to main profile.

Committed: https://crrev.com/6f385b640644bf1cf3c132d5cae98f347c016606
Cr-Commit-Position: refs/heads/master@{#418905}

[khmel, 2016-09-14 23:42:18]

khmel@chromium.org changed reviewers:
+ oshima@chromium.org, reveman@chromium.org, skuhne@chromium.org

[khmel, 2016-09-14 23:42:19]

PTAL

https://codereview.chromium.org/2339313002/diff/1/chrome/browser/ui/ash/launc...
File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc
(right):

https://codereview.chromium.org/2339313002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:95:
views::Widget* widget,
Setting widget is legacy code for one of the first arc window impl. Now it is
not required once Arc app has separate window. Moreover changing widget (and
native window) runtime is crash condition in controllers (problem with closing
windows for example).
Now it safe to discard it.

https://codereview.chromium.org/2339313002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:165: void
Show() override { widget_->Show(); }
This is needed to correctly restore from clicking on shelf items in several
cases.

https://codereview.chromium.org/2339313002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:173: void
Activate() override { widget_->Activate(); }
As discussed let use wayland protocol whenever possible.

https://codereview.chromium.org/2339313002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:291: void
ArcAppWindowLauncherController::OnWindowVisibilityChanged(
Quite tricky case. We used OnWindowVisibilityChanging event to attach controller
to window. This also registers window in multi user manager. Although window is
changing to visible, it is visible status is still off. Multi-user manager
stores its current status which is off. Of cause it sets window observer and
listen the same OnWindowVisibilityChanging. It is not called for new listener
and multi user manager thinks that Arc window is invisible. When we switch user
to primary profile back, multi user window does not show our window.

[Mr4D (OOO till 08-26), 2016-09-15 14:03:19]

I think this is lgtm, but please wait for Oshima's review as well.

https://codereview.chromium.org/2339313002/diff/1/chrome/browser/ui/ash/launc...
File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc
(right):

https://codereview.chromium.org/2339313002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:213:
views::Widget* widget_;
could this be then const?

https://codereview.chromium.org/2339313002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:291: void
ArcAppWindowLauncherController::OnWindowVisibilityChanged(
You might want to add a comment here towards that. Just to make sure that a
viewer of the codes does understand why you use this and not the other.

[khmel, 2016-09-15 15:43:27]

Thanks Stefan for review!

Your comments were addressed.

https://codereview.chromium.org/2339313002/diff/1/chrome/browser/ui/ash/launc...
File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc
(right):

https://codereview.chromium.org/2339313002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:213:
views::Widget* widget_;
On 2016/09/15 14:03:19, Mr4D wrote:
> could this be then const?

Indeed

https://codereview.chromium.org/2339313002/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:291: void
ArcAppWindowLauncherController::OnWindowVisibilityChanged(
On 2016/09/15 14:03:19, Mr4D wrote:
> You might want to add a comment here towards that. Just to make sure that a
> viewer of the codes does understand why you use this and not the other.

Done.

[oshima, 2016-09-15 17:14:32]

CL itself lgtm, but I have a few questions w.r.t multi profile handling.

https://codereview.chromium.org/2339313002/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc
(right):

https://codereview.chromium.org/2339313002/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:300: if
(visible && observed_profile_ == owner()->GetProfile())
It looks to me that observerd_profile_ and owner()->GetProfile() are always same
when enabled. Am I missing something?

https://codereview.chromium.org/2339313002/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:309:
window->RemoveObserver(this);
can you also stop observing if the window belongs to another user?

[khmel, 2016-09-15 17:32:15]

Thanks for review!

https://codereview.chromium.org/2339313002/diff/20001/chrome/browser/ui/ash/l...
File chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc
(right):

https://codereview.chromium.org/2339313002/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:300: if
(visible && observed_profile_ == owner()->GetProfile())
On 2016/09/15 17:14:32, oshima wrote:
> It looks to me that observerd_profile_ and owner()->GetProfile() are always
same
> when enabled. Am I missing something?
> 

Actually not, observerd_profile_ is set on creation time when primary profile is
active. Profile can be changed to secondary profile. In this case
ChromeContollerLauncher has another profile attached (owner()->GetProfile()).
This class is notified via ActiveUserChanged()

https://codereview.chromium.org/2339313002/diff/20001/chrome/browser/ui/ash/l...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:309:
window->RemoveObserver(this);
On 2016/09/15 17:14:32, oshima wrote:
> can you also stop observing if the window belongs to another user?

Will dig into this in next CLs, there are several edge cases here, not safe to
it right now. Key issue here on the moment of creation we can not set if this
Arc window or not. In this case we monitor all top level  window. But
potentially this could be good place to optimize.

[khmel, 2016-09-15 17:32:17]

Thanks for review!

[khmel, 2016-09-15 17:32:38]

The CQ bit was checked by khmel@chromium.org

[khmel, 2016-09-15 17:32:38]

The patchset sent to the CQ was uploaded after l-g-t-m from skuhne@chromium.org
Link to the patchset: https://codereview.chromium.org/2339313002/#ps20001
(title: "comments")

[commit-bot: I haz the power, 2016-09-15 17:33:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-15 18:16:22]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-15 18:19:07]

Description was changed from

==========
arc: Support Arc window showing/hiding on a user's profile switch.

This fixes several cases when multi-user manager and shelf controller
is unable to restore Arc window correctly when the user changes his
profile.

BUG=b/31493349
TEST=Manually on a device. Arc app windows automatically restored when
     the user switch backs to main profile.
==========

to

==========
arc: Support Arc window showing/hiding on a user's profile switch.

This fixes several cases when multi-user manager and shelf controller
is unable to restore Arc window correctly when the user changes his
profile.

BUG=b/31493349
TEST=Manually on a device. Arc app windows automatically restored when
     the user switch backs to main profile.

Committed: https://crrev.com/6f385b640644bf1cf3c132d5cae98f347c016606
Cr-Commit-Position: refs/heads/master@{#418905}
==========

[commit-bot: I haz the power, 2016-09-15 18:19:08]

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