cros: WallpaperController reparent based on session state

cros: WallpaperController reparent based on session state

- Make WallpaperController do reparenting based on session state;
- Put the reparent methods into private section;
- Update chrome to no longer call the reparent methods;

BUG=678988
Review-Url: https://codereview.chromium.org/2832053003
Cr-Commit-Position: refs/heads/master@{#466398}
Committed: https://chromium.googlesource.com/chromium/src/+/65401397d03401a6c4ff391d0d6ee9bb673ad68c

[xiyuan, 2017-04-20 23:04:29]

The CQ bit was checked by xiyuan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 23:04:41]

Dry run: 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, 2017-04-20 23:06:25]

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

[commit-bot: I haz the power, 2017-04-20 23:06:25]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[xiyuan, 2017-04-21 15:30:57]

xiyuan@chromium.org changed reviewers:
+ jamescook@chromium.org

[xiyuan, 2017-04-21 15:30:58]

https://codereview.chromium.org/2832053003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/lock/screen_locker.cc (left):

https://codereview.chromium.org/2832053003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.cc:514:
ash::Shell::Get()->wallpaper_controller()->MoveToUnlockedContainer();
Covered by session state change to ACTIVE in line 528.

https://codereview.chromium.org/2832053003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/lock/screen_locker.cc:548:
ash::Shell::Get()->wallpaper_controller()->MoveToLockedContainer();
Covered by session state change to LOCKED in line 559.

https://codereview.chromium.org/2832053003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/supervised/supervised_user_creation_screen.cc
(left):

https://codereview.chromium.org/2832053003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/supervised/supervised_user_creation_screen.cc:368:
ash::Shell::Get()->wallpaper_controller()->MoveToLockedContainer();
Not necessary since this happens on the login screen, session state is
LOGIN_PRIMARY, which implies wallpaper in locked containers.

https://codereview.chromium.org/2832053003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/ui/login_display_host_impl.cc (left):

https://codereview.chromium.org/2832053003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:546:
ash::Shell::Get()->wallpaper_controller()->MoveToUnlockedContainer();
For non-user-adding (login, kiosk launch etc), LoginDisplayHostImpl::Finalize is
called before starting user session. When user session starts, session state
becomes ACTIVE and moving wallpaper would happen with that state change now.

https://codereview.chromium.org/2832053003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:659:
ash::Shell::Get()->wallpaper_controller()->MoveToLockedContainer();
Not necessary now. This is called from UserAddingScreenImpl, which would change
session state to LOGIN_SECONDARY to make this happen.

https://codereview.chromium.org/2832053003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:917:
ash::Shell::Get()->wallpaper_controller()->MoveToLockedContainer();
User image picker etc is under session state LOGGED_IN_NOT_ACTIVE and implies
this.

https://codereview.chromium.org/2832053003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/ui/login_display_host_impl.cc:1052:
ash::Shell::Get()->wallpaper_controller()->MoveToUnlockedContainer();
This is for user adding screen and should happen when UserAddingScreenImpl
changes session state back to ACTIVE.

[James Cook, 2017-04-21 17:30:55]

LGTM

https://codereview.chromium.org/2832053003/diff/20001/ash/wallpaper/wallpaper...
File ash/wallpaper/wallpaper_controller.cc (right):

https://codereview.chromium.org/2832053003/diff/20001/ash/wallpaper/wallpaper...
ash/wallpaper/wallpaper_controller.cc:399: VLOG(1) << "Moving wallpaper to
locked container";
DVLOG? Do we need these in production?

https://codereview.chromium.org/2832053003/diff/20001/ash/wallpaper/wallpaper...
ash/wallpaper/wallpaper_controller.cc:408: VLOG(1) << "Moving wallpaper to
unlocked container";
ditto

https://codereview.chromium.org/2832053003/diff/20001/ash/wallpaper/wallpaper...
File ash/wallpaper/wallpaper_controller.h (right):

https://codereview.chromium.org/2832053003/diff/20001/ash/wallpaper/wallpaper...
ash/wallpaper/wallpaper_controller.h:15: #include "base/compiler_specific.h"
nit: still needed?

https://codereview.chromium.org/2832053003/diff/20001/ash/wallpaper/wallpaper...
ash/wallpaper/wallpaper_controller.h:39: // Controls the desktop background
wallpaper.
totally optional: Maybe add some docs about how wallpaper works with screen
locking? I presume it changes containers to hide open windows on the desktop,
but it would be nice to say that somewhere. I wish this directory had a README,
but class-level docs would be nice.

https://codereview.chromium.org/2832053003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/supervised/supervised_user_creation_screen.cc
(left):

https://codereview.chromium.org/2832053003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/supervised/supervised_user_creation_screen.cc:368:
ash::Shell::Get()->wallpaper_controller()->MoveToLockedContainer();
On 2017/04/21 15:30:58, xiyuan wrote:
> Not necessary since this happens on the login screen, session state is
> LOGIN_PRIMARY, which implies wallpaper in locked containers.

Thanks for cleaning this up!

[xiyuan, 2017-04-21 18:09:35]

https://codereview.chromium.org/2832053003/diff/20001/ash/wallpaper/wallpaper...
File ash/wallpaper/wallpaper_controller.cc (right):

https://codereview.chromium.org/2832053003/diff/20001/ash/wallpaper/wallpaper...
ash/wallpaper/wallpaper_controller.cc:399: VLOG(1) << "Moving wallpaper to
locked container";
On 2017/04/21 17:30:55, James Cook wrote:
> DVLOG? Do we need these in production?

Probably not needed. Never felt it is useful. Removed.

https://codereview.chromium.org/2832053003/diff/20001/ash/wallpaper/wallpaper...
ash/wallpaper/wallpaper_controller.cc:408: VLOG(1) << "Moving wallpaper to
unlocked container";
On 2017/04/21 17:30:55, James Cook wrote:
> ditto

Removed.

https://codereview.chromium.org/2832053003/diff/20001/ash/wallpaper/wallpaper...
File ash/wallpaper/wallpaper_controller.h (right):

https://codereview.chromium.org/2832053003/diff/20001/ash/wallpaper/wallpaper...
ash/wallpaper/wallpaper_controller.h:15: #include "base/compiler_specific.h"
On 2017/04/21 17:30:55, James Cook wrote:
> nit: still needed?

Done.

https://codereview.chromium.org/2832053003/diff/20001/ash/wallpaper/wallpaper...
ash/wallpaper/wallpaper_controller.h:39: // Controls the desktop background
wallpaper.
On 2017/04/21 17:30:55, James Cook wrote:
> totally optional: Maybe add some docs about how wallpaper works with screen
> locking? I presume it changes containers to hide open windows on the desktop,
> but it would be nice to say that somewhere. I wish this directory had a
README,
> but class-level docs would be nice.

Done.

[xiyuan, 2017-04-21 18:09:54]

The CQ bit was checked by xiyuan@chromium.org

[xiyuan, 2017-04-21 18:09:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org
Link to the patchset: https://codereview.chromium.org/2832053003/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2017-04-21 18:10:21]

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, 2017-04-21 18:44:32]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492798194909460,
"parent_rev": "a07bd8034e0ebf854efb8cd80eb7d543d7b8b228", "commit_rev":
"65401397d03401a6c4ff391d0d6ee9bb673ad68c"}

[commit-bot: I haz the power, 2017-04-21 18:44:43]

Description was changed from

==========
cros: WallpaperController reparent based on session state

- Make WallpaperController do reparenting based on session state;
- Put the reparent methods into private section;
- Update chrome to no longer call the reparent methods;

BUG=678988
==========

to

==========
cros: WallpaperController reparent based on session state

- Make WallpaperController do reparenting based on session state;
- Put the reparent methods into private section;
- Update chrome to no longer call the reparent methods;

BUG=678988

Review-Url: https://codereview.chromium.org/2832053003
Cr-Commit-Position: refs/heads/master@{#466398}
Committed:
https://chromium.googlesource.com/chromium/src/+/65401397d03401a6c4ff391d0d6e...
==========

[commit-bot: I haz the power, 2017-04-21 18:44:44]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/65401397d03401a6c4ff391d0d6e...