OAuth2 sign-in flow for ChromeOS

OAuth2 sign-in flow for ChromeOS.

Refactored all OAuth1/2 code out of LoginUtils into OAuthLoginManager class.

Created OAuth2-based specialization of OAuthLoginManager (behind --force-oauth2 switch). This new class uses OAuth2 refresh tokens as a base token from which all others are minted - incl. GAIA credentials and session cookies.

BUG=166192, 169999
TEST=existing unit, browser tests, additional manual testing
TBR=sky

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=176800

[xiyuan, 2013-01-07 23:07:55]

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/login_manager.cc (right):

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_manager.cc:3: // found in the LICENSE file.
nit: insert an empty line

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_manager.cc:74:
delegate_->OnOAuth2RefreshTokenFetchFailed(user_profile_,
token_details->service());
nit: 80 cols

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_manager.cc:98: LOG(WARNING) << "Fetched
refresh token!";
nit: VLOG(1) instead of LOG(WARNING)?

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_manager.cc:110: const std::string& sid,
const std::string& lsid, const std::string& auth) {
nit: put the args into their own lines.

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_manager.cc:119: LOG(WARNING) << "Got OAuth2
refresh token!";
nit: VLOG(1) instead of LOG(WARNING)?

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/login_manager.h (right):

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_manager.h:3: // found in the LICENSE file.
nit: insert an empty line

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_manager.h:23: class LoginManager : public
base::SupportsWeakPtr<LoginManager>,
It seems we never get a weak ptr of the LoginManager. So probably does not need
to SupportsWeakPtr here.

Also the name LoginManager is a bit confusing since it does not actually manage
login stuff. It is more like a TokenServiceObserver or something. Could we give
it a better name?

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_manager.h:30: // Reports when tokens are
loaded from the database.
nit: alignment

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_manager.h:63: // Checks GAIA error and
figures out of the request should be reattempted
nit: Checks GAIA error and figures out whether the request should be
reattempted.

And get rid of the empty comment below.

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_utils.cc:563: }
nuke it?

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_utils.cc:1217: std::string decoded_secret =
encoded_secret;
We probably should do this in CertLibrary::Decrypt/EncryptToken. Let's put a
TODO here.

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth2_login_verifier.cc (right):

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:42: Profile*
user_profile)
nit: 4-space indent for above three lines

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:173: retry_count_++ <
kMaxRequestAttemptCount) {
Get rid of '++' here since we are doing it in the next line. And fix alignment.

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth2_login_verifier.h (right):

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.h:11: #include
"base/callback.h"
callback.h is probably not needed. And you might need to add "base/bind.h" to cc
file after removing this.

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.h:25: // can be used for
other token fetching calls (i.e. TokenService).
Update the comments?

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.h:72: // retries |task|
after certain delY until max retry count is reached.
nit: delY -> delay

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.h:78: Profile*
auth_profile_;
|auth_profile_| seems not used.

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth2_token_fetcher.cc (right):

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_token_fetcher.cc:63:
auth_fetcher_.StartCookieForOAuthLoginTokenExchange("1");
Is it proper to use a hard-coded "1" as session index here?

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_token_fetcher.cc:108: retry_count_++ <
kMaxRequestAttemptCount) {
Get rid of '++' here and fix alignment.

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth2_token_fetcher.h (right):

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_token_fetcher.h:11: #include
"base/callback.h"
callback.h is probably not needed.

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_token_fetcher.h:23: // used to kick off
other token retrieval tasks (i.e. TokenService).
Update the comments here.

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/net/gaia/g...
File chrome/browser/net/gaia/gaia_oauth_fetcher.cc (right):

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/net/gaia/g...
chrome/browser/net/gaia/gaia_oauth_fetcher.cc:37: "service=%s";
kOAuth2LoginBodyFormat is not used anywhere. And I don't see where we set
|request_type_| to OAUTH2_LOGIN and OAUTH2_REVOKE_TOKEN either.

Is the snapshot up-to-date?

[zel, 2013-01-08 02:05:41]

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/login_manager.cc (right):

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_manager.cc:3: // found in the LICENSE file.
On 2013/01/07 23:07:55, xiyuan wrote:
> nit: insert an empty line

Done.

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_manager.cc:74:
delegate_->OnOAuth2RefreshTokenFetchFailed(user_profile_,
token_details->service());
On 2013/01/07 23:07:55, xiyuan wrote:
> nit: 80 cols

Done.

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_manager.cc:110: const std::string& sid,
const std::string& lsid, const std::string& auth) {
On 2013/01/07 23:07:55, xiyuan wrote:
> nit: put the args into their own lines.

Done.

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/login_manager.h (right):

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_manager.h:3: // found in the LICENSE file.
On 2013/01/07 23:07:55, xiyuan wrote:
> nit: insert an empty line

Done.

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_manager.h:23: class LoginManager : public
base::SupportsWeakPtr<LoginManager>,
On 2013/01/07 23:07:55, xiyuan wrote:
> It seems we never get a weak ptr of the LoginManager. So probably does not
need
> to SupportsWeakPtr here.
> 
> Also the name LoginManager is a bit confusing since it does not actually
manage
> login stuff. It is more like a TokenServiceObserver or something. Could we
give
> it a better name?

Done.

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_manager.h:23: class LoginManager : public
base::SupportsWeakPtr<LoginManager>,
On 2013/01/07 23:07:55, xiyuan wrote:
> It seems we never get a weak ptr of the LoginManager. So probably does not
need
> to SupportsWeakPtr here.
> 
> Also the name LoginManager is a bit confusing since it does not actually
manage
> login stuff. It is more like a TokenServiceObserver or something. Could we
give
> it a better name?

I've called it TokenServiceObserver originally, but the goal is to move all
OAuth related stuff here. I will most likely rename it to OAuthLoginManager

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_manager.h:30: // Reports when tokens are
loaded from the database.
On 2013/01/07 23:07:55, xiyuan wrote:
> nit: alignment

Done.

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_manager.h:63: // Checks GAIA error and
figures out of the request should be reattempted
On 2013/01/07 23:07:55, xiyuan wrote:
> nit: Checks GAIA error and figures out whether the request should be
> reattempted.
> 
> And get rid of the empty comment below.

Done.

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_utils.cc:563: }
On 2013/01/07 23:07:55, xiyuan wrote:
> nuke it?

Done.

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_utils.cc:1217: std::string decoded_secret =
encoded_secret;
On 2013/01/07 23:07:55, xiyuan wrote:
> We probably should do this in CertLibrary::Decrypt/EncryptToken. Let's put a
> TODO here.

Done.

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth2_login_verifier.cc (right):

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:42: Profile*
user_profile)
On 2013/01/07 23:07:55, xiyuan wrote:
> nit: 4-space indent for above three lines

Done.

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:173: retry_count_++ <
kMaxRequestAttemptCount) {
On 2013/01/07 23:07:55, xiyuan wrote:
> Get rid of '++' here since we are doing it in the next line. And fix
alignment.

Done.

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth2_login_verifier.h (right):

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.h:25: // can be used for
other token fetching calls (i.e. TokenService).
On 2013/01/07 23:07:55, xiyuan wrote:
> Update the comments?

Done.

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.h:72: // retries |task|
after certain delY until max retry count is reached.
On 2013/01/07 23:07:55, xiyuan wrote:
> nit: delY -> delay

Done.

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.h:78: Profile*
auth_profile_;
On 2013/01/07 23:07:55, xiyuan wrote:
> |auth_profile_| seems not used.

Done.

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth2_token_fetcher.cc (right):

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_token_fetcher.cc:63:
auth_fetcher_.StartCookieForOAuthLoginTokenExchange("1");
On 2013/01/07 23:07:55, xiyuan wrote:
> Is it proper to use a hard-coded "1" as session index here?

Yes, because the current session that came from GAIA web sign flow (auth profile
cookie jar) in should only have one session at this point.

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_token_fetcher.cc:108: retry_count_++ <
kMaxRequestAttemptCount) {
On 2013/01/07 23:07:55, xiyuan wrote:
> Get rid of '++' here and fix alignment.

Done.

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth2_token_fetcher.h (right):

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_token_fetcher.h:11: #include
"base/callback.h"
On 2013/01/07 23:07:55, xiyuan wrote:
> callback.h is probably not needed.

Done.

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_token_fetcher.h:23: // used to kick off
other token retrieval tasks (i.e. TokenService).
On 2013/01/07 23:07:55, xiyuan wrote:
> Update the comments here.

This is exactly what this class is used for.

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/net/gaia/g...
File chrome/browser/net/gaia/gaia_oauth_fetcher.cc (right):

https://codereview.chromium.org/11649055/diff/17001/chrome/browser/net/gaia/g...
chrome/browser/net/gaia/gaia_oauth_fetcher.cc:37: "service=%s";
On 2013/01/07 23:07:55, xiyuan wrote:
> kOAuth2LoginBodyFormat is not used anywhere. And I don't see where we set
> |request_type_| to OAUTH2_LOGIN and OAUTH2_REVOKE_TOKEN either.
> 
> Is the snapshot up-to-date?

changes to gaia_oauth_fetcher.cc/h are reverted now

[xiyuan, 2013-01-08 17:41:43]

LGTM

https://codereview.chromium.org/11649055/diff/19002/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth_login_manager.cc (right):

https://codereview.chromium.org/11649055/diff/19002/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.cc:54: TokenService*
OAuthLoginManager::SetupTokenService() {
Could SetupTokenService be called multiple times during the life time of
OAuthLoginManager? If it is only supposed to be called once, suggest to add a
DCHECK(registrar_.IsEmpty()) to be sure.

[zel, 2013-01-11 02:42:59]

https://codereview.chromium.org/11649055/diff/19002/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth_login_manager.cc (right):

https://codereview.chromium.org/11649055/diff/19002/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.cc:54: TokenService*
OAuthLoginManager::SetupTokenService() {
On 2013/01/08 17:41:43, xiyuan wrote:
> Could SetupTokenService be called multiple times during the life time of
> OAuthLoginManager? If it is only supposed to be called once, suggest to add a
> DCHECK(registrar_.IsEmpty()) to be sure.

Done.

[zel, 2013-01-11 02:53:27]

+joaodasilva, pastarmovj

...adding enterprise people to take a peek at policy token fetching part.

[pastarmovj, 2013-01-11 09:42:04]

I don't see issues with the policy fetching code but i will leave it to Joao to
stamp it as he is currently working with this code and knows the interactions
inside it better.

[Joao da Silva, 2013-01-11 16:45:07]

Lots of nits mixed within more relevant comments. The policy paths for oauth1
should be good, for oauth2 I've left a comment in oauth_login_manager.

I'll verify our use cases once this has landed too.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/c...
File chrome/browser/chromeos/cros/cert_library.cc (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/cert_library.cc:200: #ifndef NDEBUG
Is this meant to stay in?

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/login_performer.cc (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_performer.cc:10: #include
"base/command_line.h"
Not needed

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_performer.cc:28: #include
"chrome/common/chrome_switches.h"
Not needed

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_performer.cc:191:
LoginUtils::Get()->StartTokenServices(profile);
AFAICT this method is dead code.

It's only used as the callback for CreateDefaultProfileAsync above. However,
that is only posted when |delegate_| is NULL, which I think that never happens.

LoginPerformer is only created by existing_user_controller.cc, and it always
sets the ExistingUserController as the delegate (except for tests). It only
removes the delegate at ExistingUserController::OnLoginSuccess(), but it deletes
the LoginPerformer immediately after.

In any case, StartTokenServices is currently only starting the fetch of the
oauth token for the policy server. This is usually handled by
LoginUtils::PrepareProfile() or LoginUtils::OnProfileCreated(). If there is no
|delegate_| here then this should be invoked here; but AFAICT this method is
never used.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_utils.cc:274: // Removed deprecated OAuth1
token and secret form preference store.
*Removes, *from

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_utils.cc:280: // Starts signing related
services. Initiates TokenService token retreival.
*retrieval

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_utils.cc:284: bool pending_requests_;
pending_requests_ is dead code and can be removed.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_utils.cc:534: // have OAuth2 refresh token
in TokenService that could be used to retreive
*retrieve

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_utils.cc:623: /*
I guess this will be removed?

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_utils.cc:975: login_manager_.reset();
This only happens at shutdown and should be safe, however
OnConnectionTypeChanged() can happen at shutdown, and should test
|login_manager_| before using it.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/login_utils.h (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_utils.h:11: #include
"google_apis/gaia/gaia_auth_consumer.h"
Not needed

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth1_login_verifier.cc (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth1_login_verifier.cc:12: #include
"chrome/browser/profiles/profile_manager.h"
Not needed

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth1_login_verifier.cc:191: LOG(WARNING) <<
"MergeSession successful.";
warning on success?

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth1_login_verifier.h (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth1_login_verifier.h:18: class Profile;
Not needed

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth1_login_verifier.h:42:
net::URLRequestContextGetter* user_request_context,
Needs a forward decl

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth1_token_fetcher.cc (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth1_token_fetcher.cc:10: #include
"chrome/browser/profiles/profile.h"
Not needed

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth1_token_fetcher.h (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth1_token_fetcher.h:16: class Profile;
Not needed

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth1_token_fetcher.h:35:
net::URLRequestContextGetter* auth_context_getter);
Needs forward decl

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth2_login_verifier.cc (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:15: #include
"google_apis/gaia/oauth2_access_token_fetcher.h"
Already in .h

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:28: const char
kServiceScopeChromeOS[] =
Not used, did you mean to include in |scopes| below?

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:31: const char
kOAuthLoginServiceScope[] =
GaiaUrls::oauth1_login_scope()?

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:43: token_fetcher_(this,
system_request_context),
ALLOW_THIS_IN_INITIALIZER_LIST (also below)

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:68:
base::TimeDelta::FromMilliseconds(kRequestRestartDelay));
#include "base/time.h"

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:75:
std::vector<std::string> scopes;
#include <vector>

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:99: LOG(WARNING) <<
"Failed to hget OAuth2 access token, "
*get

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:109: LOG(WARNING) <<
"OAuthLogin successful!";
warning or vlog(1)?

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:120: LOG(WARNING) <<
"Failed to log in with OAuth1 access tokens,"
OAuth2

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:136: LOG(WARNING) << "Got
auth token!";
warning or vlog(1)?

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:153: LOG(WARNING) <<
"MergeSession successful.";
warning or vlog?

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:162:
base::Bind(&OAuth2LoginVerifier::StartCookiesRetrieval,
Is the retry callback StartCookiesRetrieval, or should it be a new method that
calls gaia_fetcher_.StartMergeSession?

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:188:
delegate_->OnOAuth2LoginVerifierFaulure();
*Failure

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth2_login_verifier.h (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.h:33: virtual void
OnOAuth2LoginVerifierFaulure() = 0;
*Failure

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.h:37:
net::URLRequestContextGetter* system_request_context,
Needs forward decl

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth2_policy_fetcher.cc (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_policy_fetcher.cc:13: #include
"chrome/browser/profiles/profile_manager.h"
These 2 not needed

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_policy_fetcher.cc:79:
std::vector<std::string> scopes;
#include <vector>

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth2_policy_fetcher.h (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_policy_fetcher.h:11: #include
"base/callback.h"
callback_forward.h can be used here

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_policy_fetcher.h:42: explicit
OAuth2PolicyFetcher(
Doesn't have to be explicit.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth2_token_fetcher.cc (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_token_fetcher.cc:10: #include
"chrome/browser/profiles/profile.h"
Not needed

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_token_fetcher.cc:61: }
This block is repeated in a couple of files. Could it be moved to a function
that verifies the network state, and posts a delayed Closure when offline?

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_token_fetcher.cc:74: LOG(WARNING) << "Got
OAuth2 tokens!";
vlog?

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth2_token_fetcher.h (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_token_fetcher.h:8: #include <string>
Not needed

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_token_fetcher.h:17: class Profile;
Not needed

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_token_fetcher.h:37:
net::URLRequestContextGetter* context_getter);
Needs forward decl

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth_login_manager.cc (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.cc:20: #include
"content/public/browser/notification_observer.h"
Already in .h

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.cc:33: const int
kCookieRecoveryRestartDelay = 10000;
These 2 aren't used.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.cc:248: void
OAuth2LoginManager::FetchPolicyTokens() {
This is never used. I think it should be called from RestoreSessionCookies
and/or OnOAuth2LoginVerifierSuccess.

For context, there are 2 paths to get the policy tokens: the blocking path
before creating the profile (handled by RestorePolicyTokens), and the path after
the profile is created. That path enter the manager in RestoreSession, and it
should call FetchPolicyTokens as soon as it has the tokens required (I think
that's the access token, for oauth1; for oauth2 I'm not sure).

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.cc:268: void
OAuth2LoginManager::OnOAuth2LoginVerifierFaulure() {
*Failure

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth_login_manager.h (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.h:13: #include
"chrome/browser/chromeos/login/policy_oauth_fetcher.h"
Order

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.h:22: class GaiaAuthFetcher;
Not needed

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.h:29: class OAuthLoginManager;
Not needed

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.h:55: // Starts the process of
retreiving policy tokens.
*retrieving

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.h:57:
net::URLRequestContextGetter* auth_request_context) = 0;
#include "net/url_request/url_request_context_getter.h"

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.h:69: // Stops all background
authentication requests.
Update comment?

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.h:79:
scoped_refptr<net::URLRequestContextGetter> auth_request_context_;
#include "base/memory/ref_counted.h"

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.h:86: class
OAuthLoginManagerFactory {
Document this class

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.h:101:
net::URLRequestContextGetter* auth_request_context);
OVERRIDE

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.h:119: virtual void
OnOAuth2LoginVerifierFaulure() OVERRIDE;
*Failure

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.h:119: virtual void
OnOAuth2LoginVerifierFaulure() OVERRIDE;
*Failure

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.h:139: // Issue GAIA cookie
recovery (MergeSession) from |uber_token_|.
What is uber_token_?

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.h:151: int
restore_attempt_count_;
THis isn't being used in the .cc; I guess the fetchers already retry, so this
can be removed?

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.h:172:
net::URLRequestContextGetter* auth_request_context);
OVERRIDE

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/policy_oauth_fetcher.cc (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/policy_oauth_fetcher.cc:12: #include
"chrome/browser/profiles/profile_manager.h"
These 2 not needed

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/policy_oauth_fetcher.h (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/policy_oauth_fetcher.h:15: class Profile;
Not needed

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/policy_oauth_fetcher.h:28:
PolicyOAuthFetcher(net::URLRequestContextGetter* context_getter,
Needs forward decl

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/profile_auth_data.h (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/profile_auth_data.h:9: #include "base/callback.h"
Closure is forward declared in base/callback_forward.h

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/signin/sig...
File chrome/browser/signin/signin_manager.cc (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/signin/sig...
chrome/browser/signin/signin_manager.cc:133: #if !defined(OS_CHROMEOS)
It's not clear to me why this was added, can you explain?

[zel, 2013-01-11 19:51:16]

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/c...
File chrome/browser/chromeos/cros/cert_library.cc (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/cert_library.cc:200: #ifndef NDEBUG
On 2013/01/11 16:45:07, Joao da Silva wrote:
> Is this meant to stay in?

Yes it is going to stay in. While we develop on Linux workstations, we don't
really care about encryption but without this function working properly we can't
test use cases when users are already signed in. Once we fully move to OAuth2,
this function is going away.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/login_performer.cc (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_performer.cc:10: #include
"base/command_line.h"
On 2013/01/11 16:45:07, Joao da Silva wrote:
> Not needed

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_performer.cc:28: #include
"chrome/common/chrome_switches.h"
On 2013/01/11 16:45:07, Joao da Silva wrote:
> Not needed

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_performer.cc:191:
LoginUtils::Get()->StartTokenServices(profile);
On 2013/01/11 16:45:07, Joao da Silva wrote:
> AFAICT this method is dead code.
> 
> It's only used as the callback for CreateDefaultProfileAsync above. However,
> that is only posted when |delegate_| is NULL, which I think that never
happens.
> 
> LoginPerformer is only created by existing_user_controller.cc, and it always
> sets the ExistingUserController as the delegate (except for tests). It only
> removes the delegate at ExistingUserController::OnLoginSuccess(), but it
deletes
> the LoginPerformer immediately after.
> 
> In any case, StartTokenServices is currently only starting the fetch of the
> oauth token for the policy server. This is usually handled by
> LoginUtils::PrepareProfile() or LoginUtils::OnProfileCreated(). If there is no
> |delegate_| here then this should be invoked here; but AFAICT this method is
> never used.

I suspect this might have something to do with screen locker then. Nikita might
know better. I will be happy to nuke it if I can't find when its being used.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_utils.cc:274: // Removed deprecated OAuth1
token and secret form preference store.
On 2013/01/11 16:45:07, Joao da Silva wrote:
> *Removes, *from

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_utils.cc:280: // Starts signing related
services. Initiates TokenService token retreival.
On 2013/01/11 16:45:07, Joao da Silva wrote:
> *retrieval

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_utils.cc:284: bool pending_requests_;
On 2013/01/11 16:45:07, Joao da Silva wrote:
> pending_requests_ is dead code and can be removed.

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_utils.cc:534: // have OAuth2 refresh token
in TokenService that could be used to retreive
On 2013/01/11 16:45:07, Joao da Silva wrote:
> *retrieve

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_utils.cc:623: /*
On 2013/01/11 16:45:07, Joao da Silva wrote:
> I guess this will be removed?

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_utils.cc:975: login_manager_.reset();
On 2013/01/11 16:45:07, Joao da Silva wrote:
> This only happens at shutdown and should be safe, however
> OnConnectionTypeChanged() can happen at shutdown, and should test
> |login_manager_| before using it.

Good catch. Added check to OnConnectionTypeChanged(). Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/login_utils.h (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_utils.h:11: #include
"google_apis/gaia/gaia_auth_consumer.h"
On 2013/01/11 16:45:07, Joao da Silva wrote:
> Not needed

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth1_login_verifier.cc (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth1_login_verifier.cc:12: #include
"chrome/browser/profiles/profile_manager.h"
On 2013/01/11 16:45:07, Joao da Silva wrote:
> Not needed

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth1_login_verifier.cc:191: LOG(WARNING) <<
"MergeSession successful.";
On 2013/01/11 16:45:07, Joao da Silva wrote:
> warning on success?

Yeah. I want to make sure we explicitly see this event in user feedback logs.
It's important for tracing potential auth related issues.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth1_login_verifier.h (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth1_login_verifier.h:18: class Profile;
On 2013/01/11 16:45:07, Joao da Silva wrote:
> Not needed

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth1_login_verifier.h:42:
net::URLRequestContextGetter* user_request_context,
On 2013/01/11 16:45:07, Joao da Silva wrote:
> Needs a forward decl

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth1_token_fetcher.cc (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth1_token_fetcher.cc:10: #include
"chrome/browser/profiles/profile.h"
On 2013/01/11 16:45:07, Joao da Silva wrote:
> Not needed

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth1_token_fetcher.h (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth1_token_fetcher.h:16: class Profile;
On 2013/01/11 16:45:07, Joao da Silva wrote:
> Not needed

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth1_token_fetcher.h:35:
net::URLRequestContextGetter* auth_context_getter);
On 2013/01/11 16:45:07, Joao da Silva wrote:
> Needs forward decl

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth2_login_verifier.cc (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:15: #include
"google_apis/gaia/oauth2_access_token_fetcher.h"
On 2013/01/11 16:45:07, Joao da Silva wrote:
> Already in .h

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:28: const char
kServiceScopeChromeOS[] =
On 2013/01/11 16:45:07, Joao da Silva wrote:
> Not used, did you mean to include in |scopes| below?

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:31: const char
kOAuthLoginServiceScope[] =
On 2013/01/11 16:45:07, Joao da Silva wrote:
> GaiaUrls::oauth1_login_scope()?

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:43: token_fetcher_(this,
system_request_context),
On 2013/01/11 16:45:07, Joao da Silva wrote:
> ALLOW_THIS_IN_INITIALIZER_LIST (also below)

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:68:
base::TimeDelta::FromMilliseconds(kRequestRestartDelay));
On 2013/01/11 16:45:07, Joao da Silva wrote:
> #include "base/time.h"

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:75:
std::vector<std::string> scopes;
On 2013/01/11 16:45:07, Joao da Silva wrote:
> #include <vector>

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:99: LOG(WARNING) <<
"Failed to hget OAuth2 access token, "
On 2013/01/11 16:45:07, Joao da Silva wrote:
> *get

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:109: LOG(WARNING) <<
"OAuthLogin successful!";
On 2013/01/11 16:45:07, Joao da Silva wrote:
> warning or vlog(1)?

Nope, I really want this in user feedback logs as I said earlier.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:120: LOG(WARNING) <<
"Failed to log in with OAuth1 access tokens,"
On 2013/01/11 16:45:07, Joao da Silva wrote:
> OAuth2

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:136: LOG(WARNING) << "Got
auth token!";
On 2013/01/11 16:45:07, Joao da Silva wrote:
> warning or vlog(1)?

Same as above.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:153: LOG(WARNING) <<
"MergeSession successful.";
On 2013/01/11 16:45:07, Joao da Silva wrote:
> warning or vlog?

Same as above.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:162:
base::Bind(&OAuth2LoginVerifier::StartCookiesRetrieval,
On 2013/01/11 16:45:07, Joao da Silva wrote:
> Is the retry callback StartCookiesRetrieval, or should it be a new method that
> calls gaia_fetcher_.StartMergeSession?

Well, the token that we fetched with /IssueAuthToken could have become invalid
too (i.e. expired). It's safer to repeat both calls in a case of a failure.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:188:
delegate_->OnOAuth2LoginVerifierFaulure();
On 2013/01/11 16:45:07, Joao da Silva wrote:
> *Failure

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth2_login_verifier.h (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.h:33: virtual void
OnOAuth2LoginVerifierFaulure() = 0;
On 2013/01/11 16:45:07, Joao da Silva wrote:
> *Failure

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.h:37:
net::URLRequestContextGetter* system_request_context,
On 2013/01/11 16:45:07, Joao da Silva wrote:
> Needs forward decl

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth2_policy_fetcher.cc (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_policy_fetcher.cc:13: #include
"chrome/browser/profiles/profile_manager.h"
On 2013/01/11 16:45:07, Joao da Silva wrote:
> These 2 not needed

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_policy_fetcher.cc:79:
std::vector<std::string> scopes;
On 2013/01/11 16:45:07, Joao da Silva wrote:
> #include <vector>

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth2_policy_fetcher.h (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_policy_fetcher.h:11: #include
"base/callback.h"
On 2013/01/11 16:45:07, Joao da Silva wrote:
> callback_forward.h can be used here

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_policy_fetcher.h:42: explicit
OAuth2PolicyFetcher(
On 2013/01/11 16:45:07, Joao da Silva wrote:
> Doesn't have to be explicit.

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth2_token_fetcher.cc (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_token_fetcher.cc:10: #include
"chrome/browser/profiles/profile.h"
On 2013/01/11 16:45:07, Joao da Silva wrote:
> Not needed

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_token_fetcher.cc:74: LOG(WARNING) << "Got
OAuth2 tokens!";
On 2013/01/11 16:45:07, Joao da Silva wrote:
> vlog?

Intentionally left as WARNING so we can get it in user feedback logs.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth2_token_fetcher.h (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_token_fetcher.h:8: #include <string>
On 2013/01/11 16:45:07, Joao da Silva wrote:
> Not needed

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_token_fetcher.h:17: class Profile;
On 2013/01/11 16:45:07, Joao da Silva wrote:
> Not needed

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_token_fetcher.h:37:
net::URLRequestContextGetter* context_getter);
On 2013/01/11 16:45:07, Joao da Silva wrote:
> Needs forward decl

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth_login_manager.cc (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.cc:20: #include
"content/public/browser/notification_observer.h"
On 2013/01/11 16:45:07, Joao da Silva wrote:
> Already in .h

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.cc:33: const int
kCookieRecoveryRestartDelay = 10000;
On 2013/01/11 16:45:07, Joao da Silva wrote:
> These 2 aren't used.

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.cc:248: void
OAuth2LoginManager::FetchPolicyTokens() {
On 2013/01/11 16:45:07, Joao da Silva wrote:
> This is never used. I think it should be called from RestoreSessionCookies
> and/or OnOAuth2LoginVerifierSuccess.
> 
> For context, there are 2 paths to get the policy tokens: the blocking path
> before creating the profile (handled by RestorePolicyTokens), and the path
after
> the profile is created. That path enter the manager in RestoreSession, and it
> should call FetchPolicyTokens as soon as it has the tokens required (I think
> that's the access token, for oauth1; for oauth2 I'm not sure).

It's actually just needed in RestoreSessionCookies() since this method will be
kicked off whenever we retrieve OAuth2 refresh token from the TokenService. In
cookies-to-tokens path, this will be triggered through OnOAuth2TokenAvailable()
saving OAuth2 tokens in TokenService. Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.cc:268: void
OAuth2LoginManager::OnOAuth2LoginVerifierFaulure() {
On 2013/01/11 16:45:07, Joao da Silva wrote:
> *Failure

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth_login_manager.h (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.h:13: #include
"chrome/browser/chromeos/login/policy_oauth_fetcher.h"
On 2013/01/11 16:45:07, Joao da Silva wrote:
> Order

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.h:22: class GaiaAuthFetcher;
On 2013/01/11 16:45:07, Joao da Silva wrote:
> Not needed

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.h:29: class OAuthLoginManager;
On 2013/01/11 16:45:07, Joao da Silva wrote:
> Not needed

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.h:55: // Starts the process of
retreiving policy tokens.
On 2013/01/11 16:45:07, Joao da Silva wrote:
> *retrieving

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.h:57:
net::URLRequestContextGetter* auth_request_context) = 0;
On 2013/01/11 16:45:07, Joao da Silva wrote:
> #include "net/url_request/url_request_context_getter.h"

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.h:69: // Stops all background
authentication requests.
On 2013/01/11 16:45:07, Joao da Silva wrote:
> Update comment?

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.h:79:
scoped_refptr<net::URLRequestContextGetter> auth_request_context_;
On 2013/01/11 16:45:07, Joao da Silva wrote:
> #include "base/memory/ref_counted.h"

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.h:86: class
OAuthLoginManagerFactory {
On 2013/01/11 16:45:07, Joao da Silva wrote:
> Document this class

Moved factory method into OAuthLoginManager instead.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.h:101:
net::URLRequestContextGetter* auth_request_context);
On 2013/01/11 16:45:07, Joao da Silva wrote:
> OVERRIDE

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.h:119: virtual void
OnOAuth2LoginVerifierFaulure() OVERRIDE;
On 2013/01/11 16:45:07, Joao da Silva wrote:
> *Failure

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.h:139: // Issue GAIA cookie
recovery (MergeSession) from |uber_token_|.
On 2013/01/11 16:45:07, Joao da Silva wrote:
> What is uber_token_?

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.h:151: int
restore_attempt_count_;
On 2013/01/11 16:45:07, Joao da Silva wrote:
> THis isn't being used in the .cc; I guess the fetchers already retry, so this
> can be removed?

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.h:172:
net::URLRequestContextGetter* auth_request_context);
On 2013/01/11 16:45:07, Joao da Silva wrote:
> OVERRIDE

Done.

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/signin/sig...
File chrome/browser/signin/signin_manager.cc (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/signin/sig...
chrome/browser/signin/signin_manager.cc:133: #if !defined(OS_CHROMEOS)
On 2013/01/11 16:45:07, Joao da Silva wrote:
> It's not clear to me why this was added, can you explain?

I added the comment in the code. Done.

[Joao da Silva, 2013-01-11 20:13:15]

lgtm

Thanks for the explanations in the comments!

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/c...
File chrome/browser/chromeos/cros/cert_library.cc (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/cert_library.cc:200: #ifndef NDEBUG
On 2013/01/11 19:51:16, zel wrote:
> On 2013/01/11 16:45:07, Joao da Silva wrote:
> > Is this meant to stay in?
> 
> Yes it is going to stay in. While we develop on Linux workstations, we don't
> really care about encryption but without this function working properly we
can't
> test use cases when users are already signed in. Once we fully move to OAuth2,
> this function is going away.

Ah, I see now. IsRunningOnChromeOS() may be a more reliable signal in that case,
and works for Release builds on linux.

https://codereview.chromium.org/11649055/diff/41008/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/11649055/diff/41008/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_utils.cc:272: // Removes deprecated OAuth1
token and secret form preference store.
*from

https://codereview.chromium.org/11649055/diff/41008/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/policy_oauth_fetcher.cc (right):

https://codereview.chromium.org/11649055/diff/41008/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/policy_oauth_fetcher.cc:12: #include
"chrome/browser/profiles/profile_manager.h"
These 2 aren't used

https://codereview.chromium.org/11649055/diff/41008/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/policy_oauth_fetcher.h (right):

https://codereview.chromium.org/11649055/diff/41008/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/policy_oauth_fetcher.h:15: class Profile;
Not used

https://codereview.chromium.org/11649055/diff/41008/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/policy_oauth_fetcher.h:28:
PolicyOAuthFetcher(net::URLRequestContextGetter* context_getter,
Forward decl for URLRequestContextGetter

[zel, 2013-01-11 20:54:20]

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/c...
File chrome/browser/chromeos/cros/cert_library.cc (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/c...
chrome/browser/chromeos/cros/cert_library.cc:200: #ifndef NDEBUG
On 2013/01/11 20:13:15, Joao da Silva wrote:
> On 2013/01/11 19:51:16, zel wrote:
> > On 2013/01/11 16:45:07, Joao da Silva wrote:
> > > Is this meant to stay in?
> > 
> > Yes it is going to stay in. While we develop on Linux workstations, we don't
> > really care about encryption but without this function working properly we
> can't
> > test use cases when users are already signed in. Once we fully move to
OAuth2,
> > this function is going away.
> 
> Ah, I see now. IsRunningOnChromeOS() may be a more reliable signal in that
case,
> and works for Release builds on linux.

Done.

https://codereview.chromium.org/11649055/diff/41008/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/11649055/diff/41008/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_utils.cc:272: // Removes deprecated OAuth1
token and secret form preference store.
On 2013/01/11 20:13:15, Joao da Silva wrote:
> *from

That method is nuked actually.

https://codereview.chromium.org/11649055/diff/41008/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/policy_oauth_fetcher.cc (right):

https://codereview.chromium.org/11649055/diff/41008/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/policy_oauth_fetcher.cc:12: #include
"chrome/browser/profiles/profile_manager.h"
On 2013/01/11 20:13:15, Joao da Silva wrote:
> These 2 aren't used

Done.

https://codereview.chromium.org/11649055/diff/41008/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/policy_oauth_fetcher.h (right):

https://codereview.chromium.org/11649055/diff/41008/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/policy_oauth_fetcher.h:15: class Profile;
On 2013/01/11 20:13:15, Joao da Silva wrote:
> Not used

Done.

https://codereview.chromium.org/11649055/diff/41008/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/policy_oauth_fetcher.h:28:
PolicyOAuthFetcher(net::URLRequestContextGetter* context_getter,
On 2013/01/11 20:13:15, Joao da Silva wrote:
> Forward decl for URLRequestContextGetter

Done.

[zel, 2013-01-11 21:41:25]

+rogerta for TokenService and friends

[Nikita (slow), 2013-01-11 22:02:02]

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/login_performer.cc (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_performer.cc:191:
LoginUtils::Get()->StartTokenServices(profile);
On 2013/01/11 19:51:16, zel wrote:
> On 2013/01/11 16:45:07, Joao da Silva wrote:
> > AFAICT this method is dead code.
> > 
> > It's only used as the callback for CreateDefaultProfileAsync above. However,
> > that is only posted when |delegate_| is NULL, which I think that never
> happens.
> > 
> > LoginPerformer is only created by existing_user_controller.cc, and it always
> > sets the ExistingUserController as the delegate (except for tests). It only
> > removes the delegate at ExistingUserController::OnLoginSuccess(), but it
> deletes
> > the LoginPerformer immediately after.
> > 
> > In any case, StartTokenServices is currently only starting the fetch of the
> > oauth token for the policy server. This is usually handled by
> > LoginUtils::PrepareProfile() or LoginUtils::OnProfileCreated(). If there is
no
> > |delegate_| here then this should be invoked here; but AFAICT this method is
> > never used.
> 
> I suspect this might have something to do with screen locker then. Nikita
might
> know better. I will be happy to nuke it if I can't find when its being used.

Yes, this method could be removed.

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_utils.cc:281: // True if the authenrication
profile's cookie jar should contain
nit: authentication

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth1_login_verifier.cc (left):

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth1_login_verifier.cc:79: if (user_profile_ !=
ProfileManager::GetDefaultProfile())
Was this a guard to ignore call to ContinueVerification() that has been made
before profile has been loaded?

Obviously we should not make such calls before profile is loaded.
This should not happen, right?

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth1_login_verifier.cc (right):

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth1_login_verifier.cc:190: LOG(WARNING) <<
"MergeSession successful.";
nit: LOG(INFO)?

Is this a temporary change to get this info in logs?
I think that in case of successful MergeSession we might omit that from logs or
do you think it might be still helpful in some sense?

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/user_manager_impl.cc:524: DVLOG(1) << "Invalidate
OAuth token because of a sync error.";
LOG(WARNING) << OAuth1

[Nikita (slow), 2013-01-11 22:57:39]

lgtm

mostly nits

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth2_login_verifier.cc (right):

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:105: LOG(WARNING) <<
"OAuthLogin successful!";
VLOG(1).
You could temporarily change log level for this module for the duration of
testing.

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:132: LOG(WARNING) << "Got
auth token!";
VLOG(1)

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:149: LOG(WARNING) <<
"MergeSession successful.";
LOG(INFO)?

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth2_policy_fetcher.cc (right):

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_policy_fetcher.cc:28: const int
kMaxRequestAttemptCount = 5;
nit: Move these retry constants to some common place across login code?

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_policy_fetcher.cc:98: RetryOnError(error,
LOG(WARNING)?

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_policy_fetcher.cc:112: RetryOnError(error,
LOG(WARNING)?

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_policy_fetcher.cc:130: LOG(WARNING) <<
"Unrecoverable error or retry count max reached.";
Error message should include mention policy fetcher otherwise it is too generic

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth_login_manager.cc (right):

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.cc:8: #include
"base/metrics/histogram.h"
I think this is not needed.

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.cc:46: }
else
  LOG(WARNING) ?

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.cc:98: 
nit: drop empty line

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.cc:167: state_ =
OAuthLoginManager::SESSION_RESTORE_DONE;
LOG(WARNING)

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.cc:197:
UserManager::Get()->SaveUserOAuthStatus(
LOG(WARNING)

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.cc:257: state_ =
OAuthLoginManager::SESSION_RESTORE_DONE;
LOG(WARNING)

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.cc:271:
User::OAUTH2_TOKEN_STATUS_INVALID);
LOG(WARNING)

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth_login_manager.h (right):

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.h:47: // Raised when cookie
jar authentication is successfully completed.
nit: Makes sense to add empty line before comments as we usually do in codebase
to improve readability.

This applies to whole CL.

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.h:93: class OAuth2LoginManager
: public OAuthLoginManager,
nit: What do you think about splitting OAuth2LoginManager / OAuth1LoginManager 
into a separate file?

[zel, 2013-01-12 02:07:36]

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/login_performer.cc (right):

https://codereview.chromium.org/11649055/diff/41001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_performer.cc:191:
LoginUtils::Get()->StartTokenServices(profile);
On 2013/01/11 22:02:03, Nikita Kostylev wrote:
> On 2013/01/11 19:51:16, zel wrote:
> > On 2013/01/11 16:45:07, Joao da Silva wrote:
> > > AFAICT this method is dead code.
> > > 
> > > It's only used as the callback for CreateDefaultProfileAsync above.
However,
> > > that is only posted when |delegate_| is NULL, which I think that never
> > happens.
> > > 
> > > LoginPerformer is only created by existing_user_controller.cc, and it
always
> > > sets the ExistingUserController as the delegate (except for tests). It
only
> > > removes the delegate at ExistingUserController::OnLoginSuccess(), but it
> > deletes
> > > the LoginPerformer immediately after.
> > > 
> > > In any case, StartTokenServices is currently only starting the fetch of
the
> > > oauth token for the policy server. This is usually handled by
> > > LoginUtils::PrepareProfile() or LoginUtils::OnProfileCreated(). If there
is
> no
> > > |delegate_| here then this should be invoked here; but AFAICT this method
is
> > > never used.
> > 
> > I suspect this might have something to do with screen locker then. Nikita
> might
> > know better. I will be happy to nuke it if I can't find when its being used.
> 
> Yes, this method could be removed.

Removed LoginPerformer::OnProfileCreated(), branch of delegate_==NULL in
LoginPerformer::OnLoginSuccess() and removed LoginUtils::StartTokenServices().

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/login_utils.cc (right):

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/login_utils.cc:281: // True if the authenrication
profile's cookie jar should contain
On 2013/01/11 22:02:03, Nikita Kostylev wrote:
> nit: authentication

Done.

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth1_login_verifier.cc (right):

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth1_login_verifier.cc:190: LOG(WARNING) <<
"MergeSession successful.";
On 2013/01/11 22:02:03, Nikita Kostylev wrote:
> nit: LOG(INFO)?
> 
> Is this a temporary change to get this info in logs?
> I think that in case of successful MergeSession we might omit that from logs
or
> do you think it might be still helpful in some sense?

No, I really want to see these entries in feedback logs. They are important for
debugging auth issues.

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth2_login_verifier.cc (right):

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:105: LOG(WARNING) <<
"OAuthLogin successful!";
On 2013/01/11 22:57:39, Nikita Kostylev wrote:
> VLOG(1).
> You could temporarily change log level for this module for the duration of
> testing.

Nope. I really want this in user feedback logs.

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:132: LOG(WARNING) << "Got
auth token!";
On 2013/01/11 22:57:39, Nikita Kostylev wrote:
> VLOG(1)

same

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_login_verifier.cc:149: LOG(WARNING) <<
"MergeSession successful.";
On 2013/01/11 22:57:39, Nikita Kostylev wrote:
> LOG(INFO)?

same

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth2_policy_fetcher.cc (right):

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_policy_fetcher.cc:98: RetryOnError(error,
On 2013/01/11 22:57:39, Nikita Kostylev wrote:
> LOG(WARNING)?

Done.

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_policy_fetcher.cc:112: RetryOnError(error,
On 2013/01/11 22:57:39, Nikita Kostylev wrote:
> LOG(WARNING)?

Done.

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth2_policy_fetcher.cc:130: LOG(WARNING) <<
"Unrecoverable error or retry count max reached.";
On 2013/01/11 22:57:39, Nikita Kostylev wrote:
> Error message should include mention policy fetcher otherwise it is too
generic

The comment has a class name in the log, we should be good here.

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth_login_manager.cc (right):

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.cc:8: #include
"base/metrics/histogram.h"
On 2013/01/11 22:57:39, Nikita Kostylev wrote:
> I think this is not needed.

Done.

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.cc:98: 
On 2013/01/11 22:57:39, Nikita Kostylev wrote:
> nit: drop empty line

Done.

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.cc:167: state_ =
OAuthLoginManager::SESSION_RESTORE_DONE;
On 2013/01/11 22:57:39, Nikita Kostylev wrote:
> LOG(WARNING)

Done.

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.cc:197:
UserManager::Get()->SaveUserOAuthStatus(
On 2013/01/11 22:57:39, Nikita Kostylev wrote:
> LOG(WARNING)

Actually we should never fetch this token from TokenService,
chrome::NOTIFICATION_TOKEN_REQUEST_FAILED is removed now.

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.cc:257: state_ =
OAuthLoginManager::SESSION_RESTORE_DONE;
On 2013/01/11 22:57:39, Nikita Kostylev wrote:
> LOG(WARNING)

Done.

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.cc:271:
User::OAUTH2_TOKEN_STATUS_INVALID);
On 2013/01/11 22:57:39, Nikita Kostylev wrote:
> LOG(WARNING)

Done.

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/oauth_login_manager.h (right):

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.h:47: // Raised when cookie
jar authentication is successfully completed.
On 2013/01/11 22:57:39, Nikita Kostylev wrote:
> nit: Makes sense to add empty line before comments as we usually do in
codebase
> to improve readability.
> 
> This applies to whole CL.

Done.

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/oauth_login_manager.h:93: class OAuth2LoginManager
: public OAuthLoginManager,
On 2013/01/11 22:57:39, Nikita Kostylev wrote:
> nit: What do you think about splitting OAuth2LoginManager / OAuth1LoginManager

> into a separate file?

Done.

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://codereview.chromium.org/11649055/diff/66001/chrome/browser/chromeos/l...
chrome/browser/chromeos/login/user_manager_impl.cc:524: DVLOG(1) << "Invalidate
OAuth token because of a sync error.";
On 2013/01/11 22:02:03, Nikita Kostylev wrote:
> LOG(WARNING) << OAuth1

The comment was good, the code below wasn't.

[tim (not reviewing), 2013-01-15 02:36:34]

browser/signin LGTM, although had a few comments / suggestions if you're (or
anyone!) is planning on continuing to refactor cros login code.

https://codereview.chromium.org/11649055/diff/75001/chrome/browser/signin/sig...
File chrome/browser/signin/signin_manager.cc (right):

https://codereview.chromium.org/11649055/diff/75001/chrome/browser/signin/sig...
chrome/browser/signin/signin_manager.cc:137: #if !defined(OS_CHROMEOS)
fwiw, we've been trying to get rid of ifdefs like this in this code due to the
subtle behavior differences / bugs that have cropped up, and test
cumbersomeness, but I suppose there's no way around it at the moment.  

It'd be nice if we could factor out the cros specificness into a separate class
or delegate some day, so TokenService & SigninManager (and in general, "the code
responsible for getting auth tokens") are less fragmented.

It'd also be nice to hook things up to about:signin.  We had an intern working
on parts of this for Chrome OS, although the patch never did land
(https://codereview.chromium.org/11618024/)