Changes to the sign in manager in preparation for one-click sign in. This CL

Changes to the sign in manager in preparation for one-click sign in.  This CL
depends on http://codereview.chromium.org/9465018/

BUG=110050
TEST=No user visible changes, see unit tests.

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

[Andrew T Wilson (Slow), 2012-02-27 19:00:10]

http://codereview.chromium.org/9466022/diff/1/chrome/browser/signin/signin_ma...
File chrome/browser/signin/signin_manager.cc (left):

http://codereview.chromium.org/9466022/diff/1/chrome/browser/signin/signin_ma...
chrome/browser/signin/signin_manager.cc:242: #if !defined(OS_CHROMEOS)
Is it OK to remove this #ifdef?

http://codereview.chromium.org/9466022/diff/1/chrome/browser/signin/signin_ma...
File chrome/browser/signin/signin_manager.cc (right):

http://codereview.chromium.org/9466022/diff/1/chrome/browser/signin/signin_ma...
chrome/browser/signin/signin_manager.cc:223: void
SigninManager::OnClientLoginFailure(const GoogleServiceAuthError& error) {
nit: my preference is not to move functions around in existing files as it makes
it harder to review. If we want to enforce alphabetic ordering, I'd rather drive
that as a separate CL where you explicitly state you're just moving routines
around, but as a reviewer it's not easy to see whether you changed this
implementation or just moved it. It's fine to leave this change in this CL
though.

http://codereview.chromium.org/9466022/diff/1/chrome/browser/signin/signin_ma...
File chrome/browser/signin/signin_manager.h (right):

http://codereview.chromium.org/9466022/diff/1/chrome/browser/signin/signin_ma...
chrome/browser/signin/signin_manager.h:86: // Otherwise the result is the same
as StartSignIn().
Can you clarify this - does this mean we try to login twice (once with cookies,
then once with username/password)? This seems like an odd contract (it's not
clear to me reading this what notifications would get generated in this process,
if the password is guaranteed to be valid if signin is done via cookies, etc).

http://codereview.chromium.org/9466022/diff/1/chrome/browser/signin/signin_ma...
File chrome/browser/signin/signin_manager_unittest.cc (right):

http://codereview.chromium.org/9466022/diff/1/chrome/browser/signin/signin_ma...
chrome/browser/signin/signin_manager_unittest.cc:189: EXPECT_EQ(1U,
google_login_success_.size());
Is it difficult to validate that the correct password is passed with the SUCCESS
notification?

http://codereview.chromium.org/9466022/diff/1/chrome/browser/signin/signin_ma...
chrome/browser/signin/signin_manager_unittest.cc:195:
EXPECT_EQ("user@gmail.com", manager_->GetAuthenticatedUsername());
Why does the authenticated username not match the username supplied to
StartSignInWithCredentials()?

[Roger Tawa OOO till Jul 10th, 2012-02-29 21:02:54]

Thanks Drew.  Comments addressed, changes uploaded.  Please take another look.

http://codereview.chromium.org/9466022/diff/1/chrome/browser/signin/signin_ma...
File chrome/browser/signin/signin_manager.cc (left):

http://codereview.chromium.org/9466022/diff/1/chrome/browser/signin/signin_ma...
chrome/browser/signin/signin_manager.cc:242: #if !defined(OS_CHROMEOS)
On 2012/02/27 19:00:10, Andrew T Wilson wrote:
> Is it OK to remove this #ifdef?

This was dead code that was not used.  When I first implemented pre-login, I
used the TokenAuth api.  However, this caused problems with multi-login, so I
changed to using MergeSession instead and never removed this code.

However, now I do need to use TokenAuth, but it should not call
CleanupNotificationRegistration().  Good catch.  I created a new function so as
to not duplicate too much error handling code.

http://codereview.chromium.org/9466022/diff/1/chrome/browser/signin/signin_ma...
File chrome/browser/signin/signin_manager.cc (right):

http://codereview.chromium.org/9466022/diff/1/chrome/browser/signin/signin_ma...
chrome/browser/signin/signin_manager.cc:223: void
SigninManager::OnClientLoginFailure(const GoogleServiceAuthError& error) {
On 2012/02/27 19:00:10, Andrew T Wilson wrote:
> nit: my preference is not to move functions around in existing files as it
makes
> it harder to review. If we want to enforce alphabetic ordering, I'd rather
drive
> that as a separate CL where you explicitly state you're just moving routines
> around, but as a reviewer it's not easy to see whether you changed this
> implementation or just moved it. It's fine to leave this change in this CL
> though.

OK Drew, will do that net time.  I moved them around to adhere to chrome's style
guide which states that functions in the cc file should appear in the same order
as in the h file.

http://codereview.chromium.org/9466022/diff/1/chrome/browser/signin/signin_ma...
File chrome/browser/signin/signin_manager.h (right):

http://codereview.chromium.org/9466022/diff/1/chrome/browser/signin/signin_ma...
chrome/browser/signin/signin_manager.h:86: // Otherwise the result is the same
as StartSignIn().
On 2012/02/27 19:00:10, Andrew T Wilson wrote:
> Can you clarify this - does this mean we try to login twice (once with
cookies,
> then once with username/password)? This seems like an odd contract (it's not
> clear to me reading this what notifications would get generated in this
process,
> if the password is guaranteed to be valid if signin is done via cookies, etc).

No only one login, and this with no user interaction.  This function essentially
uses the cookies already found in the cookie jar to kick off the process of
logging in.  However, the cookies are not enough to get the user's password,
hence it is passed in here.  The username is used to set
possibly_invalid_username_.

http://codereview.chromium.org/9466022/diff/1/chrome/browser/signin/signin_ma...
File chrome/browser/signin/signin_manager_unittest.cc (right):

http://codereview.chromium.org/9466022/diff/1/chrome/browser/signin/signin_ma...
chrome/browser/signin/signin_manager_unittest.cc:189: EXPECT_EQ(1U,
google_login_success_.size());
On 2012/02/27 19:00:10, Andrew T Wilson wrote:
> Is it difficult to validate that the correct password is passed with the
SUCCESS
> notification?

The password is cleared, so not sure what you mean.

http://codereview.chromium.org/9466022/diff/1/chrome/browser/signin/signin_ma...
chrome/browser/signin/signin_manager_unittest.cc:195:
EXPECT_EQ("user@gmail.com", manager_->GetAuthenticatedUsername());
On 2012/02/27 19:00:10, Andrew T Wilson wrote:
> Why does the authenticated username not match the username supplied to
> StartSignInWithCredentials()?

The test passed because SimulateValidResponseSignInWithCredentials() used
"user@gmail.com".  I changed the code for this test t be consistent and use
"user@gmail.com" everywhere.

[Andrew T Wilson (Slow), 2012-03-02 19:22:32]

LGTM with a few nits.

http://codereview.chromium.org/9466022/diff/1/chrome/browser/signin/signin_ma...
File chrome/browser/signin/signin_manager_unittest.cc (right):

http://codereview.chromium.org/9466022/diff/1/chrome/browser/signin/signin_ma...
chrome/browser/signin/signin_manager_unittest.cc:189: EXPECT_EQ(1U,
google_login_success_.size());
On 2012/02/29 21:02:54, Roger Tawa wrote:
> On 2012/02/27 19:00:10, Andrew T Wilson wrote:
> > Is it difficult to validate that the correct password is passed with the
> SUCCESS
> > notification?
> 
> The password is cleared, so not sure what you mean.
I meant that all we're doing is detecting that LOGIN_SUCCESS notifications are
sent out, without checking that the right values are passed with it (like the
password). It'd be good to test that as well, if it's not hard.

http://codereview.chromium.org/9466022/diff/23001/chrome/browser/signin/signi...
File chrome/browser/signin/signin_manager.cc (right):

http://codereview.chromium.org/9466022/diff/23001/chrome/browser/signin/signi...
chrome/browser/signin/signin_manager.cc:237:
GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS;
Probably worth documenting why we are not calling HandleAuthError() here.

http://codereview.chromium.org/9466022/diff/23001/chrome/browser/signin/signi...
chrome/browser/signin/signin_manager.cc:313: // REVIEW: why does this call
OnClientLoginFailure?
Hmmm. I'm guessing to make sure the SIGNIN_FAILED notification is sent out? I'm
not sure - timsteele may know.

[Roger Tawa OOO till Jul 10th, 2012-03-02 20:10:58]

Thanks Drew.  Comments addressed, changes uploaded.

http://codereview.chromium.org/9466022/diff/1/chrome/browser/signin/signin_ma...
File chrome/browser/signin/signin_manager_unittest.cc (right):

http://codereview.chromium.org/9466022/diff/1/chrome/browser/signin/signin_ma...
chrome/browser/signin/signin_manager_unittest.cc:189: EXPECT_EQ(1U,
google_login_success_.size());
On 2012/03/02 19:22:33, Andrew T Wilson wrote:
> On 2012/02/29 21:02:54, Roger Tawa wrote:
> > On 2012/02/27 19:00:10, Andrew T Wilson wrote:
> > > Is it difficult to validate that the correct password is passed with the
> > SUCCESS
> > > notification?
> > 
> > The password is cleared, so not sure what you mean.
> I meant that all we're doing is detecting that LOGIN_SUCCESS notifications are
> sent out, without checking that the right values are passed with it (like the
> password). It'd be good to test that as well, if it's not hard.

Its not easy, probably why it was not done for the existing SignInClientLogin
test.  The notification sent out contains a pointer to a details object which is
on the stack in OnGetUserInfoSuccess(), so we can't check it here.  To check
this we could create specific tracker instead of using the generic
TestNotificationTracker, but I'd prefer to do that in a separate CL.

http://codereview.chromium.org/9466022/diff/23001/chrome/browser/signin/signi...
File chrome/browser/signin/signin_manager.cc (right):

http://codereview.chromium.org/9466022/diff/23001/chrome/browser/signin/signi...
chrome/browser/signin/signin_manager.cc:237:
GoogleServiceAuthError::INVALID_GAIA_CREDENTIALS;
On 2012/03/02 19:22:33, Andrew T Wilson wrote:
> Probably worth documenting why we are not calling HandleAuthError() here.

Done.

[commit-bot: I haz the power, 2012-03-02 20:11:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rogerta@chromium.org/9466022/27002

[commit-bot: I haz the power, 2012-03-02 23:58:01]

Change committed as 124781