ash: Use SessionController instead of SessionStateDelegate

ash: Use SessionController instead of SessionStateDelegate

- Switch SessionStateDelegate callers to SessionController;
- Remove all migrated methods from SessionStateDelegate;
- Update tests;
- Update ash_shell_with_content;
- Changed ScreenLocker to set SessionState::ACTIVE before destroying
  LockWindow so that focus is properly restored after lock;
- Changed UserSessionManager to set SessionState::ACTIVE before
  creating browser window so that the created window receives focus;

BUG=648964, 699234
Review-Url: https://codereview.chromium.org/2734933004
Cr-Commit-Position: refs/heads/master@{#458168}
Committed: https://chromium.googlesource.com/chromium/src/+/f1ff14d7d25d3c37239e6804a3bfd6928c9cd1ef

[xiyuan, 2017-03-15 20:33:26]

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

[commit-bot: I haz the power, 2017-03-15 20:33:50]

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-03-15 20:56:13]

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

[commit-bot: I haz the power, 2017-03-15 20:56:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[xiyuan, 2017-03-15 23:08:03]

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

[commit-bot: I haz the power, 2017-03-15 23:08: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-03-16 00:03:08]

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

[commit-bot: I haz the power, 2017-03-16 00:03:09]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[xiyuan, 2017-03-16 22:40:12]

Patchset #4 (id:60001) has been deleted

[xiyuan, 2017-03-16 22:41:06]

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

[commit-bot: I haz the power, 2017-03-16 22:41:28]

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-03-16 22:45:05]

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

[commit-bot: I haz the power, 2017-03-16 22:45:06]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[xiyuan, 2017-03-16 22:55:52]

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

[commit-bot: I haz the power, 2017-03-16 22:56:22]

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-03-17 00:19:37]

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

[commit-bot: I haz the power, 2017-03-17 00:19:39]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2017-03-17 07:04:13]

Description was changed from

==========
ash: Use SessionController instead of SessionStateDelegate

- Switch SessionStateDelegate callers to SessionController;
- Remove all migrated methods from SessionStateDelegate;
- Update tests;
- Update ash_shell_with_content;

BUG=648964
==========

to

==========
ash: Use SessionController instead of SessionStateDelegate

- Switch SessionStateDelegate callers to SessionController;
- Remove all migrated methods from SessionStateDelegate;
- Update tests;
- Update ash_shell_with_content;
- Changed ScreenLocker to set SessionState::ACTIVE before destroying
  LockWindow so that focus is properly restored after lock;
- Changed UserSessionManager to set SessionState::ACTIVE before
  creating browser window so that the created window receives focus;

BUG=648964
==========

[xiyuan, 2017-03-17 07:08:13]

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

[xiyuan, 2017-03-17 07:08:16]

Most of the change are mechanical replacement. I have added comment to behavior
changes to make it easier to review.

Sorry for the big CL. :p
And many thanks.

https://codereview.chromium.org/2734933004/diff/100001/ash/accelerators/accel...
File ash/accelerators/accelerator_interactive_uitest_chromeos.cc (left):

https://codereview.chromium.org/2734933004/diff/100001/ash/accelerators/accel...
ash/accelerators/accelerator_interactive_uitest_chromeos.cc:201:
mojo::edk::Init();
Removed because mojo init is added to AshInteractiveUITestBase::SetUp, to make
base class AshTestBase::Setup able to bind TestSessionConrollerClient.

https://codereview.chromium.org/2734933004/diff/100001/ash/common/session/ses...
File ash/common/session/session_controller.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/common/session/ses...
ash/common/session/session_controller.cc:43: return IsActiveUserSessionStarted()
&& can_lock_;
Behavior change, caught by LockStateControllerTest.LockButtonBasicNotLoggedIn.
Chrome side SessionControllerClient guarantees that but we were not enforcing it
here in ash.

https://codereview.chromium.org/2734933004/diff/100001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_locking_manager.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/common/shelf/shelf...
ash/common/shelf/shelf_locking_manager.cc:15: : shelf_(shelf),
stored_alignment_(shelf->GetAlignment()) {
Initialize |stored_alignment_| with the current alignment of |shelf_|, caught by
ShelfWidgetTestWithInitializer.CreateLockedShelf.

New test setup fires SessionStateChanged and mocks a user has signed in. Thus
ShelfLockingManager::SessionStateChanged would set |shelf_| alignment to
|stored_alignment_|. Without this, |shelf_| alignment is initialized to
SHELF_ALIGNMENT_BOTTOM. This would also happen in production code and could
trigger unnecessary workspace change.

https://codereview.chromium.org/2734933004/diff/100001/ash/common/system/user...
File ash/common/system/user/tray_user_unittest.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/common/system/user...
ash/common/system/user/tray_user_unittest.cc:243: const mojom::UserSession*
second_user = controller()->GetUserSession(1);
This has to be changed because the old code depends on the fact that
SwitchActiveUser does not change the user list order in
TestSessionStateDelegate. This is not consistent with production code. The user
list that ash knows is a LRU list. After SwitchActiveUser, the list order should
have changed.

https://codereview.chromium.org/2734933004/diff/100001/ash/common/wm/workspac...
File ash/common/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/common/wm/workspac...
ash/common/wm/workspace/workspace_layout_manager_unittest.cc:901:
std::unique_ptr<test::TestSessionControllerClient> session_controller_client =
Creating a new TestSessionControllerClient because
WorkspaceLayoutManagerSoloTest is based on AshTest, which hides the
AshTestBase/AshTestHelper as implementation details. I could not figure out a
way to get the TestSessionControllerClient in AshTestHelper. :(

https://codereview.chromium.org/2734933004/diff/100001/ash/root_window_contro...
File ash/root_window_controller_unittest.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/root_window_contro...
ash/root_window_controller_unittest.cc:937: RunAllPendingInMessageLoop();
This is now needed because we need to spin the message loop after LockScreen.
The pending mouse move events would mess up the mouse event counting. So drain
these events before starting to count mouse events.

https://codereview.chromium.org/2734933004/diff/100001/ash/shelf/shelf_layout...
File ash/shelf/shelf_layout_manager_unittest.cc (left):

https://codereview.chromium.org/2734933004/diff/100001/ash/shelf/shelf_layout...
ash/shelf/shelf_layout_manager_unittest.cc:1552:
EXPECT_NE(SHELF_BACKGROUND_DEFAULT, GetShelfWidget()->GetBackgroundType());
New test setup using TestSessionControllerClient fires SessionStateChanged.
Hence SHELF_BACKGROUND_DEFAULT would be the one when the test starts.

It works with old test setup using TestSessionStateDelegate because it does not
support SessionStateObserver and does not notify observers.

https://codereview.chromium.org/2734933004/diff/100001/ash/test/ash_test_base.h
File ash/test/ash_test_base.h (left):

https://codereview.chromium.org/2734933004/diff/100001/ash/test/ash_test_base...
ash/test/ash_test_base.h:157: void SetSessionStarting();
No one calls this.

https://codereview.chromium.org/2734933004/diff/100001/ash/wm/always_on_top_c...
File ash/wm/always_on_top_controller_unittest.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/wm/always_on_top_c...
ash/wm/always_on_top_controller_unittest.cc:70:
controller->DeactivateKeyboard(keyboard_controller);
This is needed now because TestSessionControllerClient fires
SessionStateChanged. Hence WmShellAura::SessionStateChanged will be called to
CreateKeyboard, which implicitly ActivateKeyboard before test body runs.

https://codereview.chromium.org/2734933004/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/lock/screen_locker.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/lock/screen_locker.cc:489: }));
This was caught by ScreenLockerTest.TestFullscreenExit.

https://codereview.chromium.org/2734933004/diff/100001/chrome/browser/chromeo...
File
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_browsertest.cc
(right):

https://codereview.chromium.org/2734933004/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_browsertest.cc:121:
SessionControllerClient::FlushForTesting();
This would cause shelf to be created now.

https://codereview.chromium.org/2734933004/diff/100001/ui/aura/mus/window_tre...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ui/aura/mus/window_tre...
ui/aura/mus/window_tree_client.cc:1595: if (window_manager_internal_client_) {
Needed for WorkspaceLayoutManagerSoloTest.NotResizeWhenScreenIsLocked running as
part of mash_unittests. |window_manager_internal_client_| is not bound somehow.

[James Cook, 2017-03-17 17:14:33]

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

[James Cook, 2017-03-17 17:14:37]

msw, can you take a look at ash/common/shelf/shelf_locking_manager.cc?

Wow, epic CL! I like it. Mostly nits. My biggest concerns are high dpi bitmaps
and the FlushForTesting in ChromeSessionManager startup.

https://codereview.chromium.org/2734933004/diff/100001/ash/accelerators/accel...
File ash/accelerators/accelerator_filter_unittest.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/accelerators/accel...
ash/accelerators/accelerator_filter_unittest.cc:161:
RunAllPendingInMessageLoop();  // LockScreen is an async mojo call.
What do you think of adding SessionController::FlushForTesting() or
FlushMojoForTesting() or MojoFlushForTesting() that calls FlushForTesting() on
the mojo bindingset?

Then this would be:
session_controller->FlushMojoForTesting();  // For LockScreen.

which might be more self-documenting.

Update: Or you could just use LockScreenAndFlushForTesting() :-)

https://codereview.chromium.org/2734933004/diff/100001/ash/common/session/ses...
File ash/common/session/session_controller_unittest.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/common/session/ses...
ash/common/session/session_controller_unittest.cc:131: // Cannot lock screen
when there is no active user session.
nit: Put this in a separate test case.

https://codereview.chromium.org/2734933004/diff/100001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_locking_manager.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/common/shelf/shelf...
ash/common/shelf/shelf_locking_manager.cc:15: : shelf_(shelf),
stored_alignment_(shelf->GetAlignment()) {
On 2017/03/17 07:08:15, xiyuan wrote:
> Initialize |stored_alignment_| with the current alignment of |shelf_|, caught
by
> ShelfWidgetTestWithInitializer.CreateLockedShelf.
> 
> New test setup fires SessionStateChanged and mocks a user has signed in. Thus
> ShelfLockingManager::SessionStateChanged would set |shelf_| alignment to
> |stored_alignment_|. Without this, |shelf_| alignment is initialized to
> SHELF_ALIGNMENT_BOTTOM. This would also happen in production code and could
> trigger unnecessary workspace change.

msw, can you look at this piece of the CL? I think warx@ recently had a bug with
shelf locking during login.

(Also, if you initialize this here please remove the initialization from the
header file.)

(Also also, DCHECK(shelf_) while you're here.)

https://codereview.chromium.org/2734933004/diff/100001/ash/common/system/tray...
File ash/common/system/tray/system_tray.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/common/system/tray...
ash/common/system/tray/system_tray.cc:259: int maximum_user_profiles =
nit: const

https://codereview.chromium.org/2734933004/diff/100001/ash/common/system/user...
File ash/common/system/user/tray_user.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/common/system/user...
ash/common/system/user/tray_user.cc:249:
avatar_->SetImage(gfx::ImageSkia::CreateFrom1xBitmap(user_session->avatar),
Do we have high-dpi user images? If so, this might break them.

If we do we could ship both scale factors as separate fields in the mojom, at
least until we have proper support for high dpi, see crbug.com/667566

https://codereview.chromium.org/2734933004/diff/100001/ash/common/system/user...
File ash/common/system/user/tray_user_unittest.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/common/system/user...
ash/common/system/user/tray_user_unittest.cc:245: // Switch to a new user
"Second@tray" - which has a capitalized name.
For stuff like this could we provide a setter in TestSessionStateClient that
lets you reset the name of a user? Or
SessionController::SetUserNameForTesting()? Then the "Capitalized@example.com"
could be part of the test code.

https://codereview.chromium.org/2734933004/diff/100001/ash/common/system/user...
File ash/common/system/user/user_card_view.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/common/system/user...
ash/common/system/user/user_card_view.cc:67:
image_view->SetImage(gfx::ImageSkia::CreateFrom1xBitmap(
ditto re high dpi

https://codereview.chromium.org/2734933004/diff/100001/ash/common/system/user...
File ash/common/system/user/user_view.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/common/system/user...
ash/common/system/user/user_view.cc:57: DCHECK(user_index > 0);
nit: Also DCHECK less than session count

https://codereview.chromium.org/2734933004/diff/100001/ash/common/test/test_s...
File ash/common/test/test_session_controller_client.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/common/test/test_s...
ash/common/test/test_session_controller_client.cc:25: "First@tray",
Yuck. Do these have to match particular tests in ash/chrome browser? If so it
might be nice to fix those tests in a follow-up CL to use the user%d pattern.

https://codereview.chromium.org/2734933004/diff/100001/ash/common/test/test_s...
ash/common/test/test_session_controller_client.cc:31: // without introducing
dependency on google_api.
Nice comment.

https://codereview.chromium.org/2734933004/diff/100001/ash/common/test/test_s...
ash/common/test/test_session_controller_client.cc:44:
session_info_(mojom::SessionInfo::New()) {}
nit: DCHECK(controller_)

https://codereview.chromium.org/2734933004/diff/100001/ash/common/test/test_s...
ash/common/test/test_session_controller_client.cc:58: void
TestSessionControllerClient::Reset() {
Should the constructor call this?

Alternately, should this just replace session_info_ with a
mojom::SessionInfo::New()?

https://codereview.chromium.org/2734933004/diff/100001/ash/common/test/test_s...
ash/common/test/test_session_controller_client.cc:83: // TODO(xiyuan): Remove
after LoginStatus becomes part of SessionController.
Is this still necessary after my CL?

https://codereview.chromium.org/2734933004/diff/100001/ash/common/test/test_s...
ash/common/test/test_session_controller_client.cc:141: for (auto& user_session :
controller_->GetUserSessions()) {
const?

https://codereview.chromium.org/2734933004/diff/100001/ash/common/test/test_s...
ash/common/test/test_session_controller_client.cc:181: auto it =
std::find_if(sessions.begin(), sessions.end(),
+1 for using find_if(). I'm starting to like small lambdas.

https://codereview.chromium.org/2734933004/diff/100001/ash/common/test/test_s...
File ash/common/test/test_session_controller_client.h (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/common/test/test_s...
ash/common/test/test_session_controller_client.h:26: // NOT use this.
nit: Maybe add to comment an example of that kind of test. (Like, can tests in
chrome use this?)

I might also say something about how it simulates chrome's behavior (just to
emphasize this exists to break an ash/chrome dependency to allow testing ash
code in isolation).

https://codereview.chromium.org/2734933004/diff/100001/ash/common/test/test_s...
ash/common/test/test_session_controller_client.h:43: // Sets the |count|
pre-defined user sessions. The first four user session
super nit: "Creates |count| pre-defined user sessions."

https://codereview.chromium.org/2734933004/diff/100001/ash/common/test/test_s...
ash/common/test/test_session_controller_client.h:48: void
SetPredefinedUserSessions(int count);
optional: Maybe CreatePredefinedUserSessions()?

https://codereview.chromium.org/2734933004/diff/100001/ash/common/wm/workspac...
File ash/common/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/common/wm/workspac...
ash/common/wm/workspace/workspace_layout_manager_unittest.cc:901:
std::unique_ptr<test::TestSessionControllerClient> session_controller_client =
On 2017/03/17 07:08:15, xiyuan wrote:
> Creating a new TestSessionControllerClient because
> WorkspaceLayoutManagerSoloTest is based on AshTest, which hides the
> AshTestBase/AshTestHelper as implementation details. I could not figure out a
> way to get the TestSessionControllerClient in AshTestHelper. :(

Could this test be switched to use AshTestBase? We don't care about the AshTest
vs. AshTestBase distinction any more. (This could be in a follow-up CL.)

https://codereview.chromium.org/2734933004/diff/100001/ash/common/wm_shell.cc
File ash/common/wm_shell.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/common/wm_shell.cc...
ash/common/wm_shell.cc:97: // Must occur after SessionStateDelegate creation and
user login.
Is this comment still correct?

https://codereview.chromium.org/2734933004/diff/100001/ash/mus/bridge/wm_shel...
File ash/mus/bridge/wm_shell_mus.cc (left):

https://codereview.chromium.org/2734933004/diff/100001/ash/mus/bridge/wm_shel...
ash/mus/bridge/wm_shell_mus.cc:83: return session_manager::SessionState::ACTIVE;
Hooray, mash will be happier with this change!

https://codereview.chromium.org/2734933004/diff/100001/ash/mus/shell_delegate...
File ash/mus/shell_delegate_mus.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/mus/shell_delegate...
ash/mus/shell_delegate_mus.cc:27: class SessionStateDelegateStub : public
SessionStateDelegate {
We probably don't need two SessionStateDelegateStub implementations (here and in
wm_shell_mus.cc). It would be good to remove one of them. Can you file a bug for
that? I'm happy to help with that if you're busy.

https://codereview.chromium.org/2734933004/diff/100001/ash/shelf/shelf_layout...
File ash/shelf/shelf_layout_manager_unittest.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/shelf/shelf_layout...
ash/shelf/shelf_layout_manager_unittest.cc:1553: std::unique_ptr<aura::Window>
w1(CreateTestWindow());
super nit: how about |window| instead of |w1|?

https://codereview.chromium.org/2734933004/diff/100001/ash/shell/example_sess...
File ash/shell/example_session_controller_client.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/shell/example_sess...
ash/shell/example_session_controller_client.cc:23: DCHECK_EQ(instance, nullptr);
nit: Also DCHECK(controller)

https://codereview.chromium.org/2734933004/diff/100001/ash/shell/shell_delega...
File ash/shell/shell_delegate_impl.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/shell/shell_delega...
ash/shell/shell_delegate_impl.cc:62: SessionStateDelegateImpl() : user_info_(new
user_manager::UserInfoImpl()) {}
This could also use a shared SessionStateDelegateStub

https://codereview.chromium.org/2734933004/diff/100001/ash/system/chromeos/po...
File ash/system/chromeos/power/tablet_power_button_controller_unittest.cc
(right):

https://codereview.chromium.org/2734933004/diff/100001/ash/system/chromeos/po...
ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:114:
RunAllPendingInMessageLoop();
This might be a good candidate for SessionController::FlushMojoForTesting()

https://codereview.chromium.org/2734933004/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/lock/screen_locker.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/lock/screen_locker.cc:486:
base::Bind(&ScreenLocker::ScheduleDeletion);
nit: maybe inline this below?

https://codereview.chromium.org/2734933004/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/lock/screen_locker_browsertest.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/lock/screen_locker_browsertest.cc:239: // Browser
window should be activated after screen locker is gone. Otherwise,
Thanks for adding comments like this. They are really helpful when reading the
tests.

https://codereview.chromium.org/2734933004/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/session/chrome_session_manager.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/session/chrome_session_manager.cc:142:
SessionControllerClient::FlushForTesting();
Hrm. Do we have to do this?

I remember we added this line before when I was working on asynchronous shelf
creation. It seems like this makes the tests much different than production.

Frankly I think it might be better to delete/disable that one focus test instead
of changing all tests to have a synchronous flush during startup.

What do you think?

https://codereview.chromium.org/2734933004/diff/100001/chrome/browser/chromeo...
File
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_browsertest.cc
(right):

https://codereview.chromium.org/2734933004/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_browsertest.cc:121:
SessionControllerClient::FlushForTesting();
On 2017/03/17 07:08:16, xiyuan wrote:
> This would cause shelf to be created now.

Acknowledged.

Aside, not related to this CL: I've often had trouble with these wallpaper tests
while working on mustash. I wonder if there's something fundamentally wrong with
how they are initializing things.

https://codereview.chromium.org/2734933004/diff/100001/ui/aura/mus/window_tre...
File ui/aura/mus/window_tree_client.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ui/aura/mus/window_tre...
ui/aura/mus/window_tree_client.cc:1595: if (window_manager_internal_client_) {
On 2017/03/17 07:08:16, xiyuan wrote:
> Needed for WorkspaceLayoutManagerSoloTest.NotResizeWhenScreenIsLocked running
as
> part of mash_unittests. |window_manager_internal_client_| is not bound
somehow.

This seems fine -- we've checking it for null everywhere else. :-)

[xiyuan, 2017-03-17 17:24:50]

Description was changed from

==========
ash: Use SessionController instead of SessionStateDelegate

- Switch SessionStateDelegate callers to SessionController;
- Remove all migrated methods from SessionStateDelegate;
- Update tests;
- Update ash_shell_with_content;
- Changed ScreenLocker to set SessionState::ACTIVE before destroying
  LockWindow so that focus is properly restored after lock;
- Changed UserSessionManager to set SessionState::ACTIVE before
  creating browser window so that the created window receives focus;

BUG=648964
==========

to

==========
ash: Use SessionController instead of SessionStateDelegate

- Switch SessionStateDelegate callers to SessionController;
- Remove all migrated methods from SessionStateDelegate;
- Update tests;
- Update ash_shell_with_content;
- Changed ScreenLocker to set SessionState::ACTIVE before destroying
  LockWindow so that focus is properly restored after lock;
- Changed UserSessionManager to set SessionState::ACTIVE before
  creating browser window so that the created window receives focus;

BUG=648964,699234
==========

[msw, 2017-03-17 18:10:46]

https://codereview.chromium.org/2734933004/diff/100001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_locking_manager.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/common/shelf/shelf...
ash/common/shelf/shelf_locking_manager.cc:15: : shelf_(shelf),
stored_alignment_(shelf->GetAlignment()) {
On 2017/03/17 17:14:35, James Cook wrote:
> On 2017/03/17 07:08:15, xiyuan wrote:
> > Initialize |stored_alignment_| with the current alignment of |shelf_|,
caught
> by
> > ShelfWidgetTestWithInitializer.CreateLockedShelf.
> > 
> > New test setup fires SessionStateChanged and mocks a user has signed in.
Thus
> > ShelfLockingManager::SessionStateChanged would set |shelf_| alignment to
> > |stored_alignment_|. Without this, |shelf_| alignment is initialized to
> > SHELF_ALIGNMENT_BOTTOM. This would also happen in production code and could
> > trigger unnecessary workspace change.
> 
> msw, can you look at this piece of the CL? I think warx@ recently had a bug
with
> shelf locking during login.
> 
> (Also, if you initialize this here please remove the initialization from the
> header file.)
> 
> (Also also, DCHECK(shelf_) while you're here.)

This small part of the change lgtm (with James' suggestions). I'm not sure this
has any ramifications outside of test environments, but it certainly can't hurt.

[xiyuan, 2017-03-17 22:46:36]

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

[commit-bot: I haz the power, 2017-03-17 22:47:39]

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

[xiyuan, 2017-03-17 22:52:03]

Already have conflicts. So no trybots results for now. :(

https://codereview.chromium.org/2734933004/diff/100001/ash/accelerators/accel...
File ash/accelerators/accelerator_filter_unittest.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/accelerators/accel...
ash/accelerators/accelerator_filter_unittest.cc:161:
RunAllPendingInMessageLoop();  // LockScreen is an async mojo call.
On 2017/03/17 17:14:35, James Cook wrote:
> What do you think of adding SessionController::FlushForTesting() or
> FlushMojoForTesting() or MojoFlushForTesting() that calls FlushForTesting() on
> the mojo bindingset?
> 
> Then this would be:
> session_controller->FlushMojoForTesting();  // For LockScreen.
> 
> which might be more self-documenting.
> 
> Update: Or you could just use LockScreenAndFlushForTesting() :-)
> 

Done. Added a SessionController::FlushMojoForTest and use that. 

I could not use LockScreenAndFlushForTesting() here because the actual LockScree
call is in accelerator handler production code.

https://codereview.chromium.org/2734933004/diff/100001/ash/common/session/ses...
File ash/common/session/session_controller_unittest.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/common/session/ses...
ash/common/session/session_controller_unittest.cc:131: // Cannot lock screen
when there is no active user session.
On 2017/03/17 17:14:35, James Cook wrote:
> nit: Put this in a separate test case.

Done.

https://codereview.chromium.org/2734933004/diff/100001/ash/common/shelf/shelf...
File ash/common/shelf/shelf_locking_manager.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/common/shelf/shelf...
ash/common/shelf/shelf_locking_manager.cc:15: : shelf_(shelf),
stored_alignment_(shelf->GetAlignment()) {
On 2017/03/17 17:14:35, James Cook wrote:
> On 2017/03/17 07:08:15, xiyuan wrote:
> > Initialize |stored_alignment_| with the current alignment of |shelf_|,
caught
> by
> > ShelfWidgetTestWithInitializer.CreateLockedShelf.
> > 
> > New test setup fires SessionStateChanged and mocks a user has signed in.
Thus
> > ShelfLockingManager::SessionStateChanged would set |shelf_| alignment to
> > |stored_alignment_|. Without this, |shelf_| alignment is initialized to
> > SHELF_ALIGNMENT_BOTTOM. This would also happen in production code and could
> > trigger unnecessary workspace change.
> 
> msw, can you look at this piece of the CL? I think warx@ recently had a bug
with
> shelf locking during login.
> 
> (Also, if you initialize this here please remove the initialization from the
> header file.)
> 
> (Also also, DCHECK(shelf_) while you're here.)

Removed initialization from header and added DCHECK.

https://codereview.chromium.org/2734933004/diff/100001/ash/common/system/tray...
File ash/common/system/tray/system_tray.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/common/system/tray...
ash/common/system/tray/system_tray.cc:259: int maximum_user_profiles =
On 2017/03/17 17:14:35, James Cook wrote:
> nit: const

Done.

https://codereview.chromium.org/2734933004/diff/100001/ash/common/system/user...
File ash/common/system/user/tray_user.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/common/system/user...
ash/common/system/user/tray_user.cc:249:
avatar_->SetImage(gfx::ImageSkia::CreateFrom1xBitmap(user_session->avatar),
On 2017/03/17 17:14:35, James Cook wrote:
> Do we have high-dpi user images? If so, this might break them.
> 
> If we do we could ship both scale factors as separate fields in the mojom, at
> least until we have proper support for high dpi, see crbug.com/667566

The bundled avatar images support hidpi. Filed http://crbug.com/702689 to track
the work.

https://codereview.chromium.org/2734933004/diff/100001/ash/common/system/user...
File ash/common/system/user/tray_user_unittest.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/common/system/user...
ash/common/system/user/tray_user_unittest.cc:245: // Switch to a new user
"Second@tray" - which has a capitalized name.
On 2017/03/17 17:14:35, James Cook wrote:
> For stuff like this could we provide a setter in TestSessionStateClient that
> lets you reset the name of a user? Or
> SessionController::SetUserNameForTesting()? Then the
mailto:"Capitalized@example.com"
> could be part of the test code.

Make sense. Moved the predefined users from TestSessionControllerClient into
this test since they seems to be only relevant to this test.

https://codereview.chromium.org/2734933004/diff/100001/ash/common/system/user...
File ash/common/system/user/user_card_view.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/common/system/user...
ash/common/system/user/user_card_view.cc:67:
image_view->SetImage(gfx::ImageSkia::CreateFrom1xBitmap(
On 2017/03/17 17:14:35, James Cook wrote:
> ditto re high dpi

Acknowledged.

https://codereview.chromium.org/2734933004/diff/100001/ash/common/system/user...
File ash/common/system/user/user_view.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/common/system/user...
ash/common/system/user/user_view.cc:57: DCHECK(user_index > 0);
On 2017/03/17 17:14:35, James Cook wrote:
> nit: Also DCHECK less than session count

Done. And added comment of why 0 is excluded.

https://codereview.chromium.org/2734933004/diff/100001/ash/common/test/test_s...
File ash/common/test/test_session_controller_client.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/common/test/test_s...
ash/common/test/test_session_controller_client.cc:25: "First@tray",
On 2017/03/17 17:14:36, James Cook wrote:
> Yuck. Do these have to match particular tests in ash/chrome browser? If so it
> might be nice to fix those tests in a follow-up CL to use the user%d pattern.

Moved into tray_user_unittests.cc when following your other comments.

https://codereview.chromium.org/2734933004/diff/100001/ash/common/test/test_s...
ash/common/test/test_session_controller_client.cc:44:
session_info_(mojom::SessionInfo::New()) {}
On 2017/03/17 17:14:36, James Cook wrote:
> nit: DCHECK(controller_)

Done.

https://codereview.chromium.org/2734933004/diff/100001/ash/common/test/test_s...
ash/common/test/test_session_controller_client.cc:58: void
TestSessionControllerClient::Reset() {
On 2017/03/17 17:14:36, James Cook wrote:
> Should the constructor call this?
> 
> Alternately, should this just replace session_info_ with a
> mojom::SessionInfo::New()?

Made constructor to call this and also moved mojom::SessionInfo::New() into this
function.

https://codereview.chromium.org/2734933004/diff/100001/ash/common/test/test_s...
ash/common/test/test_session_controller_client.cc:83: // TODO(xiyuan): Remove
after LoginStatus becomes part of SessionController.
On 2017/03/17 17:14:35, James Cook wrote:
> Is this still necessary after my CL?

Still needs this for cash. Your CL covers the mash. I will have a follow up CL
to consolidate the code paths then we can remove this.

https://codereview.chromium.org/2734933004/diff/100001/ash/common/test/test_s...
ash/common/test/test_session_controller_client.cc:141: for (auto& user_session :
controller_->GetUserSessions()) {
On 2017/03/17 17:14:35, James Cook wrote:
> const?

Done.

https://codereview.chromium.org/2734933004/diff/100001/ash/common/test/test_s...
ash/common/test/test_session_controller_client.cc:181: auto it =
std::find_if(sessions.begin(), sessions.end(),
On 2017/03/17 17:14:36, James Cook wrote:
> +1 for using find_if(). I'm starting to like small lambdas.

:)

https://codereview.chromium.org/2734933004/diff/100001/ash/common/test/test_s...
File ash/common/test/test_session_controller_client.h (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/common/test/test_s...
ash/common/test/test_session_controller_client.h:26: // NOT use this.
On 2017/03/17 17:14:36, James Cook wrote:
> nit: Maybe add to comment an example of that kind of test. (Like, can tests in
> chrome use this?)
> 
> I might also say something about how it simulates chrome's behavior (just to
> emphasize this exists to break an ash/chrome dependency to allow testing ash
> code in isolation).

Done.

https://codereview.chromium.org/2734933004/diff/100001/ash/common/test/test_s...
ash/common/test/test_session_controller_client.h:43: // Sets the |count|
pre-defined user sessions. The first four user session
On 2017/03/17 17:14:36, James Cook wrote:
> super nit: "Creates |count| pre-defined user sessions."

Done.

https://codereview.chromium.org/2734933004/diff/100001/ash/common/test/test_s...
ash/common/test/test_session_controller_client.h:48: void
SetPredefinedUserSessions(int count);
On 2017/03/17 17:14:36, James Cook wrote:
> optional: Maybe CreatePredefinedUserSessions()?

Done.

https://codereview.chromium.org/2734933004/diff/100001/ash/common/wm/workspac...
File ash/common/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/common/wm/workspac...
ash/common/wm/workspace/workspace_layout_manager_unittest.cc:901:
std::unique_ptr<test::TestSessionControllerClient> session_controller_client =
On 2017/03/17 17:14:36, James Cook wrote:
> On 2017/03/17 07:08:15, xiyuan wrote:
> > Creating a new TestSessionControllerClient because
> > WorkspaceLayoutManagerSoloTest is based on AshTest, which hides the
> > AshTestBase/AshTestHelper as implementation details. I could not figure out
a
> > way to get the TestSessionControllerClient in AshTestHelper. :(
> 
> Could this test be switched to use AshTestBase? We don't care about the
AshTest
> vs. AshTestBase distinction any more. (This could be in a follow-up CL.)

That works in both ash_unittests and mash_unittests. I will create a separate CL
to switch to AshTestBase.

https://codereview.chromium.org/2734933004/diff/100001/ash/common/wm_shell.cc
File ash/common/wm_shell.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/common/wm_shell.cc...
ash/common/wm_shell.cc:97: // Must occur after SessionStateDelegate creation and
user login.
On 2017/03/17 17:14:36, James Cook wrote:
> Is this comment still correct?

Removed. SessionStateDelegate is no longer relevant.

https://codereview.chromium.org/2734933004/diff/100001/ash/mus/shell_delegate...
File ash/mus/shell_delegate_mus.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/mus/shell_delegate...
ash/mus/shell_delegate_mus.cc:27: class SessionStateDelegateStub : public
SessionStateDelegate {
On 2017/03/17 17:14:36, James Cook wrote:
> We probably don't need two SessionStateDelegateStub implementations (here and
in
> wm_shell_mus.cc). It would be good to remove one of them. Can you file a bug
for
> that? I'm happy to help with that if you're busy.

Filed http://crbug.com/702809 to track.

After we figure out where to put the multi-profile avatar code, we probably can
get rid of them together with SessionStateDelegate.

https://codereview.chromium.org/2734933004/diff/100001/ash/shelf/shelf_layout...
File ash/shelf/shelf_layout_manager_unittest.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/shelf/shelf_layout...
ash/shelf/shelf_layout_manager_unittest.cc:1553: std::unique_ptr<aura::Window>
w1(CreateTestWindow());
On 2017/03/17 17:14:36, James Cook wrote:
> super nit: how about |window| instead of |w1|?

Done.

https://codereview.chromium.org/2734933004/diff/100001/ash/shell/example_sess...
File ash/shell/example_session_controller_client.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/shell/example_sess...
ash/shell/example_session_controller_client.cc:23: DCHECK_EQ(instance, nullptr);
On 2017/03/17 17:14:36, James Cook wrote:
> nit: Also DCHECK(controller)

Done.

https://codereview.chromium.org/2734933004/diff/100001/ash/shell/shell_delega...
File ash/shell/shell_delegate_impl.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/shell/shell_delega...
ash/shell/shell_delegate_impl.cc:62: SessionStateDelegateImpl() : user_info_(new
user_manager::UserInfoImpl()) {}
On 2017/03/17 17:14:36, James Cook wrote:
> This could also use a shared SessionStateDelegateStub

Acknowledged.

https://codereview.chromium.org/2734933004/diff/100001/ash/system/chromeos/po...
File ash/system/chromeos/power/tablet_power_button_controller_unittest.cc
(right):

https://codereview.chromium.org/2734933004/diff/100001/ash/system/chromeos/po...
ash/system/chromeos/power/tablet_power_button_controller_unittest.cc:114:
RunAllPendingInMessageLoop();
On 2017/03/17 17:14:36, James Cook wrote:
> This might be a good candidate for SessionController::FlushMojoForTesting()

Done.

https://codereview.chromium.org/2734933004/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/lock/screen_locker.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/lock/screen_locker.cc:486:
base::Bind(&ScreenLocker::ScheduleDeletion);
On 2017/03/17 17:14:36, James Cook wrote:
> nit: maybe inline this below?

We need to fix OnLockScreenHide's signature in order to do that. Somehow, it
uses a non-const reference there.

https://codereview.chromium.org/2734933004/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/session/chrome_session_manager.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/session/chrome_session_manager.cc:142:
SessionControllerClient::FlushForTesting();
On 2017/03/17 17:14:36, James Cook wrote:
> Hrm. Do we have to do this?
> 
> I remember we added this line before when I was working on asynchronous shelf
> creation. It seems like this makes the tests much different than production.
> 
> Frankly I think it might be better to delete/disable that one focus test
instead
> of changing all tests to have a synchronous flush during startup.
> 
> What do you think?

It is not just one focus test. There are about 18 browser tests + 17
chromevox_tests [1]. Esp. the failed chromevox ones, they have something like
this.awaitDocumentFocused(...) to run their test code. Without this, browser
window is not properly focused on creation.

The problem is the race between browser window creation and session state in
SessionController. If SessionController does not have ACTIVE state, ash would
think the session is blocked and the focus rule will not activate any window.

Another possible work around is to make SessionController default to ACTIVE
instead of UNKNOWN. This way, the browser window will get focus on creation.
WDYT?

[1]:
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
[2]:
https://cs.chromium.org/chromium/src/chrome/browser/resources/chromeos/chrome...

https://codereview.chromium.org/2734933004/diff/100001/chrome/browser/chromeo...
File
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_browsertest.cc
(right):

https://codereview.chromium.org/2734933004/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager_browsertest.cc:121:
SessionControllerClient::FlushForTesting();
On 2017/03/17 17:14:37, James Cook wrote:
> On 2017/03/17 07:08:16, xiyuan wrote:
> > This would cause shelf to be created now.
> 
> Acknowledged.
> 
> Aside, not related to this CL: I've often had trouble with these wallpaper
tests
> while working on mustash. I wonder if there's something fundamentally wrong
with
> how they are initializing things.

I have not looked at how wallpaper is migrated to mash. But I would not be
surprised if there is more work needed there.

[commit-bot: I haz the power, 2017-03-17 22:52:29]

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

[commit-bot: I haz the power, 2017-03-17 22:52:30]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[James Cook, 2017-03-17 22:57:06]

FYI - I'm not going to get to this before the end of the day.  I'll get to it as
soon as I can.

One question: Is this safe to land without support for high dpi avatar icons?
Won't that cause a regression?

[xiyuan, 2017-03-17 23:01:14]

On 2017/03/17 22:57:06, James Cook wrote:
> FYI - I'm not going to get to this before the end of the day.  I'll get to it
as
> soon as I can.
> 
> One question: Is this safe to land without support for high dpi avatar icons?
> Won't that cause a regression?

Take your time. I am not in a hurry to get it landed.

Good point about the high dpi icons. I will create another CL and land it
separately, following your idea of also passing a scale factor. I will pass the
image of current display scale factor and observer display change to update
them.

[James Cook, 2017-03-18 18:40:04]

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

[James Cook, 2017-03-18 18:40:06]

rockot, can you answer the question about FlushForTesting in
https://codereview.chromium.org/2734933004/diff/100001/chrome/browser/chromeo...
?  No need to review anything.

Everything else looks good. Feel free to rebase.

https://codereview.chromium.org/2734933004/diff/100001/ash/common/test/test_s...
File ash/common/test/test_session_controller_client.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/common/test/test_s...
ash/common/test/test_session_controller_client.cc:83: // TODO(xiyuan): Remove
after LoginStatus becomes part of SessionController.
On 2017/03/17 22:52:02, xiyuan wrote:
> On 2017/03/17 17:14:35, James Cook wrote:
> > Is this still necessary after my CL?
> 
> Still needs this for cash. Your CL covers the mash. I will have a follow up CL
> to consolidate the code paths then we can remove this.

Acknowledged.

https://codereview.chromium.org/2734933004/diff/100001/ash/common/wm/workspac...
File ash/common/wm/workspace/workspace_layout_manager_unittest.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/ash/common/wm/workspac...
ash/common/wm/workspace/workspace_layout_manager_unittest.cc:901:
std::unique_ptr<test::TestSessionControllerClient> session_controller_client =
On 2017/03/17 22:52:02, xiyuan wrote:
> On 2017/03/17 17:14:36, James Cook wrote:
> > On 2017/03/17 07:08:15, xiyuan wrote:
> > > Creating a new TestSessionControllerClient because
> > > WorkspaceLayoutManagerSoloTest is based on AshTest, which hides the
> > > AshTestBase/AshTestHelper as implementation details. I could not figure
out
> a
> > > way to get the TestSessionControllerClient in AshTestHelper. :(
> > 
> > Could this test be switched to use AshTestBase? We don't care about the
> AshTest
> > vs. AshTestBase distinction any more. (This could be in a follow-up CL.)
> 
> That works in both ash_unittests and mash_unittests. I will create a separate
CL
> to switch to AshTestBase.

Sounds good. I'm pretty sure we run tests based on AshTestBase in mash_unittests
now.

https://codereview.chromium.org/2734933004/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/session/chrome_session_manager.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/session/chrome_session_manager.cc:85: // Restores
user sessions for a crash-and-restarted chrome.
This also handles --login-user for tests, right? It would be helpful for me if
the comment explained that, and/or the function was renamed. (Doesn't have to be
in this CL, but I've been confused many times about this code path.)

https://codereview.chromium.org/2734933004/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/session/chrome_session_manager.cc:142:
SessionControllerClient::FlushForTesting();
On 2017/03/17 22:52:03, xiyuan wrote:
> On 2017/03/17 17:14:36, James Cook wrote:
> > Hrm. Do we have to do this?
> > 
> > I remember we added this line before when I was working on asynchronous
shelf
> > creation. It seems like this makes the tests much different than production.
> > 
> > Frankly I think it might be better to delete/disable that one focus test
> instead
> > of changing all tests to have a synchronous flush during startup.
> > 
> > What do you think?
> 
> It is not just one focus test. There are about 18 browser tests + 17
> chromevox_tests [1]. Esp. the failed chromevox ones, they have something like
> this.awaitDocumentFocused(...) to run their test code. Without this, browser
> window is not properly focused on creation.
> 
> The problem is the race between browser window creation and session state in
> SessionController. If SessionController does not have ACTIVE state, ash would
> think the session is blocked and the focus rule will not activate any window.
> 
> Another possible work around is to make SessionController default to ACTIVE
> instead of UNKNOWN. This way, the browser window will get focus on creation.
> WDYT?
> 
> [1]:
>
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
> [2]:
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/chromeos/chrome...

Could we do something like have ash look for the --login-user command line flag
and in that case default the session to ACTIVE?

Also, I would be concerned about this if the *only* messages it flushes are the
ones on the session controller interface. Then I would have more confidence
about test coverage in other cases. But I don't know if that's how it works.
rockot, in practice does binding.FlushForTest() end up flushing all IPCs?

[xiyuan, 2017-03-19 08:44:15]

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

[commit-bot: I haz the power, 2017-03-19 08:44:23]

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

[xiyuan, 2017-03-19 08:48:33]

https://codereview.chromium.org/2734933004/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/session/chrome_session_manager.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/session/chrome_session_manager.cc:85: // Restores
user sessions for a crash-and-restarted chrome.
On 2017/03/18 18:40:06, James Cook wrote:
> This also handles --login-user for tests, right? It would be helpful for me if
> the comment explained that, and/or the function was renamed. (Doesn't have to
be
> in this CL, but I've been confused many times about this code path.)

This probably should be called for StartUserSession to be more exact. It starts
chrome with a user session. This includes both crash-n-restore and tests on dev
box. Tests on dev box runs chromeos-chrome without any switches (to be
compatible with running tests for other platforms). They run this code path
because they run with a stub user account [1], with StartStubLoginSession
injects the stub account info.

[1]:
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/chrome_browser_m...

https://codereview.chromium.org/2734933004/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/session/chrome_session_manager.cc:142:
SessionControllerClient::FlushForTesting();
On 2017/03/18 18:40:06, James Cook wrote:
> On 2017/03/17 22:52:03, xiyuan wrote:
> > On 2017/03/17 17:14:36, James Cook wrote:
> > > Hrm. Do we have to do this?
> > > 
> > > I remember we added this line before when I was working on asynchronous
> shelf
> > > creation. It seems like this makes the tests much different than
production.
> > > 
> > > Frankly I think it might be better to delete/disable that one focus test
> > instead
> > > of changing all tests to have a synchronous flush during startup.
> > > 
> > > What do you think?
> > 
> > It is not just one focus test. There are about 18 browser tests + 17
> > chromevox_tests [1]. Esp. the failed chromevox ones, they have something
like
> > this.awaitDocumentFocused(...) to run their test code. Without this, browser
> > window is not properly focused on creation.
> > 
> > The problem is the race between browser window creation and session state in
> > SessionController. If SessionController does not have ACTIVE state, ash
would
> > think the session is blocked and the focus rule will not activate any
window.
> > 
> > Another possible work around is to make SessionController default to ACTIVE
> > instead of UNKNOWN. This way, the browser window will get focus on creation.
> > WDYT?
> > 
> > [1]:
> >
>
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
> > [2]:
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/resources/chromeos/chrome...
> 
> Could we do something like have ash look for the --login-user command line
flag
> and in that case default the session to ACTIVE?
> 

Yep, sounds good.

> Also, I would be concerned about this if the *only* messages it flushes are
the
> ones on the session controller interface. Then I would have more confidence
> about test coverage in other cases. But I don't know if that's how it works.
> rockot, in practice does binding.FlushForTest() end up flushing all IPCs?

Not expert, but think your concern is valid that this would make test code and
production code runs very differently. FlushForTest would run nested loop until
it hears back from the other side.

[commit-bot: I haz the power, 2017-03-19 09:57:22]

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

[commit-bot: I haz the power, 2017-03-19 09:57:23]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[James Cook, 2017-03-19 19:02:52]

Almost there! Sorry I didn't catch these before. This has been a challenging CL
to review.

Also, I don't think we need to wait on rockot@, since you took out that
FlushForTesting().

https://codereview.chromium.org/2734933004/diff/160001/ash/common/session/ses...
File ash/common/session/session_controller.cc (right):

https://codereview.chromium.org/2734933004/diff/160001/ash/common/session/ses...
ash/common/session/session_controller.cc:22: session_manager::SessionState
GetDefaultSessionState() {
This function needs comments explaining why the default is different.

https://codereview.chromium.org/2734933004/diff/160001/ash/common/test/test_s...
File ash/common/test/test_session_controller_client.cc (right):

https://codereview.chromium.org/2734933004/diff/160001/ash/common/test/test_s...
ash/common/test/test_session_controller_client.cc:87: void
TestSessionControllerClient::CreatePredefinedUserSessions(int count) {
Sorry I didn't catch this before, but it seems weird that you can call this with
count == 0 (like from AshTestBase).

I think it would be better if this did DCHECK_GT(count, 0) and test code that
needs an active session state with no users should call Reset() then
SetSessionState(ACTIVE). Hopefully we don't have too many tests like that, since
it's a weird state.

https://codereview.chromium.org/2734933004/diff/160001/ash/common/test/test_s...
File ash/common/test/test_session_state_delegate.cc (right):

https://codereview.chromium.org/2734933004/diff/160001/ash/common/test/test_s...
ash/common/test/test_session_state_delegate.cc:17: #include
"testing/gtest/include/gtest/gtest.h"
nit: most of these headers are not needed

https://codereview.chromium.org/2734933004/diff/160001/ash/common/test/test_s...
File ash/common/test/test_session_state_delegate.h (right):

https://codereview.chromium.org/2734933004/diff/160001/ash/common/test/test_s...
ash/common/test/test_session_state_delegate.h:9: #include <vector>
nit: not needed

https://codereview.chromium.org/2734933004/diff/160001/ash/common/test/test_s...
ash/common/test/test_session_state_delegate.h:12: #include
"base/compiler_specific.h"
nit: not needed

https://codereview.chromium.org/2734933004/diff/160001/ash/test/ash_test_base.cc
File ash/test/ash_test_base.cc (right):

https://codereview.chromium.org/2734933004/diff/160001/ash/test/ash_test_base...
ash/test/ash_test_base.cc:314: session_started ? 1 : 0);
It seems weird you call this with 0 users. See comment in
TestSessionControllerClient.

https://codereview.chromium.org/2734933004/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/session/chrome_session_manager.cc (right):

https://codereview.chromium.org/2734933004/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/login/session/chrome_session_manager.cc:29: #include
"chrome/browser/ui/ash/session_controller_client.h"
nit: not needed

https://codereview.chromium.org/2734933004/diff/160001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/session_state_delegate_chromeos.h (right):

https://codereview.chromium.org/2734933004/diff/160001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/session_state_delegate_chromeos.h:9: #include
"base/compiler_specific.h"
nit: not needed

[Ken Rockot(use gerrit already), 2017-03-20 15:59:26]

https://codereview.chromium.org/2734933004/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/session/chrome_session_manager.cc (right):

https://codereview.chromium.org/2734933004/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/login/session/chrome_session_manager.cc:142:
SessionControllerClient::FlushForTesting();
On 2017/03/19 at 08:48:33, xiyuan wrote:
> On 2017/03/18 18:40:06, James Cook wrote:
> > On 2017/03/17 22:52:03, xiyuan wrote:
> > > On 2017/03/17 17:14:36, James Cook wrote:
> > > > Hrm. Do we have to do this?
> > > > 
> > > > I remember we added this line before when I was working on asynchronous
> > shelf
> > > > creation. It seems like this makes the tests much different than
production.
> > > > 
> > > > Frankly I think it might be better to delete/disable that one focus test
> > > instead
> > > > of changing all tests to have a synchronous flush during startup.
> > > > 
> > > > What do you think?
> > > 
> > > It is not just one focus test. There are about 18 browser tests + 17
> > > chromevox_tests [1]. Esp. the failed chromevox ones, they have something
like
> > > this.awaitDocumentFocused(...) to run their test code. Without this,
browser
> > > window is not properly focused on creation.
> > > 
> > > The problem is the race between browser window creation and session state
in
> > > SessionController. If SessionController does not have ACTIVE state, ash
would
> > > think the session is blocked and the focus rule will not activate any
window.
> > > 
> > > Another possible work around is to make SessionController default to
ACTIVE
> > > instead of UNKNOWN. This way, the browser window will get focus on
creation.
> > > WDYT?
> > > 
> > > [1]:
> > >
> >
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
> > > [2]:
> > >
> >
https://cs.chromium.org/chromium/src/chrome/browser/resources/chromeos/chrome...
> > 
> > Could we do something like have ash look for the --login-user command line
flag
> > and in that case default the session to ACTIVE?
> > 
> 
> Yep, sounds good.
> 
> > Also, I would be concerned about this if the *only* messages it flushes are
the
> > ones on the session controller interface. Then I would have more confidence
> > about test coverage in other cases. But I don't know if that's how it works.
> > rockot, in practice does binding.FlushForTest() end up flushing all IPCs?
> 
> Not expert, but think your concern is valid that this would make test code and
production code runs very differently. FlushForTest would run nested loop until
it hears back from the other side.

Sounds like my answer doesn't matter anymore, but just in case: FlushForTesting
simply pings a single interface and runs the message loop on the calling thread
until it gets a ping response. So yes, other messages may or may not be
dispatched on that interface or other interfaces in the interim.

The intent of the API is not to allow tests to force synchronous behavior in
certain situations, but only to allow tests to verify that e.g. a certain
message or event has -not- been received as a result of some particular testing
condition.

[xiyuan, 2017-03-20 17:12:56]

https://codereview.chromium.org/2734933004/diff/160001/ash/common/session/ses...
File ash/common/session/session_controller.cc (right):

https://codereview.chromium.org/2734933004/diff/160001/ash/common/session/ses...
ash/common/session/session_controller.cc:22: session_manager::SessionState
GetDefaultSessionState() {
On 2017/03/19 19:02:51, James Cook wrote:
> This function needs comments explaining why the default is different.

Done.

https://codereview.chromium.org/2734933004/diff/160001/ash/common/test/test_s...
File ash/common/test/test_session_controller_client.cc (right):

https://codereview.chromium.org/2734933004/diff/160001/ash/common/test/test_s...
ash/common/test/test_session_controller_client.cc:87: void
TestSessionControllerClient::CreatePredefinedUserSessions(int count) {
On 2017/03/19 19:02:51, James Cook wrote:
> Sorry I didn't catch this before, but it seems weird that you can call this
with
> count == 0 (like from AshTestBase).
> 
> I think it would be better if this did DCHECK_GT(count, 0) and test code that
> needs an active session state with no users should call Reset() then
> SetSessionState(ACTIVE). Hopefully we don't have too many tests like that,
since
> it's a weird state.

Done.

We don't have any weird tests that have session started but no actual user
sessions. And SessionController makes it impossible because it does not have a
separate session_started flag and uses the # of user sessions for that.

https://codereview.chromium.org/2734933004/diff/160001/ash/common/test/test_s...
File ash/common/test/test_session_state_delegate.cc (right):

https://codereview.chromium.org/2734933004/diff/160001/ash/common/test/test_s...
ash/common/test/test_session_state_delegate.cc:17: #include
"testing/gtest/include/gtest/gtest.h"
On 2017/03/19 19:02:51, James Cook wrote:
> nit: most of these headers are not needed

Right. We can remove all of those. Done.

https://codereview.chromium.org/2734933004/diff/160001/ash/common/test/test_s...
File ash/common/test/test_session_state_delegate.h (right):

https://codereview.chromium.org/2734933004/diff/160001/ash/common/test/test_s...
ash/common/test/test_session_state_delegate.h:9: #include <vector>
On 2017/03/19 19:02:51, James Cook wrote:
> nit: not needed

Done.

https://codereview.chromium.org/2734933004/diff/160001/ash/common/test/test_s...
ash/common/test/test_session_state_delegate.h:12: #include
"base/compiler_specific.h"
On 2017/03/19 19:02:51, James Cook wrote:
> nit: not needed

Done.

https://codereview.chromium.org/2734933004/diff/160001/ash/test/ash_test_base.cc
File ash/test/ash_test_base.cc (right):

https://codereview.chromium.org/2734933004/diff/160001/ash/test/ash_test_base...
ash/test/ash_test_base.cc:314: session_started ? 1 : 0);
On 2017/03/19 19:02:51, James Cook wrote:
> It seems weird you call this with 0 users. See comment in
> TestSessionControllerClient.

Changed as suggested.

https://codereview.chromium.org/2734933004/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/login/session/chrome_session_manager.cc (right):

https://codereview.chromium.org/2734933004/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/login/session/chrome_session_manager.cc:29: #include
"chrome/browser/ui/ash/session_controller_client.h"
On 2017/03/19 19:02:51, James Cook wrote:
> nit: not needed

Done.

https://codereview.chromium.org/2734933004/diff/160001/chrome/browser/ui/ash/...
File chrome/browser/ui/ash/session_state_delegate_chromeos.h (right):

https://codereview.chromium.org/2734933004/diff/160001/chrome/browser/ui/ash/...
chrome/browser/ui/ash/session_state_delegate_chromeos.h:9: #include
"base/compiler_specific.h"
On 2017/03/19 19:02:51, James Cook wrote:
> nit: not needed

Done.

[xiyuan, 2017-03-20 17:12:59]

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

[commit-bot: I haz the power, 2017-03-20 17:13:27]

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

[James Cook, 2017-03-20 17:47:27]

LGTM. Great CL!

[commit-bot: I haz the power, 2017-03-20 18:31:37]

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

[commit-bot: I haz the power, 2017-03-20 18:31:38]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2017-03-20 19:03:02]

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

[xiyuan, 2017-03-20 19:03:03]

sky@, could you do owner review for:

    c/test/base/view_event_test_platform_part_chromeos.cc
    ui/aura/mus/window_tree_client.cc

?

Thanks.

[sky, 2017-03-20 20:12:59]

LGTM

[xiyuan, 2017-03-20 20:18:37]

The CQ bit was checked by xiyuan@chromium.org

[xiyuan, 2017-03-20 20:18:37]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2734933004/#ps200001
(title: "rebase to get WorkspaceLayoutManagerSoloTest change")

[commit-bot: I haz the power, 2017-03-20 20:19:30]

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-03-20 20:28:19]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1490041117270760,
"parent_rev": "fc7f323f548c926b99fe6ebd15f46a514f0df198", "commit_rev":
"f1ff14d7d25d3c37239e6804a3bfd6928c9cd1ef"}

[commit-bot: I haz the power, 2017-03-20 20:29:09]

Description was changed from

==========
ash: Use SessionController instead of SessionStateDelegate

- Switch SessionStateDelegate callers to SessionController;
- Remove all migrated methods from SessionStateDelegate;
- Update tests;
- Update ash_shell_with_content;
- Changed ScreenLocker to set SessionState::ACTIVE before destroying
  LockWindow so that focus is properly restored after lock;
- Changed UserSessionManager to set SessionState::ACTIVE before
  creating browser window so that the created window receives focus;

BUG=648964,699234
==========

to

==========
ash: Use SessionController instead of SessionStateDelegate

- Switch SessionStateDelegate callers to SessionController;
- Remove all migrated methods from SessionStateDelegate;
- Update tests;
- Update ash_shell_with_content;
- Changed ScreenLocker to set SessionState::ACTIVE before destroying
  LockWindow so that focus is properly restored after lock;
- Changed UserSessionManager to set SessionState::ACTIVE before
  creating browser window so that the created window receives focus;

BUG=648964,699234

Review-Url: https://codereview.chromium.org/2734933004
Cr-Commit-Position: refs/heads/master@{#458168}
Committed:
https://chromium.googlesource.com/chromium/src/+/f1ff14d7d25d3c37239e6804a3bf...
==========

[commit-bot: I haz the power, 2017-03-20 20:29:11]

Committed patchset #10 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/f1ff14d7d25d3c37239e6804a3bf...