Show/Hide the Arc App windows and corresponding shelf items when switching between desktops.

Show/Hide the Arc App windows and corresponding shelf items when switching between desktops.

BUG=b/27776147

Committed: https://crrev.com/69f2cc12fbcf8127048ee39d3c92cd6d6ba79cfd
Cr-Commit-Position: refs/heads/master@{#398122}

[xdai1, 2016-06-03 21:03:00]

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

[xdai1, 2016-06-03 21:03:01]

khmel@, skuhne@, reveman@, Could you help review this CL please? Thanks! 

+reveman@ as discussed offline. 

This CL can hide all ARC app windows & ARC shelf items when switching from the
primary user's desktop to another user's desktop, and show them when switching
back. However, when switching back, all the arc app windows are transparent, and
one has to move the cursor around to make them show up again. It seems Android
doesn't repaint the layer when a window's statue changes from Hide() to Show().
According to reveman@, this will be addressed by minimize support in b/29114896.

[reveman, 2016-06-03 21:51:48]

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

https://codereview.chromium.org/2033373003/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:389:
aura::Window* lost_active) {
can you explain what the changes to this function are needed for?

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

https://codereview.chromium.org/2033373003/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.h:77: void
UnRegisterApp(AppWindow* app_window);
nit: UnregisterApp

[xdai1, 2016-06-03 22:14:46]

reveman@, I've addressed your comments. Please take another look, thanks!

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

https://codereview.chromium.org/2033373003/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:389:
aura::Window* lost_active) {
On 2016/06/03 21:51:47, reveman wrote:
> can you explain what the changes to this function are needed for?

This change is necessary because otherwise OnTaskSetActive() will be called
whenever there is a change on window activation. OnTaskSetActive() tries to
update the arc shelf item's status, however, in multi-profile scenario, the
corresponding ArcAppWindowLauncherItemController might have been deleted in
UnRegisterApp(), which will then cause a browser crash.

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

https://codereview.chromium.org/2033373003/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.h:77: void
UnRegisterApp(AppWindow* app_window);
On 2016/06/03 21:51:47, reveman wrote:
> nit: UnregisterApp

Done.

[reveman, 2016-06-03 22:49:36]

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

https://codereview.chromium.org/2033373003/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/arc_app_window_launcher_controller.cc:389:
aura::Window* lost_active) {
On 2016/06/03 at 22:14:46, xdai1 wrote:
> On 2016/06/03 21:51:47, reveman wrote:
> > can you explain what the changes to this function are needed for?
> 
> This change is necessary because otherwise OnTaskSetActive() will be called
whenever there is a change on window activation. OnTaskSetActive() tries to
update the arc shelf item's status, however, in multi-profile scenario, the
corresponding ArcAppWindowLauncherItemController might have been deleted in
UnRegisterApp(), which will then cause a browser crash.

Ok, I think that might be a bit easier to understand if we instead handle that
in OnTaskSetActive. And maybe use
ArcAppWindowLauncherController::ControllerForWindow to make it even more clear?

[xdai1, 2016-06-06 18:48:25]

On 2016/06/03 22:49:36, reveman wrote:
> 
> Ok, I think that might be a bit easier to understand if we instead handle that
> in OnTaskSetActive. And maybe use
> ArcAppWindowLauncherController::ControllerForWindow to make it even more
clear?

As you suggested, I handle it in OnTaskSetActive() (which I think is better!)
and with your Minimize support work, it works fine now (no transparent windows
are seen any more). Please take another look, thanks for your review.

[reveman, 2016-06-06 19:01:44]

lgtm

[xdai1, 2016-06-06 19:02:51]

On 2016/06/06 19:01:44, reveman wrote:
> lgtm

Thanks! khmel@, skuhne@, could you help take a look at this CL? Thanks!

[Mr4D (OOO till 08-26), 2016-06-06 20:52:39]

lgtm

[khmel, 2016-06-06 20:53:47]

lgtm

[xdai1, 2016-06-06 20:58:42]

The CQ bit was checked by xdai@chromium.org

[commit-bot: I haz the power, 2016-06-06 20:59:04]

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

[commit-bot: I haz the power, 2016-06-06 21:04:19]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-06 21:06:31]

Description was changed from

==========
Show/Hide the Arc App windows and corresponding shelf items when switching
between desktops.

BUG=b/27776147
==========

to

==========
Show/Hide the Arc App windows and corresponding shelf items when switching
between desktops.

BUG=b/27776147

Committed: https://crrev.com/69f2cc12fbcf8127048ee39d3c92cd6d6ba79cfd
Cr-Commit-Position: refs/heads/master@{#398122}
==========

[commit-bot: I haz the power, 2016-06-06 21:06:32]

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