[Refactor] Use ScopedTestPublicSessionLoginState in code

[Refactor] Use ScopedTestPublicSessionLoginState in code

BUG=718078
Review-Url: https://codereview.chromium.org/2856053004
Cr-Commit-Position: refs/heads/master@{#471259}
Committed: https://chromium.googlesource.com/chromium/src/+/3a0258864f393c913a30cdd2665d56f213395b1c

[Ivan Šandrk, 2017-05-04 17:01:31]

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

[commit-bot: I haz the power, 2017-05-04 17:02:17]

Dry run: CQ is trying da patch.

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

[Ivan Šandrk, 2017-05-04 17:57:00]

isandrk@chromium.org changed reviewers:
+ alemate@chromium.org, rdevlin.cronin@chromium.org

[Ivan Šandrk, 2017-05-04 17:57:00]

Devlin, ptal!

Alexander, ptal at chromeos/login!

[commit-bot: I haz the power, 2017-05-04 18:00:30]

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

[commit-bot: I haz the power, 2017-05-04 18:00:31]

Dry run: This issue passed the CQ dry run.

[Devlin, 2017-05-04 18:03:18]

lgtm

https://codereview.chromium.org/2856053004/diff/1/chromeos/login/scoped_test_...
File chromeos/login/scoped_test_public_session_login_state.cc (right):

https://codereview.chromium.org/2856053004/diff/1/chromeos/login/scoped_test_...
chromeos/login/scoped_test_public_session_login_state.cc:15: needs_shutdown_ =
true;
It might be worth noting (in the class comments in the .h file) that this is now
designed only to nest, rather than to be serially constructed/destructed.  For
instance, consider the two patterns of calls:

State a;  // construct
State b;  // construct
~State b;  // destruct
~State a;  // destruct

This works fine, because `b` didn't initialize state and won't tear it down. 
This is also most common, since it's what's handled by stack-allocated members.

a = new State;  // construct
b = new State;  // construct
delete a;  // destruct
// Weird State
delete b;  // destruct

This is a problem, because `a` will perform the shutdown, even though `b` is
still active.

I don't think it's worth the complexity to address this, but it warrants a
comment. :)

[Ivan Šandrk, 2017-05-04 18:15:09]

On 2017/05/04 18:03:18, Devlin wrote:
> lgtm
> 
>
https://codereview.chromium.org/2856053004/diff/1/chromeos/login/scoped_test_...
> File chromeos/login/scoped_test_public_session_login_state.cc (right):
> 
>
https://codereview.chromium.org/2856053004/diff/1/chromeos/login/scoped_test_...
> chromeos/login/scoped_test_public_session_login_state.cc:15: needs_shutdown_ =
> true;
> It might be worth noting (in the class comments in the .h file) that this is
now
> designed only to nest, rather than to be serially constructed/destructed.  For
> instance, consider the two patterns of calls:
> 
> State a;  // construct
> State b;  // construct
> ~State b;  // destruct
> ~State a;  // destruct
> 
> This works fine, because `b` didn't initialize state and won't tear it down. 
> This is also most common, since it's what's handled by stack-allocated
members.
> 
> a = new State;  // construct
> b = new State;  // construct
> delete a;  // destruct
> // Weird State
> delete b;  // destruct
> 
> This is a problem, because `a` will perform the shutdown, even though `b` is
> still active.
> 
> I don't think it's worth the complexity to address this, but it warrants a
> comment. :)

Hm. What if I allow only one of those to exist an any given time? Have a static
member var that tracks if any instances exist, and then add a CHECK in the code.

[Devlin, 2017-05-04 18:16:54]

On 2017/05/04 18:15:09, Ivan Šandrk wrote:
> On 2017/05/04 18:03:18, Devlin wrote:
> > lgtm
> > 
> >
>
https://codereview.chromium.org/2856053004/diff/1/chromeos/login/scoped_test_...
> > File chromeos/login/scoped_test_public_session_login_state.cc (right):
> > 
> >
>
https://codereview.chromium.org/2856053004/diff/1/chromeos/login/scoped_test_...
> > chromeos/login/scoped_test_public_session_login_state.cc:15: needs_shutdown_
=
> > true;
> > It might be worth noting (in the class comments in the .h file) that this is
> now
> > designed only to nest, rather than to be serially constructed/destructed. 
For
> > instance, consider the two patterns of calls:
> > 
> > State a;  // construct
> > State b;  // construct
> > ~State b;  // destruct
> > ~State a;  // destruct
> > 
> > This works fine, because `b` didn't initialize state and won't tear it down.

> > This is also most common, since it's what's handled by stack-allocated
> members.
> > 
> > a = new State;  // construct
> > b = new State;  // construct
> > delete a;  // destruct
> > // Weird State
> > delete b;  // destruct
> > 
> > This is a problem, because `a` will perform the shutdown, even though `b` is
> > still active.
> > 
> > I don't think it's worth the complexity to address this, but it warrants a
> > comment. :)
> 
> Hm. What if I allow only one of those to exist an any given time? Have a
static
> member var that tracks if any instances exist, and then add a CHECK in the
code.

That could work, too, as long as there are no cases where we'd want to allow
nesting.  Then you don't need the shutdown_ bool at all.

Either way is fine with me.

[Ivan Šandrk, 2017-05-04 18:21:32]

On 2017/05/04 18:16:54, Devlin wrote:
> On 2017/05/04 18:15:09, Ivan Šandrk wrote:
> > On 2017/05/04 18:03:18, Devlin wrote:
> > > lgtm
> > > 
> > >
> >
>
https://codereview.chromium.org/2856053004/diff/1/chromeos/login/scoped_test_...
> > > File chromeos/login/scoped_test_public_session_login_state.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2856053004/diff/1/chromeos/login/scoped_test_...
> > > chromeos/login/scoped_test_public_session_login_state.cc:15:
needs_shutdown_
> =
> > > true;
> > > It might be worth noting (in the class comments in the .h file) that this
is
> > now
> > > designed only to nest, rather than to be serially constructed/destructed. 
> For
> > > instance, consider the two patterns of calls:
> > > 
> > > State a;  // construct
> > > State b;  // construct
> > > ~State b;  // destruct
> > > ~State a;  // destruct
> > > 
> > > This works fine, because `b` didn't initialize state and won't tear it
down.
> 
> > > This is also most common, since it's what's handled by stack-allocated
> > members.
> > > 
> > > a = new State;  // construct
> > > b = new State;  // construct
> > > delete a;  // destruct
> > > // Weird State
> > > delete b;  // destruct
> > > 
> > > This is a problem, because `a` will perform the shutdown, even though `b`
is
> > > still active.
> > > 
> > > I don't think it's worth the complexity to address this, but it warrants a
> > > comment. :)
> > 
> > Hm. What if I allow only one of those to exist an any given time? Have a
> static
> > member var that tracks if any instances exist, and then add a CHECK in the
> code.
> 
> That could work, too, as long as there are no cases where we'd want to allow
> nesting.  Then you don't need the shutdown_ bool at all.
> 
> Either way is fine with me.

I think I still need the shutdown_ bool - in some cases LoginState is already
initialized, and in some it's not. I'd like to cover both cases with this class.
Bottom line I could add a constructor parameter for needs_initialization, but I
don't see a good reason for it :)

[Ivan Šandrk, 2017-05-04 18:25:54]

On 2017/05/04 18:21:32, Ivan Šandrk wrote:
> On 2017/05/04 18:16:54, Devlin wrote:
> > On 2017/05/04 18:15:09, Ivan Šandrk wrote:
> > > On 2017/05/04 18:03:18, Devlin wrote:
> > > > lgtm
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2856053004/diff/1/chromeos/login/scoped_test_...
> > > > File chromeos/login/scoped_test_public_session_login_state.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2856053004/diff/1/chromeos/login/scoped_test_...
> > > > chromeos/login/scoped_test_public_session_login_state.cc:15:
> needs_shutdown_
> > =
> > > > true;
> > > > It might be worth noting (in the class comments in the .h file) that
this
> is
> > > now
> > > > designed only to nest, rather than to be serially
constructed/destructed. 
> > For
> > > > instance, consider the two patterns of calls:
> > > > 
> > > > State a;  // construct
> > > > State b;  // construct
> > > > ~State b;  // destruct
> > > > ~State a;  // destruct
> > > > 
> > > > This works fine, because `b` didn't initialize state and won't tear it
> down.
> > 
> > > > This is also most common, since it's what's handled by stack-allocated
> > > members.
> > > > 
> > > > a = new State;  // construct
> > > > b = new State;  // construct
> > > > delete a;  // destruct
> > > > // Weird State
> > > > delete b;  // destruct
> > > > 
> > > > This is a problem, because `a` will perform the shutdown, even though
`b`
> is
> > > > still active.
> > > > 
> > > > I don't think it's worth the complexity to address this, but it warrants
a
> > > > comment. :)
> > > 
> > > Hm. What if I allow only one of those to exist an any given time? Have a
> > static
> > > member var that tracks if any instances exist, and then add a CHECK in the
> > code.
> > 
> > That could work, too, as long as there are no cases where we'd want to allow
> > nesting.  Then you don't need the shutdown_ bool at all.
> > 
> > Either way is fine with me.
> 
> I think I still need the shutdown_ bool - in some cases LoginState is already
> initialized, and in some it's not. I'd like to cover both cases with this
class.
> Bottom line I could add a constructor parameter for needs_initialization, but
I
> don't see a good reason for it :)

P.S. I also want to keep the previous state of LoginState (so if I had to
initialize it, shutdown it in the cleanup) :)

[Devlin, 2017-05-04 18:53:34]

On 2017/05/04 18:25:54, Ivan Šandrk wrote:
> On 2017/05/04 18:21:32, Ivan Šandrk wrote:
> > On 2017/05/04 18:16:54, Devlin wrote:
> > > On 2017/05/04 18:15:09, Ivan Šandrk wrote:
> > > > On 2017/05/04 18:03:18, Devlin wrote:
> > > > > lgtm
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2856053004/diff/1/chromeos/login/scoped_test_...
> > > > > File chromeos/login/scoped_test_public_session_login_state.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2856053004/diff/1/chromeos/login/scoped_test_...
> > > > > chromeos/login/scoped_test_public_session_login_state.cc:15:
> > needs_shutdown_
> > > =
> > > > > true;
> > > > > It might be worth noting (in the class comments in the .h file) that
> this
> > is
> > > > now
> > > > > designed only to nest, rather than to be serially
> constructed/destructed. 
> > > For
> > > > > instance, consider the two patterns of calls:
> > > > > 
> > > > > State a;  // construct
> > > > > State b;  // construct
> > > > > ~State b;  // destruct
> > > > > ~State a;  // destruct
> > > > > 
> > > > > This works fine, because `b` didn't initialize state and won't tear it
> > down.
> > > 
> > > > > This is also most common, since it's what's handled by stack-allocated
> > > > members.
> > > > > 
> > > > > a = new State;  // construct
> > > > > b = new State;  // construct
> > > > > delete a;  // destruct
> > > > > // Weird State
> > > > > delete b;  // destruct
> > > > > 
> > > > > This is a problem, because `a` will perform the shutdown, even though
> `b`
> > is
> > > > > still active.
> > > > > 
> > > > > I don't think it's worth the complexity to address this, but it
warrants
> a
> > > > > comment. :)
> > > > 
> > > > Hm. What if I allow only one of those to exist an any given time? Have a
> > > static
> > > > member var that tracks if any instances exist, and then add a CHECK in
the
> > > code.
> > > 
> > > That could work, too, as long as there are no cases where we'd want to
allow
> > > nesting.  Then you don't need the shutdown_ bool at all.
> > > 
> > > Either way is fine with me.
> > 
> > I think I still need the shutdown_ bool - in some cases LoginState is
already
> > initialized, and in some it's not. I'd like to cover both cases with this
> class.
> > Bottom line I could add a constructor parameter for needs_initialization,
but
> I
> > don't see a good reason for it :)
> 
> P.S. I also want to keep the previous state of LoginState (so if I had to
> initialize it, shutdown it in the cleanup) :)

Ah, didn't realize things other than this object would be initializing the
state.  Makes sense.

[Ivan Šandrk, 2017-05-05 11:12:49]

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

[commit-bot: I haz the power, 2017-05-05 11:12:56]

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-05-05 11:55:15]

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

[commit-bot: I haz the power, 2017-05-05 11:55:16]

Dry run: This issue passed the CQ dry run.

[Ivan Šandrk, 2017-05-05 18:13:09]

Alexander, friendly ping!

[Ivan Šandrk, 2017-05-11 14:09:10]

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

[commit-bot: I haz the power, 2017-05-11 14:09:45]

Dry run: CQ is trying da patch.

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

[Ivan Šandrk, 2017-05-11 14:10:02]

Patchset #3 (id:40001) has been deleted

[Ivan Šandrk, 2017-05-11 14:10:40]

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

[commit-bot: I haz the power, 2017-05-11 14:11:05]

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-05-11 14:15:07]

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

[commit-bot: I haz the power, 2017-05-11 14:15:08]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Ivan Šandrk, 2017-05-11 15:02:34]

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

[commit-bot: I haz the power, 2017-05-11 15:03:32]

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-05-11 15:45:58]

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

[commit-bot: I haz the power, 2017-05-11 15:45:59]

Dry run: This issue passed the CQ dry run.

[Alexander Alekseev, 2017-05-11 20:00:59]

lgtm

[Ivan Šandrk, 2017-05-12 09:33:02]

The CQ bit was checked by isandrk@chromium.org

[Ivan Šandrk, 2017-05-12 09:33:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2856053004/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2017-05-12 09:33:19]

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-05-12 09:37:55]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494581582881230,
"parent_rev": "6cf9c20af607acbd32f39af217756d9c39a9e12e", "commit_rev":
"3a0258864f393c913a30cdd2665d56f213395b1c"}

[commit-bot: I haz the power, 2017-05-12 09:38:06]

Description was changed from

==========
[Refactor] Use ScopedTestPublicSessionLoginState in code

BUG=718078
==========

to

==========
[Refactor] Use ScopedTestPublicSessionLoginState in code

BUG=718078

Review-Url: https://codereview.chromium.org/2856053004
Cr-Commit-Position: refs/heads/master@{#471259}
Committed:
https://chromium.googlesource.com/chromium/src/+/3a0258864f393c913a30cdd2665d...
==========

[commit-bot: I haz the power, 2017-05-12 09:38:11]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/3a0258864f393c913a30cdd2665d...