Make login cancel logic load localaccount file before attempting offline login

Make login cancel logic load localaccount file before attempting offline login

BUG=chromium-os:5169
TEST=unit tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=53913

[Paweł Hajdan Jr., 2010-07-28 01:12:24]

Drive-by with a test comment.

http://codereview.chromium.org/2847080/diff/5001/6004
File chrome/common/net/gaia/gaia_authenticator2_unittest.h (right):

http://codereview.chromium.org/2847080/diff/5001/6004#newcode74
chrome/common/net/gaia/gaia_authenticator2_unittest.h:74:
sleeper_->DelayedCompleteFetch(success_, url_, delegate(), 500);
Why is this necessary? It seems like it's going to make tests run slower.

[Chris Masone, 2010-07-28 01:15:20]

http://codereview.chromium.org/2847080/diff/5001/6004
File chrome/common/net/gaia/gaia_authenticator2_unittest.h (right):

http://codereview.chromium.org/2847080/diff/5001/6004#newcode74
chrome/common/net/gaia/gaia_authenticator2_unittest.h:74:
sleeper_->DelayedCompleteFetch(success_, url_, delegate(), 500);
On 2010/07/28 01:12:24, Paweł Hajdan Jr. wrote:
> Why is this necessary? It seems like it's going to make tests run slower.

To simulate network delays.

[Paweł Hajdan Jr., 2010-07-28 01:16:55]

http://codereview.chromium.org/2847080/diff/5001/6004
File chrome/common/net/gaia/gaia_authenticator2_unittest.h (right):

http://codereview.chromium.org/2847080/diff/5001/6004#newcode74
chrome/common/net/gaia/gaia_authenticator2_unittest.h:74:
sleeper_->DelayedCompleteFetch(success_, url_, delegate(), 500);
On 2010/07/28 01:15:20, cmasone wrote:
> On 2010/07/28 01:12:24, Paweł Hajdan Jr. wrote:
> > Why is this necessary? It seems like it's going to make tests run slower.
> 
> To simulate network delays.

Does it take a different code path then? Can the delay be smaller, like 100ms?

[Chris Masone, 2010-07-28 01:18:31]

On Tue, Jul 27, 2010 at 6:16 PM,  <phajdan.jr@chromium.org> wrote:
>
> http://codereview.chromium.org/2847080/diff/5001/6004
> File chrome/common/net/gaia/gaia_authenticator2_unittest.h (right):
>
> http://codereview.chromium.org/2847080/diff/5001/6004#newcode74
> chrome/common/net/gaia/gaia_authenticator2_unittest.h:74:
> sleeper_->DelayedCompleteFetch(success_, url_, delegate(), 500);
> On 2010/07/28 01:15:20, cmasone wrote:
>>
>> On 2010/07/28 01:12:24, Paweł Hajdan Jr. wrote:
>> > Why is this necessary? It seems like it's going to make tests run
>
> slower.
>
>> To simulate network delays.
>
> Does it take a different code path then? Can the delay be smaller, like
> 100ms?

Yes to both

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

[Chris Masone, 2010-07-28 03:29:06]

PTAL :-)

On 2010/07/28 01:18:31, cmasone wrote:
> On Tue, Jul 27, 2010 at 6:16 PM,  <mailto:phajdan.jr@chromium.org> wrote:
> >
> > http://codereview.chromium.org/2847080/diff/5001/6004
> > File chrome/common/net/gaia/gaia_authenticator2_unittest.h (right):
> >
> > http://codereview.chromium.org/2847080/diff/5001/6004#newcode74
> > chrome/common/net/gaia/gaia_authenticator2_unittest.h:74:
> > sleeper_->DelayedCompleteFetch(success_, url_, delegate(), 500);
> > On 2010/07/28 01:15:20, cmasone wrote:
> >>
> >> On 2010/07/28 01:12:24, Paweł Hajdan Jr. wrote:
> >> > Why is this necessary? It seems like it's going to make tests run
> >
> > slower.
> >
> >> To simulate network delays.
> >
> > Does it take a different code path then? Can the delay be smaller, like
> > 100ms?
> 
> Yes to both
> 
> >
> > http://codereview.chromium.org/2847080/show
> >
>

[DaveMoore, 2010-07-28 04:44:33]

It's a bit convoluted but after an IM discussion I think it makes sense. LGTM
modulo nits and some addition comments describing the testing approach.

http://codereview.chromium.org/2847080/diff/11001/12002
File chrome/browser/chromeos/login/google_authenticator_unittest.cc (right):

http://codereview.chromium.org/2847080/diff/11001/12002#newcode58
chrome/browser/chromeos/login/google_authenticator_unittest.cc:58: const GURL&
url,
nit: indenting

http://codereview.chromium.org/2847080/diff/11001/12002#newcode65
chrome/browser/chromeos/login/google_authenticator_unittest.cc:65: }
nit: nl

http://codereview.chromium.org/2847080/diff/11001/12002#newcode101
chrome/browser/chromeos/login/google_authenticator_unittest.cc:101: }
nit: nl

http://codereview.chromium.org/2847080/diff/11001/12002#newcode550
chrome/browser/chromeos/login/google_authenticator_unittest.cc:550: }
nit: nl

http://codereview.chromium.org/2847080/diff/11001/12002#newcode556
chrome/browser/chromeos/login/google_authenticator_unittest.cc:556: }
nit: nl

http://codereview.chromium.org/2847080/diff/11001/12002#newcode560
chrome/browser/chromeos/login/google_authenticator_unittest.cc:560: }
nit: nl

[Chris Masone, 2010-07-28 05:55:52]

http://codereview.chromium.org/2847080/diff/11001/12002
File chrome/browser/chromeos/login/google_authenticator_unittest.cc (right):

http://codereview.chromium.org/2847080/diff/11001/12002#newcode58
chrome/browser/chromeos/login/google_authenticator_unittest.cc:58: const GURL&
url,
On 2010/07/28 04:44:33, davemoore wrote:
> nit: indenting

Done.

http://codereview.chromium.org/2847080/diff/11001/12002#newcode65
chrome/browser/chromeos/login/google_authenticator_unittest.cc:65: }
On 2010/07/28 04:44:33, davemoore wrote:
> nit: nl

Done.

http://codereview.chromium.org/2847080/diff/11001/12002#newcode101
chrome/browser/chromeos/login/google_authenticator_unittest.cc:101: }
On 2010/07/28 04:44:33, davemoore wrote:
> nit: nl

Done.

http://codereview.chromium.org/2847080/diff/11001/12002#newcode550
chrome/browser/chromeos/login/google_authenticator_unittest.cc:550: }
On 2010/07/28 04:44:33, davemoore wrote:
> nit: nl

Done.

http://codereview.chromium.org/2847080/diff/11001/12002#newcode556
chrome/browser/chromeos/login/google_authenticator_unittest.cc:556: }
On 2010/07/28 04:44:33, davemoore wrote:
> nit: nl

Done.

http://codereview.chromium.org/2847080/diff/11001/12002#newcode560
chrome/browser/chromeos/login/google_authenticator_unittest.cc:560: }
On 2010/07/28 04:44:33, davemoore wrote:
> nit: nl

Done.

[oshima, 2010-07-28 08:00:10]

thank you for fix, and sorry for jumping in late.
just nits and you may address them next time.

http://codereview.chromium.org/2847080/diff/11001/12002
File chrome/browser/chromeos/login/google_authenticator_unittest.cc (right):

http://codereview.chromium.org/2847080/diff/11001/12002#newcode66
chrome/browser/chromeos/login/google_authenticator_unittest.cc:66:
~TimedMockFetcher() {
virtual (just for style)

http://codereview.chromium.org/2847080/diff/11001/12002#newcode102
chrome/browser/chromeos/login/google_authenticator_unittest.cc:102:
CancelableTask* task_;
DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/2847080/diff/11001/12002#newcode564
chrome/browser/chromeos/login/google_authenticator_unittest.cc:564: }
better to move these static functions to anonymous namespace.

http://codereview.chromium.org/2847080/diff/11001/12002#newcode642
chrome/browser/chromeos/login/google_authenticator_unittest.cc:642: MessageLoop
message_loop(MessageLoop::TYPE_UI);
just for my understanding. what's the difference?

[Chris Masone, 2010-07-28 15:25:29]

working on a slightly bigger change that cleans up more of this stuff to make it
simpler.  Will send along to oshima this AM.

http://codereview.chromium.org/2847080/diff/11001/12002
File chrome/browser/chromeos/login/google_authenticator_unittest.cc (right):

http://codereview.chromium.org/2847080/diff/11001/12002#newcode642
chrome/browser/chromeos/login/google_authenticator_unittest.cc:642: MessageLoop
message_loop(MessageLoop::TYPE_UI);
On 2010/07/28 08:00:10, oshima wrote:
> just for my understanding. what's the difference?

You can't call Run() on a MessageLoopForUI, only RunAllPending().