[sync] Automatic bootstrapping of Sync on Win 8 from cached credentials

[sync] Automatic bootstrapping of Sync on Win 8 from cached credentials

Windows 8 has both a classic "Desktop" mode and a new "Metro" mode, that
must use different browser processes because Chrome profile data stores
can only be accessed from one profile at a time, and it is not
technically feasible to allow multi process access today.

The short term solution to making the experience less jarring on Windows
8 is to cache the sync credentials in a separate file when the user
signs in on either Metro or Desktop, so that they can be used to
auto-start sync on the alternate profile (Desktop or Metro) when it is
opened.

This patch contains the following changes:

- A new class CredentialCacheService, that contains code capable of
  encrypting and persisting the google username, the lsid and sid,
  the encryption bootstrap token, and a list of enabled / disabled sync
  datatypes to a json encoded "Sync Credentials" file in the profile
  directory.
- Methods that listen to the PrefService and TokenService for changes
  to the above credentials and persist them when the user successfully
  signs in manually.
- Code in ProfileSyncService that detects that we are running on Windows
  8, and if sync has never been set up, tries to load cached credentials
  from the alternate Metro / Desktop profile, and if they were found,
  bootstrap sync using the credentials.
- Mechanisms to detect that we are using the "Default" profile on Metro
  or Desktop. As of today, Metro Chrome on Windows 8 does not support
  multiple profiles, and there is no good way to pair up a non-default
  profile directory ("Profile 1", "Profile 2", etc.) on Desktop to its
  corresponding directory on Metro and vice versa.
- Unit tests for functionality in credential_cache_service_win.{h|cc}.

BUG=139199, 139200
TEST=Sign in to sync on Desktop, launch Metro, and watch sync automatically start up

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

[Raghu Simha, 2012-06-26 01:32:05]

Tim/Roger/Drew: Please review, and let me know what you think of this approach.

Drew: Code paths where we decide whether to call
ProfileSyncService::ConsumeCachedPassphraseIfPossible?

Roger: Sign in bootstrap code, centered around
SigninManager::StartSignInWithCachedCredentials.

Tim: Overall review, please!

[Andrew T Wilson (Slow), 2012-06-26 23:26:13]

So, the thing I'd like to see changed is how this logic is threaded through
ProfileSyncService and SigninManager. It seems like we could move this logic
outside those classes entirely - perhaps via a CachedCredentialsService that
listens to signin notifications and notifications from SyncBackendHost and
writes the credentials out. Ideally we wouldn't need to thread things like
setting_up_with_cached_credentials_ throughout ProfileSyncService.

One other question - what happens if one side signs out from sync? Will it get
signed back in via the cached credentials on restart?

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/signin/sign...
File chrome/browser/signin/signin_manager.cc (right):

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager.cc:468: PersistAuthTokensAsync();
I wonder if we need to do this, or if it's better/less coupled for some other
component to just listen for GOOGLE_SIGNIN_SUCCESSFUL and write out the
credentials when that happens?

I'm not sure why SigninManager is the right place for this persistence code.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager.cc:500: password_.clear();
Hmmm. It previously has been illegal to send out SIGNIN_SUCCESSFUL notifications
with an empty password. Are we sure we want to change this?

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/signin/sign...
File chrome/browser/signin/signin_manager.h (right):

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager.h:201: // profile to automatically start up
sync.
What happens if both are already running? Is there polling going on, or some
kind of notification to the other process when one process signs in?

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/creden...
File chrome/browser/sync/credential_cache_win.cc (right):

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_win.cc:144: store->ReadPrefs();
So, if we ever don't have sid/lsid here, that's an error, right? Should we add
some dchecks?

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/creden...
File chrome/browser/sync/credential_cache_win.h (right):

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_win.h:14: #include <string>
std includes go above project-specific includes.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_win.h:60: // BrowserThread, and only for
the "Default" profile. Takes over ownership of
Nobody owns a refcounted ptr, so it's not true that it takes over ownership.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_win.h:67: // for the "Default" profile.
Takes over ownership of |credentials|.
Nobody owns a refcounted object, so this doesn't take over ownership.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/profil...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service.cc:237: if
(ShouldSigninUsingCachedCredentials()) {
What happens if the user is already logged in to a different acct? This seems
like it'd log them out or something.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service.cc:746: if
(!setting_up_with_cached_credentials_)
Don't need this check since ConsumeCachedPassphraseIfPossible() does nothing if
there's no cached passphrase.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service.cc:767: if (HasSyncSetupCompleted() ||
setting_up_with_cached_credentials_) {
Seems like it'd be better to just set HasSyncSetupCompleted() above.

Also, are we concerned that we aren't carrying over the user's data types (i.e.
if I setup sync on desktop for just bookmarks, it'll automatically start syncing
everything else on the other side - passwords, tabs, etc). Seems like a privacy
violation.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service.cc:1554:
!successful->password.empty());
This is kind of ugly, as it means that people outside of sync can't rely on the
password field in this notification.

I'd rather have the signin manager code not send out SIGNIN_SUCCESSFUL at all if
signing in from cached credentials, or else send out some other kind of signin
notification (SIGNIN_SUCCESSFUL_FROM_CACHED_CREDENTIALS) that doesn't have a
password field.

[Raghu Simha, 2012-06-27 00:17:04]

Drew: Some preliminary responses to your comments. Thanks for the very
insightful code review. I'll chat with you offline about some of the upcoming
changes, since there are are a couple of directions we could go.

Tim/Roger: Could you guys let me know what you think of the overall approach?

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/signin/sign...
File chrome/browser/signin/signin_manager.cc (right):

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager.cc:468: PersistAuthTokensAsync();
On 2012/06/26 23:26:13, Andrew T Wilson wrote:
> I wonder if we need to do this, or if it's better/less coupled for some other
> component to just listen for GOOGLE_SIGNIN_SUCCESSFUL and write out the
> credentials when that happens?

I agree with the premise of your suggestion. It turns out that the
GOOGLE_SIGNIN_SUCCESSFUL notification sends out a username and password, instead
of what we use, which is a username, sid and lsid. Do you think it's preferable
to store the gaia password in an encrypted form? Is that not a bigger concern?

> 
> I'm not sure why SigninManager is the right place for this persistence code.

ProfileSyncService doesn't have access to the ClientLoginResult returned to
SigninManager on a successful signin. I'll investigate the approach you
suggested.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager.cc:500: password_.clear();
On 2012/06/26 23:26:13, Andrew T Wilson wrote:
> Hmmm. It previously has been illegal to send out SIGNIN_SUCCESSFUL
notifications
> with an empty password. Are we sure we want to change this?

I had to do this due to a DCHECK. This problem should go away when I add a new
notification type. Coming up.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/signin/sign...
File chrome/browser/signin/signin_manager.h (right):

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager.h:201: // profile to automatically start up
sync.
On 2012/06/26 23:26:13, Andrew T Wilson wrote:
> What happens if both are already running? Is there polling going on, or some
> kind of notification to the other process when one process signs in?

I don't believe there is a mechanism within Chrome by which we can exchange 
messages between Desktop and Metro processes on Win 8. I chatted with Tim, and
it seemed reasonable to do nothing until the second process (that wasn't signed
in) is restarted.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/profil...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service.cc:237: if
(ShouldSigninUsingCachedCredentials()) {
On 2012/06/26 23:26:13, Andrew T Wilson wrote:
> What happens if the user is already logged in to a different acct? This seems
> like it'd log them out or something.

No. In such cases, signin_->GetAuthenticatedUsername().empty() will be
non-empty, and ShouldSigninUsingCachedCredentials() will return false.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service.cc:767: if (HasSyncSetupCompleted() ||
setting_up_with_cached_credentials_) {
On 2012/06/26 23:26:13, Andrew T Wilson wrote:
> Seems like it'd be better to just set HasSyncSetupCompleted() above.
> 
> Also, are we concerned that we aren't carrying over the user's data types
(i.e.
> if I setup sync on desktop for just bookmarks, it'll automatically start
syncing
> everything else on the other side - passwords, tabs, etc). Seems like a
privacy
> violation.

This is an excellent point. I believe I'm going to have to persist not only the
credentials, but also a list of synced types. Which means I'm going to have to
use a better class name than CredentialCache.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service.cc:1554:
!successful->password.empty());
On 2012/06/26 23:26:13, Andrew T Wilson wrote:
> This is kind of ugly, as it means that people outside of sync can't rely on
the
> password field in this notification.
> 
> I'd rather have the signin manager code not send out SIGNIN_SUCCESSFUL at all
if
> signing in from cached credentials, or else send out some other kind of signin
> notification (SIGNIN_SUCCESSFUL_FROM_CACHED_CREDENTIALS) that doesn't have a
> password field.

Agreed. Coming up.

[Roger Tawa OOO till Jul 10th, 2012-06-27 21:23:28]

Hi Raghu, sorry for the late response.  See comments below.  Thanks.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/signin/sign...
File chrome/browser/signin/signin_manager.cc (right):

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager.cc:492:
scoped_refptr<csync::CredentialCache> credentials) {
The order of implementation in the cc file should match the order in the h file,
so this definition should move up.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager.cc:505:
content::Source<TokenService>(token_service));
why do you need to register?  Note that the case for this notif in
SigninManager::Observe() is wrapped in #ifdef !OS_CHROMEOS

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/signin/sign...
File chrome/browser/signin/signin_manager.h (right):

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager.h:117: #endif
generally speaking there should be no #ifdef for platforms inside header files. 
I would not put the #ifdef here.  Put in the cc file, and for no-windows cases
use NOTREACHED().

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/creden...
File chrome/browser/sync/credential_cache_win.cc (right):

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_win.cc:50:
CredentialCache::~CredentialCache() {}
i think the } should go on the next line.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_win.cc:118: !credentials->sid_.empty() &&
should this be indented only 4 spaces?

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_win.cc:141:
credentials->profile_dir_.Append(kSyncCredentialsFilename),
I think it would be better to write a function that builds the  syncCred path
for the default profile, just like GetCredentialPathInAlternateProfile().

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/creden...
File chrome/browser/sync/credential_cache_win.h (right):

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_win.h:29: // cleaning up the object.
I'm not sure why you need this class.  why can't you simply use a
base::DictionaryValue?  Also, I don't see why this needs to be refcounted.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_win.h:30: class CredentialCache : public
base::RefCounted<CredentialCache>{
space before {

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_win.h:49: // Encrypts and base 64 encodes
|plaintext|, converts the result to a
|plaintext| not right.  Document that the caller now owns the StringValue.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_win.h:55: static void
UnpackCredential(const base::Value* packed,
use a const ref?

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_win.h:85: }
i would return const ref return values for all these methods.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_win.h:90: virtual bool
RunningOnCorrectThread() const;
I think the name of this function should say "file thread"

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_win.h:95: virtual bool
RunningInCorrectProfileDir() const;
I think the name of this function should say "default profile".

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:1336: credentials));
why create a csync::CredentialCache instead of just binding the token and path
directly?

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/profil...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service.cc:279: credentials));
why not bind the path directly instead of creating a csync::CredentialCache?

[Raghu Simha, 2012-07-19 06:57:06]

Drew / Roger / Tim:

Sorry about the time it took to update this patch. I put it through a
considerable redesign based on your feedback, and based on the design doc that I
shared last week, that outlines corner cases that we don't want to handle.

The new design, uses a CredentialCacheService that writes credentials to the
cache by listening to notifications from the PrefService and TokenService. On
the flip side, when credentials are successfully loaded, the service updates the
pref and token services, and ProfileSyncService is alerted via a notification
from the token service.

I decided to leave out the polling of the credential files for a separate patch,
and have called it out as a TODO.

Let me know what you think.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/signin/sign...
File chrome/browser/signin/signin_manager.cc (right):

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager.cc:468: PersistAuthTokensAsync();
On 2012/06/26 23:26:13, Andrew T Wilson wrote:
> I wonder if we need to do this, or if it's better/less coupled for some other
> component to just listen for GOOGLE_SIGNIN_SUCCESSFUL and write out the
> credentials when that happens?
> 
> I'm not sure why SigninManager is the right place for this persistence code.

Done. All code removed from SigninManager.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager.cc:492:
scoped_refptr<csync::CredentialCache> credentials) {
On 2012/06/27 21:23:28, Roger Tawa wrote:
> The order of implementation in the cc file should match the order in the h
file,
> so this definition should move up.

Moot for this file, but I've followed this rule in other files.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager.cc:505:
content::Source<TokenService>(token_service));
On 2012/06/27 21:23:28, Roger Tawa wrote:
> why do you need to register?  Note that the case for this notif in
> SigninManager::Observe() is wrapped in #ifdef !OS_CHROMEOS

We no longer do this.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/signin/sign...
File chrome/browser/signin/signin_manager.h (right):

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/signin/sign...
chrome/browser/signin/signin_manager.h:117: #endif
On 2012/06/27 21:23:28, Roger Tawa wrote:
> generally speaking there should be no #ifdef for platforms inside header
files. 
> I would not put the #ifdef here.  Put in the cc file, and for no-windows cases
> use NOTREACHED().

Done.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/creden...
File chrome/browser/sync/credential_cache_win.cc (right):

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_win.cc:50:
CredentialCache::~CredentialCache() {}
On 2012/06/27 21:23:28, Roger Tawa wrote:
> i think the } should go on the next line.

Done.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_win.cc:118: !credentials->sid_.empty() &&
On 2012/06/27 21:23:28, Roger Tawa wrote:
> should this be indented only 4 spaces?

This method has been removed.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_win.cc:141:
credentials->profile_dir_.Append(kSyncCredentialsFilename),
On 2012/06/27 21:23:28, Roger Tawa wrote:
> I think it would be better to write a function that builds the  syncCred path
> for the default profile, just like GetCredentialPathInAlternateProfile().

Agree. Done.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_win.cc:144: store->ReadPrefs();
On 2012/06/26 23:26:13, Andrew T Wilson wrote:
> So, if we ever don't have sid/lsid here, that's an error, right? Should we add
> some dchecks?

This method has been removed.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/creden...
File chrome/browser/sync/credential_cache_win.h (right):

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_win.h:14: #include <string>
On 2012/06/26 23:26:13, Andrew T Wilson wrote:
> std includes go above project-specific includes.

Done.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_win.h:29: // cleaning up the object.
On 2012/06/27 21:23:28, Roger Tawa wrote:
> I'm not sure why you need this class.  why can't you simply use a
> base::DictionaryValue?  Also, I don't see why this needs to be refcounted.

The design has changed pretty significantly. Please take a look at the new
implementation.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_win.h:49: // Encrypts and base 64 encodes
|plaintext|, converts the result to a
On 2012/06/27 21:23:28, Roger Tawa wrote:
> |plaintext| not right.  Document that the caller now owns the StringValue.

Done.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_win.h:55: static void
UnpackCredential(const base::Value* packed,
On 2012/06/27 21:23:28, Roger Tawa wrote:
> use a const ref?

Done.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_win.h:60: // BrowserThread, and only for
the "Default" profile. Takes over ownership of
On 2012/06/26 23:26:13, Andrew T Wilson wrote:
> Nobody owns a refcounted ptr, so it's not true that it takes over ownership.

This method has been removed.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_win.h:67: // for the "Default" profile.
Takes over ownership of |credentials|.
On 2012/06/26 23:26:13, Andrew T Wilson wrote:
> Nobody owns a refcounted object, so this doesn't take over ownership.

This method has been removed.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_win.h:85: }
On 2012/06/27 21:23:28, Roger Tawa wrote:
> i would return const ref return values for all these methods.

These methods have been removed.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_win.h:90: virtual bool
RunningOnCorrectThread() const;
On 2012/06/27 21:23:28, Roger Tawa wrote:
> I think the name of this function should say "file thread"

This method has been removed.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_win.h:95: virtual bool
RunningInCorrectProfileDir() const;
On 2012/06/27 21:23:28, Roger Tawa wrote:
> I think the name of this function should say "default profile".

This method has been removed.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:1336: credentials));
On 2012/06/27 21:23:28, Roger Tawa wrote:
> why create a csync::CredentialCache instead of just binding the token and path
> directly?

We now create a CredentialCacheService using a CredentialCacheServiceFactory.
Should be a lot cleaner.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/profil...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service.cc:279: credentials));
On 2012/06/27 21:23:28, Roger Tawa wrote:
> why not bind the path directly instead of creating a csync::CredentialCache?

We now use a CredentialCacheService to do this.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service.cc:746: if
(!setting_up_with_cached_credentials_)
On 2012/06/26 23:26:13, Andrew T Wilson wrote:
> Don't need this check since ConsumeCachedPassphraseIfPossible() does nothing
if
> there's no cached passphrase.

Removed.

http://codereview.chromium.org/10656033/diff/16001/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service.cc:1554:
!successful->password.empty());
On 2012/06/26 23:26:13, Andrew T Wilson wrote:
> This is kind of ugly, as it means that people outside of sync can't rely on
the
> password field in this notification.
> 
> I'd rather have the signin manager code not send out SIGNIN_SUCCESSFUL at all
if
> signing in from cached credentials, or else send out some other kind of signin
> notification (SIGNIN_SUCCESSFUL_FROM_CACHED_CREDENTIALS) that doesn't have a
> password field.

Concerns addressed in new design, I hope :)

[Andrew T Wilson (Slow), 2012-07-20 01:04:04]

I like the direction this has taken - I have a few concerns about us only
syncing the sync token, and also I think the CCS can be decoupled a bit more
from the PSS, but it's looking pretty good.

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/signin/toke...
File chrome/browser/signin/token_service.h (right):

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/signin/toke...
chrome/browser/signin/token_service.h:157: void SaveCachedSyncTokenToDB(const
std::string& sync_token);
I think the intended way for external components to provide tokens to the token
DB is via NOTIFICATION_TOKEN_UPDATED rather than through APIs like this.

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/sync/creden...
File chrome/browser/sync/credential_cache_service_factory_win.h (right):

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_factory_win.h:15: class
CredentialCacheService;
Just out of curiosity, why is CredentialCacheService within the syncer
namespace, but not this class?

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/sync/creden...
File chrome/browser/sync/credential_cache_service_win.cc (right):

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.cc:43: if (profile_)
When is profile_ null? Is this just for unit tests? Maybe add a comment to that
effect?

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.cc:51: void
CredentialCacheService::Shutdown() {
I thought the point of using a pref_registrar_, etc was that you didn't have to
manually do all of this invalidation (it is handled by the destructor
automatically). Is there a reason you are manually calling RemoveAll() and
InvalidateWeakPtrs()?

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.cc:86: return NULL;
Do you really want to return NULL from this routine on error, or do you want to
return an empty string?

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.cc:103: return "";
Why are we returning "" here instead of std::string() as above?

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.cc:112: std::string unpacked;
nit:Should this be called "unencrypted" not "unpacked" since technically the
encrypted string is already unpacked?

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.cc:226: bool*
should_initialize) {
Not clear why should_initialize is a pointer, since it doesn't look like we
write to it (i.e. it's not an out param).

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.cc:399:
UpdateStringPref(kSyncServiceToken, token_details.token());
I'm still not clear on why we are only syncing the sync service token, and not
the other tokens - how will the user turn on other signed-in services?

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/sync/creden...
File chrome/browser/sync/credential_cache_service_win.h (right):

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.h:37: // updates to the pref
service and token service that pertain to the user
token/pref service -> TokenService/PrefService since those are actual class
names.

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.h:166:
content::NotificationRegistrar token_service_registrar_;
I'd just call this "registrar_" since it's a general purpose registrar and
that's the convention in chrome, even for registrars used to listen to
notifications from a single source.

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/sync/profil...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service.cc:220: base::win::GetVersion() >=
base::win::VERSION_WIN8 &&
This seems like a non-optimal pattern - why not just have this routine only be
defined on OS_WIN (why do we need it on other platforms at all?).

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service.cc:241: if
(ShouldInitializeCredentialCacheService()) {
I'm not clear on why we need to do this initialization - why doesn't the
CredentialCacheService self-initialize on startup?

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service.cc:740: LoadedCachedCredentials()) {
I think a better way to do this would be for the CCS to set
SyncSetupCompleted=true when it loads cached credentials, then we don't need
this logic here.

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/sync/profil...
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service.h:520: return sync_prefs_.AsWeakPtr();
In the past, people who need to access SyncPrefs have just instantiated new
instances. Is there a reason we need to expose our internal instance here?

[Raghu Simha, 2012-07-20 23:35:22]

Thanks for all the feedback Drew. I've done more decoupling -- let me know what
you think.

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/signin/toke...
File chrome/browser/signin/token_service.h (right):

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/signin/toke...
chrome/browser/signin/token_service.h:157: void SaveCachedSyncTokenToDB(const
std::string& sync_token);
On 2012/07/20 01:04:05, Andrew T Wilson wrote:
> I think the intended way for external components to provide tokens to the
token
> DB is via NOTIFICATION_TOKEN_UPDATED rather than through APIs like this.

Agree. This method is no longer required.

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/sync/creden...
File chrome/browser/sync/credential_cache_service_factory_win.h (right):

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_factory_win.h:15: class
CredentialCacheService;
On 2012/07/20 01:04:05, Andrew T Wilson wrote:
> Just out of curiosity, why is CredentialCacheService within the syncer
> namespace, but not this class?

I was using the example set by ProfileSyncServiceFactory. Good point though.
I've put the factory into namespace syncer.

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/sync/creden...
File chrome/browser/sync/credential_cache_service_win.cc (right):

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.cc:43: if (profile_)
On 2012/07/20 01:04:05, Andrew T Wilson wrote:
> When is profile_ null? Is this just for unit tests? Maybe add a comment to
that
> effect?

Done.

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.cc:51: void
CredentialCacheService::Shutdown() {
On 2012/07/20 01:04:05, Andrew T Wilson wrote:
> I thought the point of using a pref_registrar_, etc was that you didn't have
to
> manually do all of this invalidation (it is handled by the destructor
> automatically). Is there a reason you are manually calling RemoveAll() and
> InvalidateWeakPtrs()?

Not really. Removed.

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.cc:86: return NULL;
On 2012/07/20 01:04:05, Andrew T Wilson wrote:
> Do you really want to return NULL from this routine on error, or do you want
to
> return an empty string?

Good catch. Empty string is good.

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.cc:103: return "";
On 2012/07/20 01:04:05, Andrew T Wilson wrote:
> Why are we returning "" here instead of std::string() as above?

Good catch. Fixed.

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.cc:112: std::string unpacked;
On 2012/07/20 01:04:05, Andrew T Wilson wrote:
> nit:Should this be called "unencrypted" not "unpacked" since technically the
> encrypted string is already unpacked?

Done.

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.cc:226: bool*
should_initialize) {
On 2012/07/20 01:04:05, Andrew T Wilson wrote:
> Not clear why should_initialize is a pointer, since it doesn't look like we
> write to it (i.e. it's not an out param).

This is so that base::Owned can clean up the bool* created by
LookForCachedCredentialsInAlternateProfile. Also, we can't make do with a local
bool variable and pass in its address to ShouldReadFromAlternateCache because
it's run on the file thread.

I've added a comment to make this clear.

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.cc:399:
UpdateStringPref(kSyncServiceToken, token_details.token());
On 2012/07/20 01:04:05, Andrew T Wilson wrote:
> I'm still not clear on why we are only syncing the sync service token, and not
> the other tokens - how will the user turn on other signed-in services?

Good point re: tokens for other services. I've gone back to using lsid and sid.

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/sync/creden...
File chrome/browser/sync/credential_cache_service_win.h (right):

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.h:37: // updates to the pref
service and token service that pertain to the user
On 2012/07/20 01:04:05, Andrew T Wilson wrote:
> token/pref service -> TokenService/PrefService since those are actual class
> names.

Done.

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.h:166:
content::NotificationRegistrar token_service_registrar_;
On 2012/07/20 01:04:05, Andrew T Wilson wrote:
> I'd just call this "registrar_" since it's a general purpose registrar and
> that's the convention in chrome, even for registrars used to listen to
> notifications from a single source.

Done.

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/sync/profil...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service.cc:220: base::win::GetVersion() >=
base::win::VERSION_WIN8 &&
On 2012/07/20 01:04:05, Andrew T Wilson wrote:
> This seems like a non-optimal pattern - why not just have this routine only be
> defined on OS_WIN (why do we need it on other platforms at all?).

I did this based on Roger's comment that we shouldn't conditionally define
methods for only some datatypes. But on taking a look around the codebase, I
notice other places where this is done. I've cleaned things up a little bit.

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service.cc:241: if
(ShouldInitializeCredentialCacheService()) {
On 2012/07/20 01:04:05, Andrew T Wilson wrote:
> I'm not clear on why we need to do this initialization - why doesn't the
> CredentialCacheService self-initialize on startup?

At first, I thought we would not initialize the credential cache service if sync
was disabled or suppressed. I suppose this logic lives in the PSS startup code
anyway, so I might as well move the initialization there.

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service.cc:740: LoadedCachedCredentials()) {
On 2012/07/20 01:04:05, Andrew T Wilson wrote:
> I think a better way to do this would be for the CCS to set
> SyncSetupCompleted=true when it loads cached credentials, then we don't need
> this logic here.

Agreed. Done.

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/sync/profil...
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/10656033/diff/49052/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service.h:520: return sync_prefs_.AsWeakPtr();
On 2012/07/20 01:04:05, Andrew T Wilson wrote:
> In the past, people who need to access SyncPrefs have just instantiated new
> instances. Is there a reason we need to expose our internal instance here?

I was under the (false) impression that only one SyncPrefs object could safely
be created. I've changed this to a local variable in CredentialCacheService.

[Raghu Simha, 2012-07-21 01:15:45]

http://codereview.chromium.org/10656033/diff/37024/chrome/browser/signin/toke...
File chrome/browser/signin/token_service.cc (right):

http://codereview.chromium.org/10656033/diff/37024/chrome/browser/signin/toke...
chrome/browser/signin/token_service.cc:127: // Notify the CredentialCacheService
that a new lsid and sid are available.
FYI: This code is causing failures in the TokenServiceTests due to the
additional NOTIFICATION_TOKEN_AVAILABLEs that get fired. I think the right way
to fix this is to send out a different kind of notification, like say,
NOTIFICATION_CREDENTIALS_UPDATED. Working on a fix.

[Raghu Simha, 2012-07-21 02:00:55]

http://codereview.chromium.org/10656033/diff/37024/chrome/browser/signin/toke...
File chrome/browser/signin/token_service.cc (right):

http://codereview.chromium.org/10656033/diff/37024/chrome/browser/signin/toke...
chrome/browser/signin/token_service.cc:127: // Notify the CredentialCacheService
that a new lsid and sid are available.
On 2012/07/21 01:15:46, rsimha wrote:
> FYI: This code is causing failures in the TokenServiceTests due to the
> additional NOTIFICATION_TOKEN_AVAILABLEs that get fired. I think the right way
> to fix this is to send out a different kind of notification, like say,
> NOTIFICATION_CREDENTIALS_UPDATED. Working on a fix.

Fixed in Patch set 5.

[Roger Tawa OOO till Jul 10th, 2012-07-23 15:10:33]

Looks good Raghu.  A few questions below.

http://r879-cab51e0eb6a4-rogerta.chromiumcodereview-hr.appspot.com/10656033/d...
File chrome/browser/signin/token_service.cc (right):

http://r879-cab51e0eb6a4-rogerta.chromiumcodereview-hr.appspot.com/10656033/d...
chrome/browser/signin/token_service.cc:240: content::Details<const
TokenAvailableDetails>(&details));
why not put both tokens in TokenAvailableDetails and fire only one notif?  Are
there cases where we have one and not the other?

http://r879-cab51e0eb6a4-rogerta.chromiumcodereview-hr.appspot.com/10656033/d...
File chrome/browser/sync/credential_cache_service_win.cc (right):

http://r879-cab51e0eb6a4-rogerta.chromiumcodereview-hr.appspot.com/10656033/d...
chrome/browser/sync/credential_cache_service_win.cc:56:
alternate_store_->RemoveObserver(this);
Should null out the pointers after dealing with them.  Makes Shutdown() safer to
call multiple times should it happen.

http://r879-cab51e0eb6a4-rogerta.chromiumcodereview-hr.appspot.com/10656033/d...
chrome/browser/sync/credential_cache_service_win.cc:127:
ReadCachedCredentialsFromAlternateProfile();
Could this happen twice if local store notifies after the alternate store? 
Since you don't do anything with local store notifs, why do you need to observe
it?

http://r879-cab51e0eb6a4-rogerta.chromiumcodereview-hr.appspot.com/10656033/d...
chrome/browser/sync/credential_cache_service_win.cc:244: std::string
CredentialCacheService::GetStringPref(
nit: would it be clearer if this was called GetAndUnpackStringPref?

http://r879-cab51e0eb6a4-rogerta.chromiumcodereview-hr.appspot.com/10656033/d...
File chrome/browser/sync/credential_cache_service_win.h (right):

http://r879-cab51e0eb6a4-rogerta.chromiumcodereview-hr.appspot.com/10656033/d...
chrome/browser/sync/credential_cache_service_win.h:34: // updates to the
PrefService and TokenSerervice that pertain to the user
Serervice -> Service

http://r879-cab51e0eb6a4-rogerta.chromiumcodereview-hr.appspot.com/10656033/d...
chrome/browser/sync/credential_cache_service_win.h:69: void
LookForCachedCredentialsInAlternateProfile();
I think this is the only methods that needs to be public.  The rest can be moved
into the private section (or protected if needed by tests).

http://r879-cab51e0eb6a4-rogerta.chromiumcodereview-hr.appspot.com/10656033/d...
chrome/browser/sync/credential_cache_service_win.h:115:
scoped_refptr<JsonPrefStore> local_store_;
should move data member to private section and add an accessor method which is
protected.

http://r879-cab51e0eb6a4-rogerta.chromiumcodereview-hr.appspot.com/10656033/d...
File chrome/browser/sync/profile_sync_service.cc (right):

http://r879-cab51e0eb6a4-rogerta.chromiumcodereview-hr.appspot.com/10656033/d...
chrome/browser/sync/profile_sync_service.cc:230: if
(!IsSyncEnabledAndLoggedIn()) {
Is this right condition?  For example, if IsStartSuppressed() is true, then
IsSyncEnabledAndLoggedIn() returns false, in which case you call
LookForCachedCredentialsInAlternateProfile().

http://r879-cab51e0eb6a4-rogerta.chromiumcodereview-hr.appspot.com/10656033/d...
File chrome/browser/sync/sync_prefs.cc (right):

http://r879-cab51e0eb6a4-rogerta.chromiumcodereview-hr.appspot.com/10656033/d...
chrome/browser/sync/sync_prefs.cc:280: }
I think this method should be moved to match order in header file.

[Raghu Simha, 2012-07-23 20:35:54]

Thanks for the feedback, Roger. PTAL.

http://codereview.chromium.org/10656033/diff/49057/chrome/browser/signin/toke...
File chrome/browser/signin/token_service.cc (right):

http://codereview.chromium.org/10656033/diff/49057/chrome/browser/signin/toke...
chrome/browser/signin/token_service.cc:240: content::Details<const
TokenAvailableDetails>(&details));
On 2012/07/23 15:10:33, Roger Tawa wrote:
> why not put both tokens in TokenAvailableDetails and fire only one notif?  Are
> there cases where we have one and not the other?

That's because TokenAvailableDetails has only one field for service and token.
Notifications for other services get sent out separately too, even though they
may be generated at the same spot.

http://codereview.chromium.org/10656033/diff/49057/chrome/browser/sync/creden...
File chrome/browser/sync/credential_cache_service_win.cc (right):

http://codereview.chromium.org/10656033/diff/49057/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.cc:56:
alternate_store_->RemoveObserver(this);
On 2012/07/23 15:10:33, Roger Tawa wrote:
> Should null out the pointers after dealing with them.  Makes Shutdown() safer
to
> call multiple times should it happen.

Good point. Done.

http://codereview.chromium.org/10656033/diff/49057/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.cc:127:
ReadCachedCredentialsFromAlternateProfile();
On 2012/07/23 15:10:33, Roger Tawa wrote:
> Could this happen twice if local store notifies after the alternate store? 
> Since you don't do anything with local store notifs, why do you need to
observe
> it?

Excellent point. This wasn't a problem during testing because the local store is
always initialized first. But I agree with your suggestion. We no longer observe
the local store.

http://codereview.chromium.org/10656033/diff/49057/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.cc:244: std::string
CredentialCacheService::GetStringPref(
On 2012/07/23 15:10:33, Roger Tawa wrote:
> nit: would it be clearer if this was called GetAndUnpackStringPref?

If we do this, we'll also have to rename UpdateStringPref to
PackAndUpdateStringPref. Both changes made.

http://codereview.chromium.org/10656033/diff/49057/chrome/browser/sync/creden...
File chrome/browser/sync/credential_cache_service_win.h (right):

http://codereview.chromium.org/10656033/diff/49057/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.h:34: // updates to the
PrefService and TokenSerervice that pertain to the user
On 2012/07/23 15:10:33, Roger Tawa wrote:
> Serervice -> Service

Done.

http://codereview.chromium.org/10656033/diff/49057/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.h:69: void
LookForCachedCredentialsInAlternateProfile();
On 2012/07/23 15:10:33, Roger Tawa wrote:
> I think this is the only methods that needs to be public.  The rest can be
moved
> into the private section (or protected if needed by tests).

Good point, but LookForCachedCredentialsInAlternateProfile(),
IsDefaultProfileDir(), Shutdown(), OnInitializationCompleted(),
OnPrefValueChanged() and Observe() all need to be public. I've made the other
methods private / protected, and rearranged them in the cc file to match
declaration order.

http://codereview.chromium.org/10656033/diff/49057/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.h:115:
scoped_refptr<JsonPrefStore> local_store_;
On 2012/07/23 15:10:33, Roger Tawa wrote:
> should move data member to private section and add an accessor method which is
> protected.

Done.

http://codereview.chromium.org/10656033/diff/49057/chrome/browser/sync/profil...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/10656033/diff/49057/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service.cc:230: if
(!IsSyncEnabledAndLoggedIn()) {
On 2012/07/23 15:10:33, Roger Tawa wrote:
> Is this right condition?  For example, if IsStartSuppressed() is true, then
> IsSyncEnabledAndLoggedIn() returns false, in which case you call
> LookForCachedCredentialsInAlternateProfile().

I believe it is the right condition. We do make sure that IsStartSuppressed()
returns false in Initialize() before initializing the CredentialCacheService.
The only other place where TryStart is called is in UnsuppressAndStart(), where
IsStartSuppressed is guaranteed to be false.

http://codereview.chromium.org/10656033/diff/49057/chrome/browser/sync/sync_p...
File chrome/browser/sync/sync_prefs.cc (right):

http://codereview.chromium.org/10656033/diff/49057/chrome/browser/sync/sync_p...
chrome/browser/sync/sync_prefs.cc:280: }
On 2012/07/23 15:10:33, Roger Tawa wrote:
> I think this method should be moved to match order in header file.

Done.

[Roger Tawa OOO till Jul 10th, 2012-07-24 14:13:42]

Awesome Raghu, lgtm

http://r879-cab51e0eb6a4-rogerta.chromiumcodereview-hr.appspot.com/10656033/d...
File chrome/browser/sync/credential_cache_service_win.h (right):

http://r879-cab51e0eb6a4-rogerta.chromiumcodereview-hr.appspot.com/10656033/d...
chrome/browser/sync/credential_cache_service_win.h:102:
scoped_refptr<JsonPrefStore> local_store() const { return local_store_; }
should this be a const ref ?

[Raghu Simha, 2012-07-24 17:19:55]

Thanks Roger.

Drew, let me know if you have any more comments.

http://codereview.chromium.org/10656033/diff/39076/chrome/browser/sync/creden...
File chrome/browser/sync/credential_cache_service_win.h (right):

http://codereview.chromium.org/10656033/diff/39076/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.h:102:
scoped_refptr<JsonPrefStore> local_store() const { return local_store_; }
On 2012/07/24 14:13:42, Roger Tawa wrote:
> should this be a const ref ?

Done.

[Andrew T Wilson (Slow), 2012-07-24 18:00:33]

http://codereview.chromium.org/10656033/diff/38028/chrome/browser/signin/toke...
File chrome/browser/signin/token_service.cc (right):

http://codereview.chromium.org/10656033/diff/38028/chrome/browser/signin/toke...
chrome/browser/signin/token_service.cc:240: content::Details<const
TokenAvailableDetails>(&details));
Either we should just send out TOKEN_AVAILABLE here, or you should create a new
details structure that contains both sid and lsid. Having a separate
notification that uses TokenAvailableDetails is clunky.

http://codereview.chromium.org/10656033/diff/38028/chrome/browser/sync/profil...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/10656033/diff/38028/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service.cc:218: if (base::win::GetVersion() >=
base::win::VERSION_WIN8 &&
I guess I still don't understand why PSS needs to do all this stuff - why
doesn't CredentialCacheService handle all of this (logging in the user, etc)
without requiring Sync to explicitly force its creation, and explicitly call
LookForCachedCredentialsInAlternateProfile()?

You now have this explicit dependency between PSS and CCS, and seemingly the
only reason is so that the CCS doesn't have to check IsStartSuppressed(). I
think this change is much cleaner if the CCS self initializes and doesn't
require PSS to call into it.

[Raghu Simha, 2012-07-24 20:45:03]

Thanks for the review, Drew. PTAL.

Tim: Could you look at the change to profile_impl.cc? Thanks.

http://codereview.chromium.org/10656033/diff/38028/chrome/browser/signin/toke...
File chrome/browser/signin/token_service.cc (right):

http://codereview.chromium.org/10656033/diff/38028/chrome/browser/signin/toke...
chrome/browser/signin/token_service.cc:240: content::Details<const
TokenAvailableDetails>(&details));
On 2012/07/24 18:00:33, Andrew T Wilson wrote:
> Either we should just send out TOKEN_AVAILABLE here, or you should create a
new
> details structure that contains both sid and lsid. Having a separate
> notification that uses TokenAvailableDetails is clunky.

Added a new detail type for updated credentials. We only need to send out one
notification now. Done.

http://codereview.chromium.org/10656033/diff/38028/chrome/browser/sync/profil...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/10656033/diff/38028/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service.cc:218: if (base::win::GetVersion() >=
base::win::VERSION_WIN8 &&
On 2012/07/24 18:00:33, Andrew T Wilson wrote:
> I guess I still don't understand why PSS needs to do all this stuff - why
> doesn't CredentialCacheService handle all of this (logging in the user, etc)
> without requiring Sync to explicitly force its creation, and explicitly call
> LookForCachedCredentialsInAlternateProfile()?
> 
> You now have this explicit dependency between PSS and CCS, and seemingly the
> only reason is so that the CCS doesn't have to check IsStartSuppressed(). I
> think this change is much cleaner if the CCS self initializes and doesn't
> require PSS to call into it.

I see what you mean. I originally coupled PSS and CCS because the primary
purpose of CCS was to enable sync if possible. I've moved the "should we
initialize CCS" logic to within CCS. We now (lazily) instantiate CCS at the same
spot where PSS is instantiated, in profile_impl.cc.

[Andrew T Wilson (Slow), 2012-07-24 20:55:29]

http://codereview.chromium.org/10656033/diff/37044/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/10656033/diff/37044/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:863:
syncer::CredentialCacheServiceFactory::GetInstance()->GetForProfile(this);
I think the cleaner way to do this is to have CredentialCacheServiceFactory
return true from ServiceIsCreatedWithProfile() - then it just gets created on
startup.

http://codereview.chromium.org/10656033/diff/37044/chrome/browser/sync/creden...
File chrome/browser/sync/credential_cache_service_win.cc (right):

http://codereview.chromium.org/10656033/diff/37044/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.cc:258: IsDefaultProfile();
So, this is fine, but another way to do it would be to have your CCSFactory
return NULL from BuildServiceInstanceFor() if the profile is not the default
profile. Then your service object would only ever be created for default
profiles.

[Andrew T Wilson (Slow), 2012-07-24 20:57:41]

http://codereview.chromium.org/10656033/diff/37044/chrome/browser/signin/toke...
File chrome/browser/signin/token_service.h (right):

http://codereview.chromium.org/10656033/diff/37044/chrome/browser/signin/toke...
chrome/browser/signin/token_service.h:99: CredentialsUpdatedDetails() {}
Do we need this default constructor? Seems illegal to create one of these with
no sid/lsid.

[Raghu Simha, 2012-07-24 22:17:35]

Thanks Drew. PTAL.

http://codereview.chromium.org/10656033/diff/37044/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/10656033/diff/37044/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:863:
syncer::CredentialCacheServiceFactory::GetInstance()->GetForProfile(this);
On 2012/07/24 20:55:29, Andrew T Wilson wrote:
> I think the cleaner way to do this is to have CredentialCacheServiceFactory
> return true from ServiceIsCreatedWithProfile() - then it just gets created on
> startup. 

Done.

http://codereview.chromium.org/10656033/diff/37044/chrome/browser/signin/toke...
File chrome/browser/signin/token_service.h (right):

http://codereview.chromium.org/10656033/diff/37044/chrome/browser/signin/toke...
chrome/browser/signin/token_service.h:99: CredentialsUpdatedDetails() {}
On 2012/07/24 20:57:41, Andrew T Wilson wrote:
> Do we need this default constructor? Seems illegal to create one of these with
> no sid/lsid.

Removed.

http://codereview.chromium.org/10656033/diff/37044/chrome/browser/sync/creden...
File chrome/browser/sync/credential_cache_service_win.cc (right):

http://codereview.chromium.org/10656033/diff/37044/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_win.cc:258: IsDefaultProfile();
On 2012/07/24 20:55:29, Andrew T Wilson wrote:
> So, this is fine, but another way to do it would be to have your CCSFactory
> return NULL from BuildServiceInstanceFor() if the profile is not the default
> profile. Then your service object would only ever be created for default
> profiles.

Done. (Unnecessary methods removed).

[tim (not reviewing), 2012-07-24 23:49:25]

LGTM, but I'm not actually an OWNER for profiles/ (not sure why the presubmit
script is recommending me!) +erg

[Elliot Glaysher, 2012-07-24 23:59:32]

lgtm

http://codereview.chromium.org/10656033/diff/51042/chrome/browser/sync/creden...
File chrome/browser/sync/credential_cache_service_factory_win.cc (right):

http://codereview.chromium.org/10656033/diff/51042/chrome/browser/sync/creden...
chrome/browser/sync/credential_cache_service_factory_win.cc:36: //
DependsOn(PrefServiceFactory::GetInstance());
Thank you for adding these todos. Some people forget.

[Raghu Simha, 2012-07-26 01:04:48]

+sky

Looks like I still need owners approval for chrome/chrome_browser.gypi and
chrome/chrome_tests.gypi. I picked a name form the OWNERS hat, and it had your
name on it :)

[Andrew T Wilson (Slow), 2012-07-26 20:28:07]

LGTM!

[jam, 2012-07-26 21:11:17]

lgtm