Cleanup PolicyOAuth2TokenFetcher method signatures.

Cleanup PolicyOAuth2TokenFetcher method signatures.

* Replace ctors of PolicyOAuth2TokenFetcher and Start(), with StartWithSigninContext, and StartWithAuthCode.
* Replace WildcardLoginChecker::Start with WildcardLoginChecker::StartWithSigninContext for consistency.

BUG=470984
TEST=compiles

Committed: https://crrev.com/20ec2f062ff3cf451cef9433f0c27a51ea070032
Cr-Commit-Position: refs/heads/master@{#327458}

[achuithb, 2015-04-28 22:14:27]

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

[achuithb, 2015-04-28 22:14:28]

This is the cleanup I promised to Mattias in the other CL.

Please take a look, Mattias and Xiyuan.

[achuithb, 2015-04-28 22:20:46]

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

[commit-bot: I haz the power, 2015-04-28 22:21:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1109133004/20001

[xiyuan, 2015-04-28 22:23:01]

LGTM

https://codereview.chromium.org/1109133004/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc (right):

https://codereview.chromium.org/1109133004/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc:43: const
TokenCallback& callback) {
nit: Let's DCHECK(!refresh_token_fetcher_ && !access_token_fetcher_) to catch
overlapping StartXXX calls, here and the other two StartXXX.

[commit-bot: I haz the power, 2015-04-29 00:29:32]

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

[commit-bot: I haz the power, 2015-04-29 00:29:32]

Dry run: This issue passed the CQ dry run.

[achuithb, 2015-04-29 03:17:10]

Thank you! Will wait for Mattias

https://codereview.chromium.org/1109133004/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc (right):

https://codereview.chromium.org/1109133004/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc:43: const
TokenCallback& callback) {
On 2015/04/28 22:23:01, xiyuan wrote:
> nit: Let's DCHECK(!refresh_token_fetcher_ && !access_token_fetcher_) to catch
> overlapping StartXXX calls, here and the other two StartXXX.

Done.

[Mattias Nissler (ping if slow), 2015-04-29 07:18:41]

https://codereview.chromium.org/1109133004/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc (right):

https://codereview.chromium.org/1109133004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc:44:
DCHECK(!refresh_token_fetcher_ && !access_token_fetcher_);
Maybe a better solution would be to make Reset() helper that we invoke here to
clear out all state? That way, starting over by calling one of the Start
functions again would be totally safe.

[Mattias Nissler (ping if slow), 2015-04-29 07:19:17]

Oh, LGTM also for the current version of the CL, the comment I put is merely a
suggestion.

[achuithb, 2015-04-29 07:30:06]

Thank you for the review and suggestion, Mattias!

https://codereview.chromium.org/1109133004/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc (right):

https://codereview.chromium.org/1109133004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc:44:
DCHECK(!refresh_token_fetcher_ && !access_token_fetcher_);
On 2015/04/29 07:18:41, Mattias Nissler wrote:
> Maybe a better solution would be to make Reset() helper that we invoke here to
> clear out all state? That way, starting over by calling one of the Start
> functions again would be totally safe.

I'm not sure how safe it is to try to clear state - there are some
PostDelayedTask and other asynchronous calls that call back into this class. I
worry that this could be a source of arcane bugs.

I think we would prefer to discourage people from trying to re-use an instance
of this class - there's no cost in creating a new instance. So once StartWithXXX
is called, I think it's best to forbid people from calling another StartWithXXX,
and this is also consistent with current usage.

[achuithb, 2015-04-29 07:30:13]

The CQ bit was checked by achuith@chromium.org

[achuithb, 2015-04-29 07:30:14]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/1109133004/#ps40001
(title: "Add safety DCHECKs")

[commit-bot: I haz the power, 2015-04-29 07:31:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1109133004/40001

[Mattias Nissler (ping if slow), 2015-04-29 07:35:17]

https://codereview.chromium.org/1109133004/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc (right):

https://codereview.chromium.org/1109133004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc:44:
DCHECK(!refresh_token_fetcher_ && !access_token_fetcher_);
On 2015/04/29 07:30:05, achuithb wrote:
> On 2015/04/29 07:18:41, Mattias Nissler wrote:
> > Maybe a better solution would be to make Reset() helper that we invoke here
to
> > clear out all state? That way, starting over by calling one of the Start
> > functions again would be totally safe.
> 
> I'm not sure how safe it is to try to clear state - there are some
> PostDelayedTask and other asynchronous calls that call back into this class. I
> worry that this could be a source of arcane bugs.

Clearing state would obviously reset weak pointer as well, so I don't see how
this would create any bugs.

> 
> I think we would prefer to discourage people from trying to re-use an instance
> of this class - there's no cost in creating a new instance. So once
StartWithXXX
> is called, I think it's best to forbid people from calling another
StartWithXXX,
> and this is also consistent with current usage.

If you want to really prevent people from calling Start again, you should
probably upgrade your DCHECKs to CHECKs. DCHECKs have a long history of being
essentially worthless on OS_CHROMEOS (or has that changed?).

[achuithb, 2015-04-29 08:28:37]

https://codereview.chromium.org/1109133004/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc (right):

https://codereview.chromium.org/1109133004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc:44:
DCHECK(!refresh_token_fetcher_ && !access_token_fetcher_);
On 2015/04/29 07:35:17, Mattias Nissler wrote:
> On 2015/04/29 07:30:05, achuithb wrote:
> > On 2015/04/29 07:18:41, Mattias Nissler wrote:
> > > Maybe a better solution would be to make Reset() helper that we invoke
here
> to
> > > clear out all state? That way, starting over by calling one of the Start
> > > functions again would be totally safe.
> > 
> > I'm not sure how safe it is to try to clear state - there are some
> > PostDelayedTask and other asynchronous calls that call back into this class.
I
> > worry that this could be a source of arcane bugs.
> 
> Clearing state would obviously reset weak pointer as well, so I don't see how
> this would create any bugs.

I'm not 100% clear on this, but the current object exists after a Reset. If
there's a closure that's bound to the current object, the weak ptr is not
invalidated, and it could still be called into this object, right? 

> > 
> > I think we would prefer to discourage people from trying to re-use an
instance
> > of this class - there's no cost in creating a new instance. So once
> StartWithXXX
> > is called, I think it's best to forbid people from calling another
> StartWithXXX,
> > and this is also consistent with current usage.
> 
> If you want to really prevent people from calling Start again, you should
> probably upgrade your DCHECKs to CHECKs. DCHECKs have a long history of being
> essentially worthless on OS_CHROMEOS (or has that changed?).

We could probably make the same argument for most DCHECKs in cros code. This
code has unit test/browser test coverage which runs with DCHECKs enabled on the
chromiumos-chromium bots. I can confirm this since we had some recent bustage
with failing DCHECKs. I also believe all the current code paths with
StartWithXXX are exercised by tests, but obviously people could introduce new
calls that are untested.

I'm happy to create another CL to address your concerns if you have a strong
preference on what you'd like to see here (CHECK or Reset), since you are after
all the owner of this code. Let me know!

[Mattias Nissler (ping if slow), 2015-04-29 08:48:50]

https://codereview.chromium.org/1109133004/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc (right):

https://codereview.chromium.org/1109133004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc:44:
DCHECK(!refresh_token_fetcher_ && !access_token_fetcher_);
On 2015/04/29 08:28:37, achuithb wrote:
> On 2015/04/29 07:35:17, Mattias Nissler wrote:
> > On 2015/04/29 07:30:05, achuithb wrote:
> > > On 2015/04/29 07:18:41, Mattias Nissler wrote:
> > > > Maybe a better solution would be to make Reset() helper that we invoke
> here
> > to
> > > > clear out all state? That way, starting over by calling one of the Start
> > > > functions again would be totally safe.
> > > 
> > > I'm not sure how safe it is to try to clear state - there are some
> > > PostDelayedTask and other asynchronous calls that call back into this
class.
> I
> > > worry that this could be a source of arcane bugs.
> > 
> > Clearing state would obviously reset weak pointer as well, so I don't see
how
> > this would create any bugs.
> 
> I'm not 100% clear on this, but the current object exists after a Reset. If
> there's a closure that's bound to the current object, the weak ptr is not
> invalidated, and it could still be called into this object, right? 

That's why Reset() would call InvalidateWeakPtrs() (after converting this class
to use a regular WeakPtrFactory as everyone else does).
 
> > > 
> > > I think we would prefer to discourage people from trying to re-use an
> instance
> > > of this class - there's no cost in creating a new instance. So once
> > StartWithXXX
> > > is called, I think it's best to forbid people from calling another
> > StartWithXXX,
> > > and this is also consistent with current usage.
> > 
> > If you want to really prevent people from calling Start again, you should
> > probably upgrade your DCHECKs to CHECKs. DCHECKs have a long history of
being
> > essentially worthless on OS_CHROMEOS (or has that changed?).
> 
> We could probably make the same argument for most DCHECKs in cros code. This
> code has unit test/browser test coverage which runs with DCHECKs enabled on
the
> chromiumos-chromium bots. I can confirm this since we had some recent bustage
> with failing DCHECKs. I also believe all the current code paths with
> StartWithXXX are exercised by tests, but obviously people could introduce new
> calls that are untested.

My personal experience with running a Chrome OS Debug build is that you're
running into DCHECKs all of the place if you're only slightly leaving the
well-trodden path. If we have tests, that definitely improves the situation, but
if you want to strictly "forbid" reuse (as you said) then DCHECK is probably too
weak. If the DCHECK is merely documentation for the fact that you shouldn't
re-use, I think it's appropriate.

> 
> I'm happy to create another CL to address your concerns if you have a strong
> preference on what you'd like to see here (CHECK or Reset), since you are
after
> all the owner of this code. Let me know!

As I said, I don't feel too strong and I'm fine with the code as is. My comments
were just meant to add context I felt was missing in your reasoning. There's
always room for judgement calls by CL authors. If I hadn't been generally OK
with your code, I wouldn't have put my l-g-t-m.

[commit-bot: I haz the power, 2015-04-29 09:39:23]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-04-29 09:40:36]

Patchset 3 (id:??) landed as
https://crrev.com/20ec2f062ff3cf451cef9433f0c27a51ea070032
Cr-Commit-Position: refs/heads/master@{#327458}