When a user logs into sync, the appropriate cookies are retrieved so that

When a user logs into sync, the appropriate cookies are retrieved so that
she is already logged into Google web services, and does not need to enter
her username and password again.

This feature is on by default, but can be turned off by specifying
--disable-auto-login on the command line or the about:flags page.

BUG=None
TEST=Make sure the browser has no google or youtube cookies.  Either clear
all the cookies or start with a brand new profile.  Go to menu item
"Wrench / Options", go to the tab "Personal stuff", and click the "Enable
these features..." button to enable sync.  Follow the wizard to login to your
google account and finish the sync process.  Once terminated, browse to
gmail.com and you should be already logged in.

[tim (not reviewing), 2011-06-08 21:01:53]

On 2011/06/08 19:49:57, Roger Tawa wrote:

If the user signs out of (e.g.) gmail, possibly signing in to a different
account, do we really want to forcefully re-sign them back in to the sync
account each time a token is available?  E.g. password changes.. but also, does
TOKEN_AVAILABLE not get sent on each restart?

[Roger Tawa OOO till Jul 10th, 2011-06-10 13:34:02]

Thanks for the feedback Tim.  Please take another look, I've made the changes we
discussed plus other changes based on email threads.

[tim (not reviewing), 2011-06-14 13:18:29]

sorry for late reply on this!

one comment, i'll take a look after but looks pretty good.

http://codereview.chromium.org/7121014/diff/10004/chrome/browser/sync/signin_...
File chrome/browser/sync/signin_manager.h (right):

http://codereview.chromium.org/7121014/diff/10004/chrome/browser/sync/signin_...
chrome/browser/sync/signin_manager.h:86: #if !defined(OS_CHROMEOS)
maybe we should create a const in browser/defaults.h instead of ifdefing
everywhere here?

[Roger Tawa OOO till Jul 10th, 2011-06-14 18:42:43]

Hi Tim,

Maybe I don't quite understand what you mean, please see below.  Thanks.

http://codereview.chromium.org/7121014/diff/10004/chrome/browser/sync/signin_...
File chrome/browser/sync/signin_manager.h (right):

http://codereview.chromium.org/7121014/diff/10004/chrome/browser/sync/signin_...
chrome/browser/sync/signin_manager.h:86: #if !defined(OS_CHROMEOS)
On 2011/06/14 13:18:29, timsteele wrote:
> maybe we should create a const in browser/defaults.h instead of ifdefing
> everywhere here? 

I don't want to compile any of this code in the case of chromeos.  Maybe I don't
understand your request?

[tim (not reviewing), 2011-06-15 16:12:13]

It's just pretty cumbersome to have inheritance controlled by #ifdef IMO.  And
this isn't a case where the types in question (NotificationService) don't exist
on the platform.  I think historically we've done other things. E.g. you could
have a BaseSigninService and an AutoLoginSigninService. Here I thought just
having the AddObserver call controlled by an OS-setting was cleaner than the
ifdef. 

Maybe I'm crazy though.  Do you see precedence for this?  Adding sky for another
opinion. Sorry for slowing this down so much...

[sky, 2011-06-15 16:44:08]

Yes, we generally try to avoid ifdef'tastic code like this. Two suggestions:

. Add a factory, and have a chromeos specific subclass.
. Remove all the ifdefs from the .h (meaning SigninManager implements
NotificationObserver). Have the constructor call some method, say
RegisterObservers. RegisterObservers and and Observe are ifdef'd in the .cc to
do nothing for non-chromeos builds, and create a SigninManager_chromeos.cc that
has the chromeos-specific bits.

The first creates a cleaner separation, but the second is commonly used for
cases where the platform specific code isn't that pervasive (like here).

  -Scott

[(NOT FOR CODE REVIEWS), 2011-06-15 16:56:12]

Thanks for the suggestions Tim, Scott.  I'll get around to it later this week.

Roger

-



On Wed, Jun 15, 2011 at 12:44,  <sky@chromium.org> wrote:
> Yes, we generally try to avoid ifdef'tastic code like this. Two suggestions:
>
> . Add a factory, and have a chromeos specific subclass.
> . Remove all the ifdefs from the .h (meaning SigninManager implements
> NotificationObserver). Have the constructor call some method, say
> RegisterObservers. RegisterObservers and and Observe are ifdef'd in the .cc
> to
> do nothing for non-chromeos builds, and create a SigninManager_chromeos.cc
> that
> has the chromeos-specific bits.
>
> The first creates a cleaner separation, but the second is commonly used for
> cases where the platform specific code isn't that pervasive (like here).
>
>  -Scott
>
> http://codereview.chromium.org/7121014/
>

[Roger Tawa OOO till Jul 10th, 2011-06-21 01:03:42]

Hi guys,

Changed code as suggested in Scott's, and made a change to GaiaUrls (and updated
chromeos unit tests).  Please take another look.  Thanks.

[Chris Masone, 2011-06-21 01:26:31]

http://codereview.chromium.org/7121014/diff/33005/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/cookie_fetcher_unittest.cc (right):

http://codereview.chromium.org/7121014/diff/33005/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/cookie_fetcher_unittest.cc:188:
scoped_ptr<URLFetcher> fetcher(handler.Handle(std::string("a"), NULL));
so, the trailing newline is fatal?  Why?

http://codereview.chromium.org/7121014/diff/33005/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/issue_response_handler.cc (right):

http://codereview.chromium.org/7121014/diff/33005/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/issue_response_handler.cc:47: return
base::StringPrintf(kUrlFormat,
Why aren't you using GaiaAuthFetcher::MakeTokenAuthBody() here?

[sky, 2011-06-21 02:46:42]

This approach looks much better. I'm assuming Tim and Chris are doing the actual
reviewing.

  -Scott

http://codereview.chromium.org/7121014/diff/33005/chrome/browser/sync/signin_...
File chrome/browser/sync/signin_manager.h (right):

http://codereview.chromium.org/7121014/diff/33005/chrome/browser/sync/signin_...
chrome/browser/sync/signin_manager.h:83: const NotificationDetails& details);
OVERRIDE

http://codereview.chromium.org/7121014/diff/33005/chrome/browser/sync/signin_...
chrome/browser/sync/signin_manager.h:100: };
DISALLOW_COPY_AND_ASSIGN

[Roger Tawa OOO till Jul 10th, 2011-06-21 03:21:13]

Thanks guys.  Changes uploaded.

http://codereview.chromium.org/7121014/diff/33005/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/cookie_fetcher_unittest.cc (right):

http://codereview.chromium.org/7121014/diff/33005/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/cookie_fetcher_unittest.cc:188:
scoped_ptr<URLFetcher> fetcher(handler.Handle(std::string("a"), NULL));
On 2011/06/21 01:26:31, Chris Masone wrote:
> so, the trailing newline is fatal?  Why?

My bad.  I meant to put it back but forgot.

http://codereview.chromium.org/7121014/diff/33005/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/issue_response_handler.cc (right):

http://codereview.chromium.org/7121014/diff/33005/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/issue_response_handler.cc:47: return
base::StringPrintf(kUrlFormat,
On 2011/06/21 01:26:31, Chris Masone wrote:
> Why aren't you using GaiaAuthFetcher::MakeTokenAuthBody() here?

All the GaiaAuthFetcher::MakeXxxBody() functions are private.  The TokenAuth API
accepts its parameters in the body or in the URL, both ways are equivalent.  I
did not want to disturb this code path, so the code is equivalent to the way it
was previously.

http://codereview.chromium.org/7121014/diff/33005/chrome/browser/sync/signin_...
File chrome/browser/sync/signin_manager.h (right):

http://codereview.chromium.org/7121014/diff/33005/chrome/browser/sync/signin_...
chrome/browser/sync/signin_manager.h:83: const NotificationDetails& details);
On 2011/06/21 02:46:43, sky wrote:
> OVERRIDE

Done.

http://codereview.chromium.org/7121014/diff/33005/chrome/browser/sync/signin_...
chrome/browser/sync/signin_manager.h:100: };
On 2011/06/21 02:46:43, sky wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

[Chris Masone, 2011-06-21 03:39:18]

On 2011/06/21 03:21:13, Roger Tawa wrote:
> Thanks guys.  Changes uploaded.
> 
>
http://codereview.chromium.org/7121014/diff/33005/chrome/browser/chromeos/log...
> File chrome/browser/chromeos/login/cookie_fetcher_unittest.cc (right):
> 
>
http://codereview.chromium.org/7121014/diff/33005/chrome/browser/chromeos/log...
> chrome/browser/chromeos/login/cookie_fetcher_unittest.cc:188:
> scoped_ptr<URLFetcher> fetcher(handler.Handle(std::string("a"), NULL));
> On 2011/06/21 01:26:31, Chris Masone wrote:
> > so, the trailing newline is fatal?  Why?
> 
> My bad.  I meant to put it back but forgot.
> 
>
http://codereview.chromium.org/7121014/diff/33005/chrome/browser/chromeos/log...
> File chrome/browser/chromeos/login/issue_response_handler.cc (right):
> 
>
http://codereview.chromium.org/7121014/diff/33005/chrome/browser/chromeos/log...
> chrome/browser/chromeos/login/issue_response_handler.cc:47: return
> base::StringPrintf(kUrlFormat,
> On 2011/06/21 01:26:31, Chris Masone wrote:
> > Why aren't you using GaiaAuthFetcher::MakeTokenAuthBody() here?
> 
> All the GaiaAuthFetcher::MakeXxxBody() functions are private.  The TokenAuth
API
> accepts its parameters in the body or in the URL, both ways are equivalent.  I
> did not want to disturb this code path, so the code is equivalent to the way
it
> was previously.
> 
>
http://codereview.chromium.org/7121014/diff/33005/chrome/browser/sync/signin_...
> File chrome/browser/sync/signin_manager.h (right):
> 
>
http://codereview.chromium.org/7121014/diff/33005/chrome/browser/sync/signin_...
> chrome/browser/sync/signin_manager.h:83: const NotificationDetails& details);
> On 2011/06/21 02:46:43, sky wrote:
> > OVERRIDE
> 
> Done.
> 
>
http://codereview.chromium.org/7121014/diff/33005/chrome/browser/sync/signin_...
> chrome/browser/sync/signin_manager.h:100: };
> On 2011/06/21 02:46:43, sky wrote:
> > DISALLOW_COPY_AND_ASSIGN
> 
> Done.



*/chromeos/login/* changes LGTM

[tim (not reviewing), 2011-06-21 15:21:39]

LGTM

[(NOT FOR CODE REVIEWS), 2011-06-21 15:33:18]

Thanks Tim.

Roger

-



On Tue, Jun 21, 2011 at 11:21,  <tim@chromium.org> wrote:
> LGTM
>
> http://codereview.chromium.org/7121014/
>

[commit-bot: I haz the power, 2011-06-21 16:39:25]

Change committed as 89842