[Chrome OS] Infrastucture for doing offline/online login simultaneously

[Chrome OS] Infrastucture for doing offline/online login simultaneously

These are meant to wrap and replace large chunks of code in GoogleAuthenticator,
once I rewrite that code to do offline and online login at the same time.
That's why there's some code that's still in GoogleAuthenticator that's
refactored into some of these classes.

Essentially, one creates an AuthAttemptState object, which will only allow
its mutable contents to be read/written on the IO thread.  Then, you pass it
to an OnlineAttempt which, when done, stores results in the AuthAttemptState.
You can do the same thing with the various CryptohomeOp classes.  They call out
over DBus to asynchronously do stuff with the user's cryptohome and then,
when the callbacks come in, these object post a call over to the IO thread to
update the contents of the AuthAttemptState instance they're holding on to.

BUG=chromium-os:4929
TEST=unit testss

[Paweł Hajdan Jr., 2010-09-20 11:56:04]

Drive-by with a test comment.

http://codereview.chromium.org/3407008/diff/1018/3015
File chrome/browser/chromeos/login/online_attempt_unittest.cc (right):

http://codereview.chromium.org/3407008/diff/1018/3015#newcode134
chrome/browser/chromeos/login/online_attempt_unittest.cc:134:
io_thread_.Start();
Can we always start the IO thread at the beginning of the test, and stop it at
the end? I think that doing so in random places is quite error prone.

[Chris Masone, 2010-09-20 14:40:42]

On Mon, Sep 20, 2010 at 4:56 AM, <phajdan.jr@chromium.org> wrote:

> Drive-by with a test comment.
>
>
> http://codereview.chromium.org/3407008/diff/1018/3015
> File chrome/browser/chromeos/login/online_attempt_unittest.cc (right):
>
> http://codereview.chromium.org/3407008/diff/1018/3015#newcode134
> chrome/browser/chromeos/login/online_attempt_unittest.cc:134:
> io_thread_.Start();
> Can we always start the IO thread at the beginning of the test, and stop
> it at the end? I think that doing so in random places is quite error
> prone.
>
>
>
 I need to ensure that all the tasks I've posted to the IO thread have
completed before checking the state they were supposed to modify, I don't
see how I could do this without stopping the IO thread at the appropriate
place in each test.  Given that, it seemed more clear to just handle IO
thread lifetime in completely in each test and Start/Stop it directly.


> http://codereview.chromium.org/3407008/show
>

[Paweł Hajdan Jr., 2010-09-20 14:58:23]

I'd prefer posting a task to the IO thread that will signal a WaitableEvent, and
then waiting on that event in places where you currently Stop the thread.

The problem here is that if the IO thread doesn't exist, all PostTasks to it are
going to fail. If the return codes of PostTask are not checked (usually they
aren't), the test may behave differently than the actual code in production.

[Chris Masone, 2010-09-20 15:07:27]

On Mon, Sep 20, 2010 at 7:58 AM, <phajdan.jr@chromium.org> wrote:

> I'd prefer posting a task to the IO thread that will signal a
> WaitableEvent, and
> then waiting on that event in places where you currently Stop the thread.
>
> The problem here is that if the IO thread doesn't exist, all PostTasks to
> it are
> going to fail. If the return codes of PostTask are not checked (usually
> they
> aren't), the test may behave differently than the actual code in
> production.


You're worried that the IO thread won't exist in production?  Because I
create the IO thread in the test fixture, and start it in every test.


>
>
> http://codereview.chromium.org/3407008/show
>

[Chris Masone, 2010-09-20 16:04:05]

Changed per convo on IRC.  PTAL

On 2010/09/20 15:07:27, Chris Masone wrote:
> On Mon, Sep 20, 2010 at 7:58 AM, <mailto:phajdan.jr@chromium.org> wrote:
> 
> > I'd prefer posting a task to the IO thread that will signal a
> > WaitableEvent, and
> > then waiting on that event in places where you currently Stop the thread.
> >
> > The problem here is that if the IO thread doesn't exist, all PostTasks to
> > it are
> > going to fail. If the return codes of PostTask are not checked (usually
> > they
> > aren't), the test may behave differently than the actual code in
> > production.
> 
> 
> You're worried that the IO thread won't exist in production?  Because I
> create the IO thread in the test fixture, and start it in every test.
> 
> 
> >
> >
> > http://codereview.chromium.org/3407008/show
> >
>

[Paweł Hajdan Jr., 2010-09-20 16:32:04]

Code I commented in the drive-by LGTM.

[Chris Masone, 2010-09-21 15:53:51]

On 2010/09/20 16:32:04, Paweł Hajdan Jr. wrote:
> Code I commented in the drive-by LGTM.

davemoore, ping?

[DaveMoore, 2010-09-21 18:42:40]

http://codereview.chromium.org/3407008/diff/9001/10003
File chrome/browser/chromeos/login/auth_attempt_state.h (right):

http://codereview.chromium.org/3407008/diff/9001/10003#newcode32
chrome/browser/chromeos/login/auth_attempt_state.h:32: void
RecordOnlineLoginStatus(
Please comment the public and protected methods.

http://codereview.chromium.org/3407008/diff/9001/10003#newcode67
chrome/browser/chromeos/login/auth_attempt_state.h:67: int offline_code_;
nit: nl

http://codereview.chromium.org/3407008/diff/9001/10003#newcode72
chrome/browser/chromeos/login/auth_attempt_state.h:72: class TestAttemptState :
public AuthAttemptState {
Move the public testing api to a separate file so it doesn't bloat release
builds.

http://codereview.chromium.org/3407008/diff/9001/10003#newcode86
chrome/browser/chromeos/login/auth_attempt_state.h:86: void
PresetOfflineLoginStatus(bool offline_outcome, int offline_code);
Doc

http://codereview.chromium.org/3407008/diff/9001/10004
File chrome/browser/chromeos/login/auth_attempt_state_resolver.h (right):

http://codereview.chromium.org/3407008/diff/9001/10004#newcode14
chrome/browser/chromeos/login/auth_attempt_state_resolver.h:14: virtual
~AuthAttemptStateResolver() {}
constructor / destructor implementation in headers can bloat subclasses. Better
to move them to the implementation files.

http://codereview.chromium.org/3407008/diff/9001/10004#newcode17
chrome/browser/chromeos/login/auth_attempt_state_resolver.h:17: virtual void
Resolve() = 0;
nit: nl

http://codereview.chromium.org/3407008/diff/9001/10006
File chrome/browser/chromeos/login/cryptohome_op.h (right):

http://codereview.chromium.org/3407008/diff/9001/10006#newcode12
chrome/browser/chromeos/login/cryptohome_op.h:12: #include "base/ref_counted.h"
nit: alphabetical order

http://codereview.chromium.org/3407008/diff/9001/10006#newcode31
chrome/browser/chromeos/login/cryptohome_op.h:31: virtual void OnComplete(bool
success, int return_code);
nit: nl

http://codereview.chromium.org/3407008/diff/9001/10006#newcode36
chrome/browser/chromeos/login/cryptohome_op.h:36: AuthAttemptStateResolver*
const resolver_;
nit: nl

http://codereview.chromium.org/3407008/diff/9001/10011
File chrome/browser/chromeos/login/mock_url_fetchers.cc (right):

http://codereview.chromium.org/3407008/diff/9001/10011#newcode35
chrome/browser/chromeos/login/mock_url_fetchers.cc:35: 100);
Will this work the same with just a PostTask, so there's less potential for
flakiness?

http://codereview.chromium.org/3407008/diff/9001/10014
File chrome/browser/chromeos/login/online_attempt.h (right):

http://codereview.chromium.org/3407008/diff/9001/10014#newcode34
chrome/browser/chromeos/login/online_attempt.h:34: void Initiate(Profile*
profile);
Doc

[Chris Masone, 2010-09-22 01:29:35]

Thanks, Dave.  PTAL

http://codereview.chromium.org/3407008/diff/9001/10003
File chrome/browser/chromeos/login/auth_attempt_state.h (right):

http://codereview.chromium.org/3407008/diff/9001/10003#newcode32
chrome/browser/chromeos/login/auth_attempt_state.h:32: void
RecordOnlineLoginStatus(
On 2010/09/21 18:42:40, davemoore wrote:
> Please comment the public and protected methods.

Done.

http://codereview.chromium.org/3407008/diff/9001/10003#newcode67
chrome/browser/chromeos/login/auth_attempt_state.h:67: int offline_code_;
On 2010/09/21 18:42:40, davemoore wrote:
> nit: nl

Done.

http://codereview.chromium.org/3407008/diff/9001/10003#newcode72
chrome/browser/chromeos/login/auth_attempt_state.h:72: class TestAttemptState :
public AuthAttemptState {
On 2010/09/21 18:42:40, davemoore wrote:
> Move the public testing api to a separate file so it doesn't bloat release
> builds.

Done.

http://codereview.chromium.org/3407008/diff/9001/10003#newcode86
chrome/browser/chromeos/login/auth_attempt_state.h:86: void
PresetOfflineLoginStatus(bool offline_outcome, int offline_code);
On 2010/09/21 18:42:40, davemoore wrote:
> Doc

Done.

http://codereview.chromium.org/3407008/diff/9001/10004
File chrome/browser/chromeos/login/auth_attempt_state_resolver.h (right):

http://codereview.chromium.org/3407008/diff/9001/10004#newcode14
chrome/browser/chromeos/login/auth_attempt_state_resolver.h:14: virtual
~AuthAttemptStateResolver() {}
On 2010/09/21 18:42:40, davemoore wrote:
> constructor / destructor implementation in headers can bloat subclasses.
Better
> to move them to the implementation files.

I thought that, if you were going to create an entire impl file just for the
constructor/destructor, that wasn't required.  Oh well.  Done

http://codereview.chromium.org/3407008/diff/9001/10004#newcode17
chrome/browser/chromeos/login/auth_attempt_state_resolver.h:17: virtual void
Resolve() = 0;
On 2010/09/21 18:42:40, davemoore wrote:
> nit: nl

Done.

http://codereview.chromium.org/3407008/diff/9001/10006
File chrome/browser/chromeos/login/cryptohome_op.h (right):

http://codereview.chromium.org/3407008/diff/9001/10006#newcode12
chrome/browser/chromeos/login/cryptohome_op.h:12: #include "base/ref_counted.h"
On 2010/09/21 18:42:40, davemoore wrote:
> nit: alphabetical order

Done.

http://codereview.chromium.org/3407008/diff/9001/10006#newcode31
chrome/browser/chromeos/login/cryptohome_op.h:31: virtual void OnComplete(bool
success, int return_code);
On 2010/09/21 18:42:40, davemoore wrote:
> nit: nl

Done.

http://codereview.chromium.org/3407008/diff/9001/10006#newcode36
chrome/browser/chromeos/login/cryptohome_op.h:36: AuthAttemptStateResolver*
const resolver_;
On 2010/09/21 18:42:40, davemoore wrote:
> nit: nl

Done.

http://codereview.chromium.org/3407008/diff/9001/10011
File chrome/browser/chromeos/login/mock_url_fetchers.cc (right):

http://codereview.chromium.org/3407008/diff/9001/10011#newcode35
chrome/browser/chromeos/login/mock_url_fetchers.cc:35: 100);
On 2010/09/21 18:42:40, davemoore wrote:
> Will this work the same with just a PostTask, so there's less potential for
> flakiness?

Sadly, it does not.  I can't get them to interleave correctly doing that :-/

The test completes -- but fails -- if this task is executed, so it's not going
to hang the bots.

http://codereview.chromium.org/3407008/diff/9001/10014
File chrome/browser/chromeos/login/online_attempt.h (right):

http://codereview.chromium.org/3407008/diff/9001/10014#newcode34
chrome/browser/chromeos/login/online_attempt.h:34: void Initiate(Profile*
profile);
On 2010/09/21 18:42:40, davemoore wrote:
> Doc

Done.

[DaveMoore, 2010-09-22 14:09:51]

LGTM