suppress user/password dialog when re-enabling sync

On ChromeOS, the user is always signed in and we should not
show user/password dialog when re-enabling sync.

BUG=chromium-os:27955, chromium-os:27956, chromium:121563
TEST=manual, unit_tests --gtest_filter="*Sync*"

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

[Andrew T Wilson (Slow), 2012-04-03 17:45:13]

Looks like you're mostly on the right track on the UI side. The bootstrap token
piece needs some work though.

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/signi...
File chrome/browser/signin/signin_manager.h (right):

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/signi...
chrome/browser/signin/signin_manager.h:132: const std::string&
convenient_password() const { return convenient_password_; }
nit: line too long

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/signi...
File chrome/browser/signin/signin_tracker.cc (right):

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/signi...
chrome/browser/signin/signin_tracker.cc:28: state_(WAITING_FOR_GAIA_VALIDATION),
I think it would be better to have an alternate constructor that lets you set
the initial state. That way, you can run tests for this case on any platform.

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/signi...
chrome/browser/signin/signin_tracker.cc:39: #if !defined(OS_CHROMEOS)
I'd say you should probably just register for all these notifications even if
you don't need them on cros. For example, you don't care about the
TOKEN_AVAILABLE/REQUEST_FAILED notifications either.

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/signi...
chrome/browser/signin/signin_tracker.cc:62: DCHECK_EQ(state_,
SERVICES_INITIALIZING);
When you change the constructor to allow setting the intitial state, I'd change
this to "if (state_ == SERVICES_INITIALIZING) HandleServiceStateChange();" - no
need to make it cros only,

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/signi...
chrome/browser/signin/signin_tracker.cc:143: LOG(WARNING) <<
"HandleServiceStateChange: service is waiting for auth";
BTW, it might be useful to keep some of these messages around as DVLOG(), so
feel free to do that if you find them useful (I've had to add similar messages
in the past, so it might be something we want to keep).

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/signi...
File chrome/browser/signin/signin_tracker_unittest.cc (right):

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/signi...
chrome/browser/signin/signin_tracker_unittest.cc:229: #if !defined(OS_CHROMEOS)
If you make the signin tracker so you can inject the starting state via the
constructor, you won't need to disable this test. And you'll be able to create a
test for the chromeos flow that will run on every platform.

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/sync/...
File chrome/browser/sync/sync_prefs.cc (right):

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/sync/...
chrome/browser/sync/sync_prefs.cc:53:
pref_service_->ClearPref(prefs::kSyncEncryptionBootstrapToken);
Not sure this is right. We should be maintaining the GAIA bootstrap token in a
separate pref and using that when re-enabling sync, rather than maintaining the
old bootstrap token which might contain a token related to a custom passphrase).

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/ui/we...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/ui/we...
chrome/browser/ui/webui/sync_setup_handler.cc:603: #endif
We should probably close the overlay here though, to make sure the spinner is
hidden?

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/ui/we...
chrome/browser/ui/webui/sync_setup_handler.cc:627: DisplayConfigureSync(true,
false);
Are you sure that's what we want here (not the "Sync Everything" button
instead)?

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/ui/we...
chrome/browser/ui/webui/sync_setup_handler.cc:827: browser_sync::SyncPrefs
sync_prefs(GetProfile()->GetPrefs());
Should probably just move this into a helper function.

Also, if you never want to display a GAIA login on cros, you should probably #if
it out of the header and .cc file to make sure calls don't sneak back in.

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/ui/we...
chrome/browser/ui/webui/sync_setup_handler.cc:831: signin_tracker_.reset(new
SigninTracker(GetProfile(), this));
We don't need a signin tracker in the case that sync is already initialized.

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/ui/we...
chrome/browser/ui/webui/sync_setup_handler.cc:836:
token_service->StartFetchingTokens();
Shouldn't need to do this once you remove SigninManager::SignOut() on cros.

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/ui/we...
chrome/browser/ui/webui/sync_setup_handler.cc:842:
service->set_setup_in_progress(true);
This looks good. In fact, we might want to have this in general (not just for
os_chromeos) in case someone tries to bring up the configure sync dialog before
the backend is initialized.

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/ui/we...
File chrome/browser/ui/webui/sync_setup_handler.h (right):

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/ui/we...
chrome/browser/ui/webui/sync_setup_handler.h:127: // is running in the
background.
Note: http://code.google.com/p/chromium/issues/detail?id=121563

You should probably include that bug number in your CL BUG= description.

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/ui/we...
File chrome/browser/ui/webui/sync_setup_handler_unittest.cc (right):

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/ui/we...
chrome/browser/ui/webui/sync_setup_handler_unittest.cc:424: #if
!defined(OS_CHROMEOS)
I think we'll still need some flavor of this test, since even on cros you can
get an unrecoverable error while starting up sync.

[Nicolas Zea, 2012-04-03 21:00:33]

http://codereview.chromium.org/9956097/diff/2008/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/9956097/diff/2008/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:407: CreateBackend();
Right before this is where you should probably fill the normal encryption
bootstrap token from the spare (but only if it's normal is empty).

http://codereview.chromium.org/9956097/diff/2008/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:1095: browser_sync::Cryptographer*
cryptographer = trans.GetCryptographer();
This is using the sync's internal cryptographer. Instead, you should declare a
new cryptographer so that you don't modify sync's internal state (and you won't
need the transaction/writenode dependency either):

ChromeEncryptor encryptor;
browser_sync::Cryptographer temp_cryptographer(&encryptor);
...
temp_cryptographer->AddKey(..)
temp_cryptographer->GetBootstrapToken(..)
sync_prefs.SetSpareBootstrapToken(..)

http://codereview.chromium.org/9956097/diff/2008/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:1469:
RefreshSpareBootstrapToken(successful->password);
Condition this on OS_CHROMEOS

http://codereview.chromium.org/9956097/diff/2008/chrome/browser/sync/sync_pre...
File chrome/browser/sync/sync_prefs.cc (right):

http://codereview.chromium.org/9956097/diff/2008/chrome/browser/sync/sync_pre...
chrome/browser/sync/sync_prefs.cc:190: CHECK(pref_service_);
this isn't really necessary when you're dereferencing pref_service_ right below,
as it's going to crash anyways.

http://codereview.chromium.org/9956097/diff/2008/chrome/browser/sync/sync_pre...
File chrome/browser/sync/sync_prefs.h (right):

http://codereview.chromium.org/9956097/diff/2008/chrome/browser/sync/sync_pre...
chrome/browser/sync/sync_prefs.h:101: std::string GetSpareBootstrapToken()
const;
Comment that this bootstrap token is used only when setting up sync for the
first time (and comment above that the encryptionbootstraptoken is used for
restarts once already syncing)

[kochi, 2012-04-03 21:55:29]

Resolving initial review comments from Andrew.

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/signi...
File chrome/browser/signin/signin_manager.h (right):

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/signi...
chrome/browser/signin/signin_manager.h:132: const std::string&
convenient_password() const { return convenient_password_; }
This line is removed.
I'll run cpplint and fix all warnings.

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/signi...
File chrome/browser/signin/signin_tracker.cc (right):

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/signi...
chrome/browser/signin/signin_tracker.cc:28: state_(WAITING_FOR_GAIA_VALIDATION),
On 2012/04/03 17:45:13, Andrew T Wilson wrote:
> I think it would be better to have an alternate constructor that lets you set
> the initial state. That way, you can run tests for this case on any platform.

Done.

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/signi...
chrome/browser/signin/signin_tracker.cc:39: #if !defined(OS_CHROMEOS)
On 2012/04/03 17:45:13, Andrew T Wilson wrote:
> I'd say you should probably just register for all these notifications even if
> you don't need them on cros. For example, you don't care about the
> TOKEN_AVAILABLE/REQUEST_FAILED notifications either.

Done.

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/signi...
chrome/browser/signin/signin_tracker.cc:62: DCHECK_EQ(state_,
SERVICES_INITIALIZING);
On 2012/04/03 17:45:13, Andrew T Wilson wrote:
> When you change the constructor to allow setting the intitial state, I'd
change
> this to "if (state_ == SERVICES_INITIALIZING) HandleServiceStateChange();" -
no
> need to make it cros only, 

Done.

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/signi...
chrome/browser/signin/signin_tracker.cc:143: LOG(WARNING) <<
"HandleServiceStateChange: service is waiting for auth";
I'm planning to clean all these LOG(WARNING) messages,
but may keep some useful ones as DVLOG.
I'll revisit this.

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/signi...
File chrome/browser/signin/signin_tracker_unittest.cc (right):

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/signi...
chrome/browser/signin/signin_tracker_unittest.cc:229: #if !defined(OS_CHROMEOS)
On 2012/04/03 17:45:13, Andrew T Wilson wrote:
> If you make the signin tracker so you can inject the starting state via the
> constructor, you won't need to disable this test. And you'll be able to create
a
> test for the chromeos flow that will run on every platform.

Done.

Added TODO.

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/sync/...
File chrome/browser/sync/sync_prefs.cc (right):

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/sync/...
chrome/browser/sync/sync_prefs.cc:53:
pref_service_->ClearPref(prefs::kSyncEncryptionBootstrapToken);
Made a separate pref to persist another bootstrap token.

On 2012/04/03 17:45:13, Andrew T Wilson wrote:
> Not sure this is right. We should be maintaining the GAIA bootstrap token in a
> separate pref and using that when re-enabling sync, rather than maintaining
the
> old bootstrap token which might contain a token related to a custom
passphrase).

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/ui/we...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/ui/we...
chrome/browser/ui/webui/sync_setup_handler.cc:603: #endif
On 2012/04/03 17:45:13, Andrew T Wilson wrote:
> We should probably close the overlay here though, to make sure the spinner is
> hidden?

Done.

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/ui/we...
chrome/browser/ui/webui/sync_setup_handler.cc:627: DisplayConfigureSync(true,
false);
On 2012/04/03 17:45:13, Andrew T Wilson wrote:
> Are you sure that's what we want here (not the "Sync Everything" button
> instead)?

I thought this was the intention (before enabling sync,
give the user an opportunity to configure which data types
to be synced), but could be okay as the user can configure
data types anyway.  To make the diversity less, removed this
ifdef.

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/ui/we...
chrome/browser/ui/webui/sync_setup_handler.cc:827: browser_sync::SyncPrefs
sync_prefs(GetProfile()->GetPrefs());
Split this part into a helper function.

For #ifdef'ing the non-ChromeOS part makes this
CL bigger, so i'd like to defer it for later CLs.

On 2012/04/03 17:45:13, Andrew T Wilson wrote:
> Should probably just move this into a helper function.
> 
> Also, if you never want to display a GAIA login on cros, you should probably
#if
> it out of the header and .cc file to make sure calls don't sneak back in.

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/ui/we...
chrome/browser/ui/webui/sync_setup_handler.cc:831: signin_tracker_.reset(new
SigninTracker(GetProfile(), this));
On 2012/04/03 17:45:13, Andrew T Wilson wrote:
> We don't need a signin tracker in the case that sync is already initialized.

But for the case sync backend is stopped, we need to have
SigninTracker.

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/ui/we...
chrome/browser/ui/webui/sync_setup_handler.cc:836:
token_service->StartFetchingTokens();
Removed.

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/ui/we...
chrome/browser/ui/webui/sync_setup_handler.cc:842:
service->set_setup_in_progress(true);
So as not to break non-ChromeOS for now, I'd like to
keep this as is...

On 2012/04/03 17:45:13, Andrew T Wilson wrote:
> This looks good. In fact, we might want to have this in general (not just for
> os_chromeos) in case someone tries to bring up the configure sync dialog
before
> the backend is initialized.

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/ui/we...
File chrome/browser/ui/webui/sync_setup_handler.h (right):

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/ui/we...
chrome/browser/ui/webui/sync_setup_handler.h:127: // is running in the
background.
On 2012/04/03 17:45:13, Andrew T Wilson wrote:
> Note: http://code.google.com/p/chromium/issues/detail?id=121563
> 
> You should probably include that bug number in your CL BUG= description.

Done.

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/ui/we...
File chrome/browser/ui/webui/sync_setup_handler_unittest.cc (right):

https://chromiumcodereview.appspot.com/9956097/diff/3001/chrome/browser/ui/we...
chrome/browser/ui/webui/sync_setup_handler_unittest.cc:424: #if
!defined(OS_CHROMEOS)
Added TODO.

On 2012/04/03 17:45:13, Andrew T Wilson wrote:
> I think we'll still need some flavor of this test, since even on cros you can
> get an unrecoverable error while starting up sync.

[Andrew T Wilson (Slow), 2012-04-03 22:22:33]

Getting pretty close - nice work on this.

https://chromiumcodereview.appspot.com/9956097/diff/1022/chrome/browser/sync/...
File chrome/browser/sync/profile_sync_service.cc (right):

https://chromiumcodereview.appspot.com/9956097/diff/1022/chrome/browser/sync/...
chrome/browser/sync/profile_sync_service.cc:1095: sync_api::WriteTransaction
trans(FROM_HERE, GetUserShare());
I think we want to create our own cryptographer here, not access one from a
transaction. We should not be grabbing any locks here (i.e. creating
Read/WriteTransaction objects in ProfileSyncService) since that can block the UI
thread. Nicolas can confirm the best way to get a local cryptographer.

https://chromiumcodereview.appspot.com/9956097/diff/1022/chrome/browser/sync/...
chrome/browser/sync/profile_sync_service.cc:1099: if
(!node.InitByTagLookup(browser_sync::kNigoriTag)) {
We don't need a real nigori node here - we can create a fake one like SyncBacken
dHost::CheckPassphraseAgainstCachedPendingKeys() does. In fact, do we even need
this nigori node (it's not used below?)

https://chromiumcodereview.appspot.com/9956097/diff/1022/chrome/browser/sync/...
chrome/browser/sync/profile_sync_service.cc:1581: LOG(WARNING) <<
"Unsuppress()";
See previous comment - not sure if we should break this out.

https://chromiumcodereview.appspot.com/9956097/diff/1022/chrome/browser/sync/...
File chrome/browser/sync/profile_sync_service.h (right):

https://chromiumcodereview.appspot.com/9956097/diff/1022/chrome/browser/sync/...
chrome/browser/sync/profile_sync_service.h:487: void Unsuppress();
Is this needed? I think mostly people talk directly to sync prefs if they just
want to clear the suppress-start flag.

https://chromiumcodereview.appspot.com/9956097/diff/1022/chrome/browser/sync/...
File chrome/browser/sync/sync_prefs.cc (right):

https://chromiumcodereview.appspot.com/9956097/diff/1022/chrome/browser/sync/...
chrome/browser/sync/sync_prefs.cc:183: return
uber nit: I'd probably move "pref_service_ ?" up to the same line as the return
statement.

https://chromiumcodereview.appspot.com/9956097/diff/1022/chrome/browser/ui/we...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

https://chromiumcodereview.appspot.com/9956097/diff/1022/chrome/browser/ui/we...
chrome/browser/ui/webui/sync_setup_handler.cc:622: if
(GetSyncService()->HasSyncSetupCompleted()) {
nit: No need to modify this code by adding braces.

https://chromiumcodereview.appspot.com/9956097/diff/1022/chrome/browser/ui/we...
chrome/browser/ui/webui/sync_setup_handler.cc:835:
sync_prefs.SetStartSuppressed(false);
Do we need to call SetStartSuppressed? Doesn't UnsuppressAndStart() below do
this already?

https://chromiumcodereview.appspot.com/9956097/diff/1022/chrome/browser/ui/we...
File chrome/browser/ui/webui/sync_setup_handler.h (right):

https://chromiumcodereview.appspot.com/9956097/diff/1022/chrome/browser/ui/we...
chrome/browser/ui/webui/sync_setup_handler.h:121: void
DisplayGaiaLoginWithErrorMessage(const string16& error_message,
I would #if !(OS_CHROMEOS) this and DisplayGaiaLogin() (maybe in a separate CL)
as otherwise you're likely to have GAIA logins sneak back into cros by people
making changes to the desktop UI flow.

[kochi, 2012-04-03 22:45:43]

https://chromiumcodereview.appspot.com/9956097/diff/2008/chrome/browser/sync/...
File chrome/browser/sync/profile_sync_service.cc (right):

https://chromiumcodereview.appspot.com/9956097/diff/2008/chrome/browser/sync/...
chrome/browser/sync/profile_sync_service.cc:407: CreateBackend();
On 2012/04/03 21:00:33, nzea wrote:
> Right before this is where you should probably fill the normal encryption
> bootstrap token from the spare (but only if it's normal is empty).

Done.

https://chromiumcodereview.appspot.com/9956097/diff/2008/chrome/browser/sync/...
chrome/browser/sync/profile_sync_service.cc:1095: browser_sync::Cryptographer*
cryptographer = trans.GetCryptographer();
Thanks!  This code snippet was very cryptic and I was not
sure.

On 2012/04/03 21:00:33, nzea wrote:
> This is using the sync's internal cryptographer. Instead, you should declare a
> new cryptographer so that you don't modify sync's internal state (and you
won't
> need the transaction/writenode dependency either):
> 
> ChromeEncryptor encryptor;
> browser_sync::Cryptographer temp_cryptographer(&encryptor);
> ...
> temp_cryptographer->AddKey(..)
> temp_cryptographer->GetBootstrapToken(..)
> sync_prefs.SetSpareBootstrapToken(..)

https://chromiumcodereview.appspot.com/9956097/diff/2008/chrome/browser/sync/...
chrome/browser/sync/profile_sync_service.cc:1469:
RefreshSpareBootstrapToken(successful->password);
On 2012/04/03 21:00:33, nzea wrote:
> Condition this on OS_CHROMEOS

Done.

https://chromiumcodereview.appspot.com/9956097/diff/2008/chrome/browser/sync/...
File chrome/browser/sync/sync_prefs.cc (right):

https://chromiumcodereview.appspot.com/9956097/diff/2008/chrome/browser/sync/...
chrome/browser/sync/sync_prefs.cc:190: CHECK(pref_service_);
Done for here and line 177.

On 2012/04/03 21:00:33, nzea wrote:
> this isn't really necessary when you're dereferencing pref_service_ right
below,
> as it's going to crash anyways.

https://chromiumcodereview.appspot.com/9956097/diff/2008/chrome/browser/sync/...
File chrome/browser/sync/sync_prefs.h (right):

https://chromiumcodereview.appspot.com/9956097/diff/2008/chrome/browser/sync/...
chrome/browser/sync/sync_prefs.h:101: std::string GetSpareBootstrapToken()
const;
On 2012/04/03 21:00:33, nzea wrote:
> Comment that this bootstrap token is used only when setting up sync for the
> first time (and comment above that the encryptionbootstraptoken is used for
> restarts once already syncing)

Done.

[Nicolas Zea, 2012-04-03 22:54:21]

almost there!

https://chromiumcodereview.appspot.com/9956097/diff/5004/chrome/browser/sync/...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

https://chromiumcodereview.appspot.com/9956097/diff/5004/chrome/browser/sync/...
chrome/browser/sync/glue/sync_backend_host.cc:357: 
can revert this file

https://chromiumcodereview.appspot.com/9956097/diff/5004/chrome/browser/sync/...
File chrome/browser/sync/profile_sync_service.cc (right):

https://chromiumcodereview.appspot.com/9956097/diff/5004/chrome/browser/sync/...
chrome/browser/sync/profile_sync_service.cc:205: LOG(WARNING) << "TryStart";
remove various warnings

https://chromiumcodereview.appspot.com/9956097/diff/5004/chrome/browser/sync/...
chrome/browser/sync/profile_sync_service.cc:407: if (bootstrap_token.empty())
curly brace around multiline if

https://chromiumcodereview.appspot.com/9956097/diff/5004/chrome/browser/sync/...
File chrome/browser/sync/profile_sync_service.h (right):

https://chromiumcodereview.appspot.com/9956097/diff/5004/chrome/browser/sync/...
chrome/browser/sync/profile_sync_service.h:625: void
RefreshSpareBootstrapToken(const std::string& passphrase);
change comment to "Called on successul sign-in notifications."

[kochi, 2012-04-03 23:21:48]

https://chromiumcodereview.appspot.com/9956097/diff/1022/chrome/browser/sync/...
File chrome/browser/sync/profile_sync_service.cc (right):

https://chromiumcodereview.appspot.com/9956097/diff/1022/chrome/browser/sync/...
chrome/browser/sync/profile_sync_service.cc:1095: sync_api::WriteTransaction
trans(FROM_HERE, GetUserShare());
Done as Nicolas suggested.

On 2012/04/03 22:22:33, Andrew T Wilson wrote:
> I think we want to create our own cryptographer here, not access one from a
> transaction. We should not be grabbing any locks here (i.e. creating
> Read/WriteTransaction objects in ProfileSyncService) since that can block the
UI
> thread. Nicolas can confirm the best way to get a local cryptographer.

https://chromiumcodereview.appspot.com/9956097/diff/1022/chrome/browser/sync/...
chrome/browser/sync/profile_sync_service.cc:1099: if
(!node.InitByTagLookup(browser_sync::kNigoriTag)) {
Done as Nicolas suggested.
On 2012/04/03 22:22:33, Andrew T Wilson wrote:
> We don't need a real nigori node here - we can create a fake one like
SyncBacken
> dHost::CheckPassphraseAgainstCachedPendingKeys() does. In fact, do we even
need
> this nigori node (it's not used below?)

https://chromiumcodereview.appspot.com/9956097/diff/1022/chrome/browser/sync/...
chrome/browser/sync/profile_sync_service.cc:1581: LOG(WARNING) <<
"Unsuppress()";
On 2012/04/03 22:22:33, Andrew T Wilson wrote:
> See previous comment - not sure if we should break this out.

Done.

https://chromiumcodereview.appspot.com/9956097/diff/1022/chrome/browser/sync/...
File chrome/browser/sync/profile_sync_service.h (right):

https://chromiumcodereview.appspot.com/9956097/diff/1022/chrome/browser/sync/...
chrome/browser/sync/profile_sync_service.h:487: void Unsuppress();
Reverted.

On 2012/04/03 22:22:33, Andrew T Wilson wrote:
> Is this needed? I think mostly people talk directly to sync prefs if they just
> want to clear the suppress-start flag.

https://chromiumcodereview.appspot.com/9956097/diff/1022/chrome/browser/sync/...
File chrome/browser/sync/sync_prefs.cc (right):

https://chromiumcodereview.appspot.com/9956097/diff/1022/chrome/browser/sync/...
chrome/browser/sync/sync_prefs.cc:183: return
On 2012/04/03 22:22:33, Andrew T Wilson wrote:
> uber nit: I'd probably move "pref_service_ ?" up to the same line as the
return
> statement.

Done.

https://chromiumcodereview.appspot.com/9956097/diff/1022/chrome/browser/ui/we...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

https://chromiumcodereview.appspot.com/9956097/diff/1022/chrome/browser/ui/we...
chrome/browser/ui/webui/sync_setup_handler.cc:622: if
(GetSyncService()->HasSyncSetupCompleted()) {
On 2012/04/03 22:22:33, Andrew T Wilson wrote:
> nit: No need to modify this code by adding braces.

Done.

https://chromiumcodereview.appspot.com/9956097/diff/1022/chrome/browser/ui/we...
chrome/browser/ui/webui/sync_setup_handler.cc:835:
sync_prefs.SetStartSuppressed(false);
I think so.  Removed.
This was done when I did something with  TokenService right after this.

On 2012/04/03 22:22:33, Andrew T Wilson wrote:
> Do we need to call SetStartSuppressed? Doesn't UnsuppressAndStart() below do
> this already?

https://chromiumcodereview.appspot.com/9956097/diff/1022/chrome/browser/ui/we...
File chrome/browser/ui/webui/sync_setup_handler.h (right):

https://chromiumcodereview.appspot.com/9956097/diff/1022/chrome/browser/ui/we...
chrome/browser/ui/webui/sync_setup_handler.h:121: void
DisplayGaiaLoginWithErrorMessage(const string16& error_message,
Yes, I agree.

That makes this CL bigger and somewhat complicated than
expected.  E.g. DisplayGaiaLogin() is called from several
places in this file.  #ifdef'ing out will make the code
messier.  Maybe in another refactoring CL would be good.

On 2012/04/03 22:22:33, Andrew T Wilson wrote:
> I would #if !(OS_CHROMEOS) this and DisplayGaiaLogin() (maybe in a separate
CL)
> as otherwise you're likely to have GAIA logins sneak back into cros by people
> making changes to the desktop UI flow.

[kochi, 2012-04-03 23:35:46]

https://chromiumcodereview.appspot.com/9956097/diff/5004/chrome/browser/sync/...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

https://chromiumcodereview.appspot.com/9956097/diff/5004/chrome/browser/sync/...
chrome/browser/sync/glue/sync_backend_host.cc:357: 
On 2012/04/03 22:54:21, nzea wrote:
> can revert this file

Done.

https://chromiumcodereview.appspot.com/9956097/diff/5004/chrome/browser/sync/...
File chrome/browser/sync/profile_sync_service.cc (right):

https://chromiumcodereview.appspot.com/9956097/diff/5004/chrome/browser/sync/...
chrome/browser/sync/profile_sync_service.cc:205: LOG(WARNING) << "TryStart";
On 2012/04/03 22:54:21, nzea wrote:
> remove various warnings

Done.

https://chromiumcodereview.appspot.com/9956097/diff/5004/chrome/browser/sync/...
chrome/browser/sync/profile_sync_service.cc:407: if (bootstrap_token.empty())
On 2012/04/03 22:54:21, nzea wrote:
> curly brace around multiline if

Done.

https://chromiumcodereview.appspot.com/9956097/diff/5004/chrome/browser/sync/...
File chrome/browser/sync/profile_sync_service.h (right):

https://chromiumcodereview.appspot.com/9956097/diff/5004/chrome/browser/sync/...
chrome/browser/sync/profile_sync_service.h:625: void
RefreshSpareBootstrapToken(const std::string& passphrase);
On 2012/04/03 22:54:21, nzea wrote:
> change comment to "Called on successul sign-in notifications."

Done.

[Nicolas Zea, 2012-04-03 23:47:20]

PSS/sync_prefs/pref_names LGTM with nits/once everything is conditioned
appropriately on CHROMEOS and trybots like it

https://chromiumcodereview.appspot.com/9956097/diff/4036/chrome/common/pref_n...
File chrome/common/pref_names.cc (right):

https://chromiumcodereview.appspot.com/9956097/diff/4036/chrome/common/pref_n...
chrome/common/pref_names.cc:1650: // A string that is used to store first-time
sync startup after once sync is
nit: change to "A bootstrap token used only for first time sync on ChromeOS.
Maintains encryption keys based on the account password".

https://chromiumcodereview.appspot.com/9956097/diff/1045/chrome/browser/sync/...
File chrome/browser/sync/profile_sync_service.cc (right):

https://chromiumcodereview.appspot.com/9956097/diff/1045/chrome/browser/sync/...
chrome/browser/sync/profile_sync_service.cc:401: std::string bootstrap_token =
sync_prefs_.GetEncryptionBootstrapToken();
OS_CHROMEOS

https://chromiumcodereview.appspot.com/9956097/diff/1045/chrome/browser/sync/...
File chrome/browser/sync/profile_sync_service.h (right):

https://chromiumcodereview.appspot.com/9956097/diff/1045/chrome/browser/sync/...
chrome/browser/sync/profile_sync_service.h:622: void
RefreshSpareBootstrapToken(const std::string& passphrase);
condition on OS_CHROMEOS

https://chromiumcodereview.appspot.com/9956097/diff/1045/chrome/browser/sync/...
File chrome/browser/sync/sync_prefs.cc (right):

https://chromiumcodereview.appspot.com/9956097/diff/1045/chrome/browser/sync/...
chrome/browser/sync/sync_prefs.cc:411:
pref_service_->RegisterStringPref(prefs::kSyncSpareBootstrapToken,
condition on OS_CHROMEOS

https://chromiumcodereview.appspot.com/9956097/diff/1045/chrome/browser/sync/...
File chrome/browser/sync/sync_prefs.h (right):

https://chromiumcodereview.appspot.com/9956097/diff/1045/chrome/browser/sync/...
chrome/browser/sync/sync_prefs.h:104: std::string GetSpareBootstrapToken()
const;
Since the preference exists only on ChromeOS, these methods should probably be
conditioned on OS_CHROMEOS as well

[Andrew T Wilson (Slow), 2012-04-03 23:56:53]

LGTM once trybots are happy (and you've verified behavior on cros)

https://chromiumcodereview.appspot.com/9956097/diff/4038/chrome/browser/sync/...
File chrome/browser/sync/profile_sync_service.cc (right):

https://chromiumcodereview.appspot.com/9956097/diff/4038/chrome/browser/sync/...
chrome/browser/sync/profile_sync_service.cc:36: #include
"chrome/browser/sync/glue/chrome_encryptor.h"
move this up one line (sort order)

[kochi, 2012-04-04 00:10:20]

+jhawkins in reviewers for OWNERS lgtm

https://chromiumcodereview.appspot.com/9956097/diff/1045/chrome/browser/sync/...
File chrome/browser/sync/profile_sync_service.cc (right):

https://chromiumcodereview.appspot.com/9956097/diff/1045/chrome/browser/sync/...
chrome/browser/sync/profile_sync_service.cc:401: std::string bootstrap_token =
sync_prefs_.GetEncryptionBootstrapToken();
On 2012/04/03 23:47:20, nzea wrote:
> OS_CHROMEOS

Done.

https://chromiumcodereview.appspot.com/9956097/diff/1045/chrome/browser/sync/...
File chrome/browser/sync/profile_sync_service.h (right):

https://chromiumcodereview.appspot.com/9956097/diff/1045/chrome/browser/sync/...
chrome/browser/sync/profile_sync_service.h:622: void
RefreshSpareBootstrapToken(const std::string& passphrase);
On 2012/04/03 23:47:20, nzea wrote:
> condition on OS_CHROMEOS

Done.

https://chromiumcodereview.appspot.com/9956097/diff/1045/chrome/browser/sync/...
File chrome/browser/sync/sync_prefs.cc (right):

https://chromiumcodereview.appspot.com/9956097/diff/1045/chrome/browser/sync/...
chrome/browser/sync/sync_prefs.cc:411:
pref_service_->RegisterStringPref(prefs::kSyncSpareBootstrapToken,
On 2012/04/03 23:47:20, nzea wrote:
> condition on OS_CHROMEOS

Done.

https://chromiumcodereview.appspot.com/9956097/diff/1045/chrome/browser/sync/...
File chrome/browser/sync/sync_prefs.h (right):

https://chromiumcodereview.appspot.com/9956097/diff/1045/chrome/browser/sync/...
chrome/browser/sync/sync_prefs.h:104: std::string GetSpareBootstrapToken()
const;
On 2012/04/03 23:47:20, nzea wrote:
> Since the preference exists only on ChromeOS, these methods should probably be
> conditioned on OS_CHROMEOS as well

Done.

https://chromiumcodereview.appspot.com/9956097/diff/4038/chrome/browser/sync/...
File chrome/browser/sync/profile_sync_service.cc (right):

https://chromiumcodereview.appspot.com/9956097/diff/4038/chrome/browser/sync/...
chrome/browser/sync/profile_sync_service.cc:36: #include
"chrome/browser/sync/glue/chrome_encryptor.h"
On 2012/04/03 23:56:53, Andrew T Wilson wrote:
> move this up one line (sort order)

I think they are already sorted.

[Andrew T Wilson (Slow), 2012-04-04 00:42:48]

https://chromiumcodereview.appspot.com/9956097/diff/4038/chrome/browser/sync/...
File chrome/browser/sync/profile_sync_service.cc (right):

https://chromiumcodereview.appspot.com/9956097/diff/4038/chrome/browser/sync/...
chrome/browser/sync/profile_sync_service.cc:36: #include
"chrome/browser/sync/glue/chrome_encryptor.h"
You're right. Apparently I can't read.

[kochi, 2012-04-04 05:27:58]

The win_rel/linux_chromeos trybots failures are very unlikely
related to this CL.  win_rel's browser_tests failed many FileIO
tests.  3 browser_tests of linux_chromeos timed out.

On 2012/04/04 00:42:48, Andrew T Wilson wrote:
>
https://chromiumcodereview.appspot.com/9956097/diff/4038/chrome/browser/sync/...
> File chrome/browser/sync/profile_sync_service.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9956097/diff/4038/chrome/browser/sync/...
> chrome/browser/sync/profile_sync_service.cc:36: #include
> "chrome/browser/sync/glue/chrome_encryptor.h"
> You're right. Apparently I can't read.

[Andrew T Wilson (Slow), 2012-04-04 16:52:10]

https://chromiumcodereview.appspot.com/9956097/diff/4061/chrome/browser/ui/we...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

https://chromiumcodereview.appspot.com/9956097/diff/4061/chrome/browser/ui/we...
chrome/browser/ui/webui/sync_setup_handler.cc:837:
service->UnsuppressAndStart();
So, if the user cancels out of the spinner, they'll be left with sync
unsuppressed which means sync will automatically start with the default encrypt
settings the next time the user restarts.

I wonder if we need to add handling for cancelling the spinner to re-suppress
sync if sync hasn't started up?

[arv (Not doing code reviews), 2012-04-04 17:04:20]

LGTM

My main concern is security here. I hope someone who knows the security
implication of this also has approved this.

https://chromiumcodereview.appspot.com/9956097/diff/4061/chrome/browser/resou...
File chrome/browser/resources/sync_setup_overlay.js (right):

https://chromiumcodereview.appspot.com/9956097/diff/4061/chrome/browser/resou...
chrome/browser/resources/sync_setup_overlay.js:400: showSpinner_: function(args)
{
args are not used

https://chromiumcodereview.appspot.com/9956097/diff/4061/chrome/browser/signi...
File chrome/browser/signin/signin_tracker.cc (right):

https://chromiumcodereview.appspot.com/9956097/diff/4061/chrome/browser/signi...
chrome/browser/signin/signin_tracker.cc:44: ProfileSyncService* service =
No need for var here

https://chromiumcodereview.appspot.com/9956097/diff/4061/chrome/browser/signi...
File chrome/browser/signin/signin_tracker_unittest.cc (right):

https://chromiumcodereview.appspot.com/9956097/diff/4061/chrome/browser/signi...
chrome/browser/signin/signin_tracker_unittest.cc:277: // TODO(kochi) Add tests
with initial state != WATIING_FOR_GAIA_VALIDATION.
Can you do this in this patch?

https://chromiumcodereview.appspot.com/9956097/diff/4061/chrome/browser/sync/...
File chrome/browser/sync/profile_sync_service.cc (right):

https://chromiumcodereview.appspot.com/9956097/diff/4061/chrome/browser/sync/...
chrome/browser/sync/profile_sync_service.cc:1087: browser_sync::KeyParams
key_params = {"localhost", "dummy", passphrase};
I have no idea if this is correct. I would add a comment explaining why
localhost and dummy are used.

https://chromiumcodereview.appspot.com/9956097/diff/4061/chrome/browser/ui/we...
File chrome/browser/ui/webui/sync_setup_handler.h (right):

https://chromiumcodereview.appspot.com/9956097/diff/4061/chrome/browser/ui/we...
chrome/browser/ui/webui/sync_setup_handler.h:130: // TODO(kochi): better to show
some message that the user can understand what
Is there a bug for this?

[kochi, 2012-04-04 17:29:46]

Thanks for the review!

http://chromiumcodereview.appspot.com/9956097/diff/4061/chrome/browser/resour...
File chrome/browser/resources/sync_setup_overlay.js (right):

http://chromiumcodereview.appspot.com/9956097/diff/4061/chrome/browser/resour...
chrome/browser/resources/sync_setup_overlay.js:400: showSpinner_: function(args)
{
On 2012/04/04 17:04:21, arv wrote:
> args are not used

Removed (and also from the caller).

http://chromiumcodereview.appspot.com/9956097/diff/4061/chrome/browser/signin...
File chrome/browser/signin/signin_tracker.cc (right):

http://chromiumcodereview.appspot.com/9956097/diff/4061/chrome/browser/signin...
chrome/browser/signin/signin_tracker.cc:44: ProfileSyncService* service =
On 2012/04/04 17:04:21, arv wrote:
> No need for var here

Done.

http://chromiumcodereview.appspot.com/9956097/diff/4061/chrome/browser/signin...
File chrome/browser/signin/signin_tracker_unittest.cc (right):

http://chromiumcodereview.appspot.com/9956097/diff/4061/chrome/browser/signin...
chrome/browser/signin/signin_tracker_unittest.cc:277: // TODO(kochi) Add tests
with initial state != WATIING_FOR_GAIA_VALIDATION.
On 2012/04/04 17:04:21, arv wrote:
> Can you do this in this patch?

Will file a issue on the bug tracker.

http://chromiumcodereview.appspot.com/9956097/diff/4061/chrome/browser/sync/p...
File chrome/browser/sync/profile_sync_service.cc (right):

http://chromiumcodereview.appspot.com/9956097/diff/4061/chrome/browser/sync/p...
chrome/browser/sync/profile_sync_service.cc:1087: browser_sync::KeyParams
key_params = {"localhost", "dummy", passphrase};
On 2012/04/04 17:04:21, arv wrote:
> I have no idea if this is correct. I would add a comment explaining why
> localhost and dummy are used.

Done.

http://chromiumcodereview.appspot.com/9956097/diff/4061/chrome/browser/ui/web...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

http://chromiumcodereview.appspot.com/9956097/diff/4061/chrome/browser/ui/web...
chrome/browser/ui/webui/sync_setup_handler.cc:837:
service->UnsuppressAndStart();
Let's suppress in PSS::DisableForUser() as discussed.

On 2012/04/04 16:52:11, Andrew T Wilson wrote:
> So, if the user cancels out of the spinner, they'll be left with sync
> unsuppressed which means sync will automatically start with the default
encrypt
> settings the next time the user restarts.
> 
> I wonder if we need to add handling for cancelling the spinner to re-suppress
> sync if sync hasn't started up?

http://chromiumcodereview.appspot.com/9956097/diff/4061/chrome/browser/ui/web...
File chrome/browser/ui/webui/sync_setup_handler.h (right):

http://chromiumcodereview.appspot.com/9956097/diff/4061/chrome/browser/ui/web...
chrome/browser/ui/webui/sync_setup_handler.h:130: // TODO(kochi): better to show
some message that the user can understand what
On 2012/04/04 17:04:21, arv wrote:
> Is there a bug for this?

Will add a comment in crbug.com/121563.
Thanks for the suggestion!

[commit-bot: I haz the power, 2012-04-04 17:31:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kochi@chromium.org/9956097/13001

[commit-bot: I haz the power, 2012-04-04 17:31:49]

Can't apply patch for file chrome/browser/ui/webui/sync_setup_handler.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/ui/webui/sync_setup_handler.cc
Hunk #1 succeeded at 492 (offset 2 lines).
Hunk #2 succeeded at 518 (offset 2 lines).
Hunk #3 succeeded at 599 (offset 2 lines).
Hunk #4 succeeded at 775 (offset 2 lines).
Hunk #5 FAILED at 813.
1 out of 5 hunks FAILED -- saving rejects to file
chrome/browser/ui/webui/sync_setup_handler.cc.rej

[Andrew T Wilson (Slow), 2012-04-04 17:55:30]

http://codereview.chromium.org/9956097/diff/10017/chrome/browser/ui/webui/syn...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

http://codereview.chromium.org/9956097/diff/10017/chrome/browser/ui/webui/syn...
chrome/browser/ui/webui/sync_setup_handler.cc:837: DisplayConfigureSync(true,
false);
I think this is wrong now. You should be #if-ing out the call to
DisplayGaiaLogin(). We really should remove that function on cros.

[kochi, 2012-04-04 18:07:09]

http://chromiumcodereview.appspot.com/9956097/diff/10017/chrome/browser/ui/we...
File chrome/browser/ui/webui/sync_setup_handler.cc (right):

http://chromiumcodereview.appspot.com/9956097/diff/10017/chrome/browser/ui/we...
chrome/browser/ui/webui/sync_setup_handler.cc:837: DisplayConfigureSync(true,
false);
On 2012/04/04 17:55:30, Andrew T Wilson wrote:
> I think this is wrong now. You should be #if-ing out the call to
> DisplayGaiaLogin(). We really should remove that function on cros.

Done.

[Andrew T Wilson (Slow), 2012-04-04 18:11:59]

LGTM

[James Hawkins, 2012-04-04 19:56:52]

LGTM but all of these cros ifdefs have got to go at some point.