Cleanup PolicyOAuth2TokenFetcher method signatures.

chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc

Index: chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc
diff --git a/chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc b/chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc
index ba802715ca00e0e65f785ae4101fd1967e647a8a..8e437b0af876ecf1032d8dcf3f52b0675e233a3e 100644
--- a/chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc
+++ b/chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.cc
@@ -31,34 +31,45 @@ const int kRequestRestartDelay = 3000;
 
 }  // namespace
 
-PolicyOAuth2TokenFetcher::PolicyOAuth2TokenFetcher(
+PolicyOAuth2TokenFetcher::PolicyOAuth2TokenFetcher() {
+}
+
+PolicyOAuth2TokenFetcher::~PolicyOAuth2TokenFetcher() {
+}
+
+void PolicyOAuth2TokenFetcher::StartWithSigninContext(
     net::URLRequestContextGetter* auth_context_getter,
     net::URLRequestContextGetter* system_context_getter,
-    const TokenCallback& callback)
-    : auth_context_getter_(auth_context_getter),
-      system_context_getter_(system_context_getter),
-      callback_(callback) {}
+    const TokenCallback& callback) {
+  DCHECK(!refresh_token_fetcher_ && !access_token_fetcher_);

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

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.

[achuithb, 2015/04/29 07:30:05]

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.

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

Clearing state would obviously reset weak pointer as well, so I don't see how
this would create any bugs.
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]

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? 
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]

That's why Reset() would call InvalidateWeakPtrs() (after converting this class
to use a regular WeakPtrFactory as everyone else does).
 
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.
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.

 
-PolicyOAuth2TokenFetcher::PolicyOAuth2TokenFetcher(
-    const std::string& auth_code,
-    net::URLRequestContextGetter* system_context_getter,
-    const TokenCallback& callback)
-    : auth_code_(auth_code),
-      system_context_getter_(system_context_getter),
-      callback_(callback) {
+  auth_context_getter_ = auth_context_getter;
+  system_context_getter_ = system_context_getter;
+  callback_ = callback;
+  StartFetchingRefreshToken();
 }
 
-PolicyOAuth2TokenFetcher::~PolicyOAuth2TokenFetcher() {}
+void PolicyOAuth2TokenFetcher::StartWithAuthCode(
+    const std::string& auth_code,
+    net::URLRequestContextGetter* system_context_getter,
+    const TokenCallback& callback) {
+  DCHECK(!refresh_token_fetcher_ && !access_token_fetcher_);
 
-void PolicyOAuth2TokenFetcher::Start() {
-  retry_count_ = 0;
+  auth_code_ = auth_code;
+  system_context_getter_ = system_context_getter;
+  callback_ = callback;
   StartFetchingRefreshToken();
 }
 
 void PolicyOAuth2TokenFetcher::StartWithRefreshToken(
-    const std::string& oauth2_refresh_token) {
-  retry_count_ = 0;
+    const std::string& oauth2_refresh_token,
+    net::URLRequestContextGetter* system_context_getter,
+    const TokenCallback& callback) {
+  DCHECK(!refresh_token_fetcher_ && !access_token_fetcher_);
+
   oauth2_refresh_token_ = oauth2_refresh_token;
+  system_context_getter_ = system_context_getter;
+  callback_ = callback;
   StartFetchingAccessToken();
 }