ash: Use session_manager::SessionState

ash: Use session_manager::SessionState

Replace SessionStateDelegate::SessionState with
session_manager::SessionState to prepare for the mojo
interface replacement of SessionStateDelegate.

BUG=496761, 648964
TEST=All existing tests pass.

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

[xiyuan, 2016-10-14 21:43:15]

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

[commit-bot: I haz the power, 2016-10-14 21:43:46]

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, 2016-10-14 21:57:21]

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

[commit-bot: I haz the power, 2016-10-14 21:57:22]

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, 2016-10-14 22:27:56]

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

[commit-bot: I haz the power, 2016-10-14 22:28:20]

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, 2016-10-14 22:59:58]

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

[commit-bot: I haz the power, 2016-10-14 22:59:58]

Dry run: This issue passed the CQ dry run.

[xiyuan, 2016-10-14 23:18:24]

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

[xiyuan, 2016-10-14 23:18:25]

Move the SessionState enum out. There should be no functionality change.

PTAL. Thanks.

[James Cook, 2016-10-15 00:12:41]

LGTM

https://codereview.chromium.org/2416253004/diff/20001/ash/common/session/sess...
File ash/common/session/session_types.h (right):

https://codereview.chromium.org/2416253004/diff/20001/ash/common/session/sess...
ash/common/session/session_types.h:16: enum class SessionState {
Thanks for switching to enum class!

[xiyuan, 2016-10-17 16:53:35]

Description was changed from

==========
ash: Move SessionState into session_types.h

Move SessionState into session_type.h to prepare to use it in
the mojo interface replacement of SessionStateDelegate.

BUG=496761,648964
==========

to

==========
ash: Move SessionState into session_types.h

Move SessionState into session_type.h to prepare to use it in
the mojo interface replacement of SessionStateDelegate.

BUG=496761,648964
TEST=All existing tests pass.
==========

[xiyuan, 2016-10-17 16:53:42]

The CQ bit was checked by xiyuan@chromium.org

[commit-bot: I haz the power, 2016-10-17 16:54:07]

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-10-17 17:04:06]

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

[commit-bot: I haz the power, 2016-10-17 17:04:07]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[xiyuan, 2016-10-17 17:10:50]

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

[xiyuan, 2016-10-17 17:10:51]

+stevenjb

Could you help with owner's approval for:
  c/b/ui/ash/session_state_delegate_chromeos.cc|h
?

Thanks.

[stevenjb, 2016-10-17 17:47:37]

On 2016/10/17 17:10:51, xiyuan wrote:
> +stevenjb
> 
> Could you help with owner's approval for:
>   c/b/ui/ash/session_state_delegate_chromeos.cc|h
> ?
> 
> Thanks.

This seems fine for now, but see also this comment:
https://cs.chromium.org/chromium/src/components/session_manager/core/session_...

session_manager.h is now in src/components so we could use it in ash. (It was in
src/chrome when ash::SessionStateDelegate::SessionState was introduced).

That said, lgtm.

[xiyuan, 2016-10-17 18:23:10]

On 2016/10/17 17:47:37, stevenjb wrote:
> On 2016/10/17 17:10:51, xiyuan wrote:
> > +stevenjb
> > 
> > Could you help with owner's approval for:
> >   c/b/ui/ash/session_state_delegate_chromeos.cc|h
> > ?
> > 
> > Thanks.
> 
> This seems fine for now, but see also this comment:
>
https://cs.chromium.org/chromium/src/components/session_manager/core/session_...
> 
> session_manager.h is now in src/components so we could use it in ash. (It was
in
> src/chrome when ash::SessionStateDelegate::SessionState was introduced).
> 
> That said, lgtm.

Good call. Let me revise the CL to merge the two.

[xiyuan, 2016-10-17 21:37:08]

Description was changed from

==========
ash: Move SessionState into session_types.h

Move SessionState into session_type.h to prepare to use it in
the mojo interface replacement of SessionStateDelegate.

BUG=496761,648964
TEST=All existing tests pass.
==========

to

==========
ash: Replace SessionStateDelegate::SessionState

Replace SessionStateDelegate::SessionState with the
session_manager::SessionState to prepare to use it in
the mojo interface replacement of SessionStateDelegate.

BUG=496761,648964
TEST=All existing tests pass.
==========

[xiyuan, 2016-10-17 21:49:22]

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

[commit-bot: I haz the power, 2016-10-17 21:50:07]

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

[xiyuan, 2016-10-17 22:12:49]

Description was changed from

==========
ash: Replace SessionStateDelegate::SessionState

Replace SessionStateDelegate::SessionState with the
session_manager::SessionState to prepare to use it in
the mojo interface replacement of SessionStateDelegate.

BUG=496761,648964
TEST=All existing tests pass.
==========

to

==========
ash: Use session_manager::SessionState

Replace SessionStateDelegate::SessionState with session_manager::SessionState to
prepare for the mojo
interface replacement of SessionStateDelegate.

BUG=496761,648964
TEST=All existing tests pass.
==========

[xiyuan, 2016-10-17 22:13:00]

Description was changed from

==========
ash: Use session_manager::SessionState

Replace SessionStateDelegate::SessionState with session_manager::SessionState to
prepare for the mojo
interface replacement of SessionStateDelegate.

BUG=496761,648964
TEST=All existing tests pass.
==========

to

==========
ash: Use session_manager::SessionState

Replace SessionStateDelegate::SessionState with
session_manager::SessionState to prepare for the mojo
interface replacement of SessionStateDelegate.

BUG=496761,648964
TEST=All existing tests pass.
==========

[xiyuan, 2016-10-17 22:14:40]

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

[xiyuan, 2016-10-17 22:14:41]

PTAL. Thanks.

+achuith for components/session_manager and making ash depends on it.

[achuithb, 2016-10-17 22:27:22]

lgtm. Feel free to add yourself to OWNERS of components/session_manager

https://codereview.chromium.org/2416253004/diff/40001/components/session_mana...
File components/session_manager/session_manager_types.h (right):

https://codereview.chromium.org/2416253004/diff/40001/components/session_mana...
components/session_manager/session_manager_types.h:10: // TODO(nkostylev): Get
rid/consolidate with chromeos::LoggedInState.
Can probably get rid of nkostylev since he's not going to fix this.

[stevenjb, 2016-10-17 22:59:34]

Thanks for doing this! LGTM

https://codereview.chromium.org/2416253004/diff/40001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (right):

https://codereview.chromium.org/2416253004/diff/40001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:43:
chromeos::LoginState::Get()->AddObserver(this);
We might want to add a comment here that we are only setting SessionState to
ACTIVE or PRIMARY instead of using SessionManager::Get()->session_state()
directly, with a TODO that we should consider doing so. (I wouldn't try to make
that change in this CL though).

https://codereview.chromium.org/2416253004/diff/40001/components/session_mana...
File components/session_manager/session_manager_types.h (right):

https://codereview.chromium.org/2416253004/diff/40001/components/session_mana...
components/session_manager/session_manager_types.h:10: // TODO(nkostylev): Get
rid/consolidate with chromeos::LoggedInState.
On 2016/10/17 22:27:22, achuithb wrote:
> Can probably get rid of nkostylev since he's not going to fix this.

Achuith, are you volunteering? :) You can go ahead and pick me since I created
LoginState.

[xiyuan, 2016-10-17 23:17:28]

https://codereview.chromium.org/2416253004/diff/40001/chrome/browser/ui/ash/s...
File chrome/browser/ui/ash/session_state_delegate_chromeos.cc (right):

https://codereview.chromium.org/2416253004/diff/40001/chrome/browser/ui/ash/s...
chrome/browser/ui/ash/session_state_delegate_chromeos.cc:43:
chromeos::LoginState::Get()->AddObserver(this);
On 2016/10/17 22:59:34, stevenjb wrote:
> We might want to add a comment here that we are only setting SessionState to
> ACTIVE or PRIMARY instead of using SessionManager::Get()->session_state()
> directly, with a TODO that we should consider doing so. (I wouldn't try to
make
> that change in this CL though).

Added a comment. My goal is to replace SessionStateDelegate with a pair of mojo
interfaces (ash -> chrome and chrome -> ash). And the replacement would get real
state from SessionManager and have getters/helpers to translate the states for
ash.

Current plan for the replacement is here:
https://bugs.chromium.org/p/chromium/issues/detail?id=648964#c7

https://codereview.chromium.org/2416253004/diff/40001/components/session_mana...
File components/session_manager/session_manager_types.h (right):

https://codereview.chromium.org/2416253004/diff/40001/components/session_mana...
components/session_manager/session_manager_types.h:10: // TODO(nkostylev): Get
rid/consolidate with chromeos::LoggedInState.
On 2016/10/17 22:59:34, stevenjb wrote:
> On 2016/10/17 22:27:22, achuithb wrote:
> > Can probably get rid of nkostylev since he's not going to fix this.
> 
> Achuith, are you volunteering? :) You can go ahead and pick me since I created
> LoginState.

I put my name here for now and could get to it after the SessionStateDelegate
replacement work.

[James Cook, 2016-10-17 23:28:41]

still lgtm

[xiyuan, 2016-10-18 15:53:27]

The CQ bit was checked by xiyuan@chromium.org

[xiyuan, 2016-10-18 15:53:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, achuith@chromium.org
Link to the patchset: https://codereview.chromium.org/2416253004/#ps60001
(title: "add comment about session state in SessionStaetDelegateChromeos")

[commit-bot: I haz the power, 2016-10-18 15:53:54]

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-10-18 17:14:31]

Description was changed from

==========
ash: Use session_manager::SessionState

Replace SessionStateDelegate::SessionState with
session_manager::SessionState to prepare for the mojo
interface replacement of SessionStateDelegate.

BUG=496761,648964
TEST=All existing tests pass.
==========

to

==========
ash: Use session_manager::SessionState

Replace SessionStateDelegate::SessionState with
session_manager::SessionState to prepare for the mojo
interface replacement of SessionStateDelegate.

BUG=496761,648964
TEST=All existing tests pass.
==========

[commit-bot: I haz the power, 2016-10-18 17:14:33]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-18 17:18:04]

Description was changed from

==========
ash: Use session_manager::SessionState

Replace SessionStateDelegate::SessionState with
session_manager::SessionState to prepare for the mojo
interface replacement of SessionStateDelegate.

BUG=496761,648964
TEST=All existing tests pass.
==========

to

==========
ash: Use session_manager::SessionState

Replace SessionStateDelegate::SessionState with
session_manager::SessionState to prepare for the mojo
interface replacement of SessionStateDelegate.

BUG=496761,648964
TEST=All existing tests pass.

Committed: https://crrev.com/ba4b204b711dff1d75ed36725aa7c8d4c46e7146
Cr-Commit-Position: refs/heads/master@{#425996}
==========

[commit-bot: I haz the power, 2016-10-18 17:18:06]

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