Support Sync following Gaia OAuth authentication

Support Sync operations after user signing with --enable-sync-oauth

BUG=92329
TEST=manual

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

[Rick Campbell, 2011-08-10 16:33:44]

Please take a look

[Rick Campbell, 2011-08-10 17:46:04]

Added Roger and Sanjeev as reviewers mostly for informational purposes.  Roger,
I put you in as the potential owner for a TODO bug that I filed for updating
Autologin for OAuth use cases.

[Roger Tawa OOO till Jul 10th, 2011-08-10 18:10:43]

Hi Rick, I can take a look at auto-login when this is done.

http://codereview.chromium.org/7497069/diff/1/chrome/browser/sync/signin_mana...
File chrome/browser/sync/signin_manager.cc (right):

http://codereview.chromium.org/7497069/diff/1/chrome/browser/sync/signin_mana...
chrome/browser/sync/signin_manager.cc:171: prefs::kGoogleServicesUsername,
GetUsername());
won't GetUsername() always return an empty string?

http://codereview.chromium.org/7497069/diff/1/chrome/browser/sync/signin_mana...
chrome/browser/sync/signin_manager.cc:280: VLOG(1) <<
"SigninManager::OnUserInfoSuccess(\"" << email << "\")";
should this VLOG also be changed?

[Rick Campbell, 2011-08-10 18:55:16]

Please take another look.

http://codereview.chromium.org/7497069/diff/1/chrome/browser/sync/signin_mana...
File chrome/browser/sync/signin_manager.cc (right):

http://codereview.chromium.org/7497069/diff/1/chrome/browser/sync/signin_mana...
chrome/browser/sync/signin_manager.cc:171: prefs::kGoogleServicesUsername,
GetUsername());
On 2011/08/10 18:10:43, Roger Tawa wrote:
> won't GetUsername() always return an empty string?

Done.  Changing this and the next one to ClearPref, noting that (a) this is a
better reflection of the intent of the code and (b) GetString returns "", and
GetBoolean returns false, for non-existent prefs.

http://codereview.chromium.org/7497069/diff/1/chrome/browser/sync/signin_mana...
chrome/browser/sync/signin_manager.cc:280: VLOG(1) <<
"SigninManager::OnUserInfoSuccess(\"" << email << "\")";
On 2011/08/10 18:10:43, Roger Tawa wrote:
> should this VLOG also be changed?

Done.

[Roger Tawa OOO till Jul 10th, 2011-08-10 19:13:04]

lgtm

http://codereview.chromium.org/7497069/diff/8001/chrome/browser/sync/signin_m...
File chrome/browser/sync/signin_manager.cc (right):

http://codereview.chromium.org/7497069/diff/8001/chrome/browser/sync/signin_m...
chrome/browser/sync/signin_manager.cc:279: VLOG(1) << "Sync signin for " <<
email << " is complete.";
Did you forget to change this vlog?

Is there a reason to change this?  I think Tim had made a comment in a previous
CL that LOG should be replaced with VLOG.

[tim (not reviewing), 2011-08-10 19:39:35]

http://codereview.chromium.org/7497069/diff/5001/chrome/browser/sync/engine/n...
File chrome/browser/sync/engine/net/syncapi_server_connection_manager.cc
(right):

http://codereview.chromium.org/7497069/diff/5001/chrome/browser/sync/engine/n...
chrome/browser/sync/engine/net/syncapi_server_connection_manager.cc:11: #include
"chrome/browser/sync/sync_setup_wizard.h"
Although we don't have a DEPS rule enforcing this, we shouldn't have a
dependency on browser/sync or UI code from engine.

http://codereview.chromium.org/7497069/diff/5001/chrome/browser/sync/signin_m...
File chrome/browser/sync/signin_manager.h (right):

http://codereview.chromium.org/7497069/diff/5001/chrome/browser/sync/signin_m...
chrome/browser/sync/signin_manager.h:123: // ClientLogin identity
style nit - add periods to comments.

[akalin, 2011-08-10 19:42:01]

http://codereview.chromium.org/7497069/diff/8001/chrome/browser/sync/signin_m...
File chrome/browser/sync/signin_manager.cc (right):

http://codereview.chromium.org/7497069/diff/8001/chrome/browser/sync/signin_m...
chrome/browser/sync/signin_manager.cc:35: if (SyncSetupWizard::IsUsingOAuth())
use SetUsername here?

http://codereview.chromium.org/7497069/diff/8001/chrome/browser/sync/signin_m...
chrome/browser/sync/signin_manager.cc:42: if (!username_.empty() ||
!oauth_username_.empty()) {
use GetUsername here?

http://codereview.chromium.org/7497069/diff/8001/chrome/browser/sync/sync_set...
File chrome/browser/sync/sync_setup_wizard.cc (right):

http://codereview.chromium.org/7497069/diff/8001/chrome/browser/sync/sync_set...
chrome/browser/sync/sync_setup_wizard.cc:98: return
CommandLine::ForCurrentProcess()->HasSwitch(
Consider adding it to about:flags, too.  It's easy, see:
http://codereview.chromium.org/7607036/ for an example CL

[Andrew T Wilson (Slow), 2011-08-10 20:54:50]

LGTM - just had one nit.

http://codereview.chromium.org/7497069/diff/8001/chrome/browser/sync/signin_m...
File chrome/browser/sync/signin_manager.cc (right):

http://codereview.chromium.org/7497069/diff/8001/chrome/browser/sync/signin_m...
chrome/browser/sync/signin_manager.cc:37:
profile_->GetPrefs()->GetString(prefs::kGoogleServicesUsername);
If you don't change this to use SetUsername(), should use braces since this
wrapped to two lines.

[Rick Campbell, 2011-08-11 03:58:23]

Please take another look.

http://codereview.chromium.org/7497069/diff/5001/chrome/browser/sync/engine/n...
File chrome/browser/sync/engine/net/syncapi_server_connection_manager.cc
(right):

http://codereview.chromium.org/7497069/diff/5001/chrome/browser/sync/engine/n...
chrome/browser/sync/engine/net/syncapi_server_connection_manager.cc:11: #include
"chrome/browser/sync/sync_setup_wizard.h"
On 2011/08/10 19:39:36, timsteele wrote:
> Although we don't have a DEPS rule enforcing this, we shouldn't have a
> dependency on browser/sync or UI code from engine.  

Done.  Eliminated SyncSetupWizard::IsUsingOAuth()

http://codereview.chromium.org/7497069/diff/5001/chrome/browser/sync/signin_m...
File chrome/browser/sync/signin_manager.h (right):

http://codereview.chromium.org/7497069/diff/5001/chrome/browser/sync/signin_m...
chrome/browser/sync/signin_manager.h:123: // ClientLogin identity
On 2011/08/10 19:39:36, timsteele wrote:
> style nit - add periods to comments.

Done.

http://codereview.chromium.org/7497069/diff/8001/chrome/browser/sync/signin_m...
File chrome/browser/sync/signin_manager.cc (right):

http://codereview.chromium.org/7497069/diff/8001/chrome/browser/sync/signin_m...
chrome/browser/sync/signin_manager.cc:35: if (SyncSetupWizard::IsUsingOAuth())
On 2011/08/10 19:42:01, akalin wrote:
> use SetUsername here?

Done.

http://codereview.chromium.org/7497069/diff/8001/chrome/browser/sync/signin_m...
chrome/browser/sync/signin_manager.cc:37:
profile_->GetPrefs()->GetString(prefs::kGoogleServicesUsername);
On 2011/08/10 20:54:50, Andrew T Wilson wrote:
> If you don't change this to use SetUsername(), should use braces since this
> wrapped to two lines.

Done, by using SetUsername.

http://codereview.chromium.org/7497069/diff/8001/chrome/browser/sync/signin_m...
chrome/browser/sync/signin_manager.cc:42: if (!username_.empty() ||
!oauth_username_.empty()) {
On 2011/08/10 19:42:01, akalin wrote:
> use GetUsername here?

Done.

http://codereview.chromium.org/7497069/diff/8001/chrome/browser/sync/signin_m...
chrome/browser/sync/signin_manager.cc:279: VLOG(1) << "Sync signin for " <<
email << " is complete.";
On 2011/08/10 19:13:05, Roger Tawa wrote:
> Did you forget to change this vlog?
> 
> Is there a reason to change this?  I think Tim had made a comment in a
previous
> CL that LOG should be replaced with VLOG.

I think that I misunderstood originally.  I changed the text of the VLOG to be
more context-specific.

My intent/understanding here is to prefer VLOG over LOG except for LOG(WARNING)
and LOG(ERROR).  Is that right?

And then the specific changes that I made to LOG(WARNING) were to highlight bad
behavior.  I guess I have an implicit TODO to go through all of the
less-than-happy use cases and augment the LOG(WARNING)s with appropriate state
and UI changes.

http://codereview.chromium.org/7497069/diff/8001/chrome/browser/sync/sync_set...
File chrome/browser/sync/sync_setup_wizard.cc (right):

http://codereview.chromium.org/7497069/diff/8001/chrome/browser/sync/sync_set...
chrome/browser/sync/sync_setup_wizard.cc:98: return
CommandLine::ForCurrentProcess()->HasSwitch(
On 2011/08/10 19:42:01, akalin wrote:
> Consider adding it to about:flags, too.  It's easy, see:
> http://codereview.chromium.org/7607036/ for an example CL

Done, and thanks for that pointer!

[akalin, 2011-08-11 18:03:37]

Few more

http://codereview.chromium.org/7497069/diff/13002/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/7497069/diff/13002/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:76: namespace {
since this is used in 3 places, can you maybe move it to a helper function in
util/?

http://codereview.chromium.org/7497069/diff/13002/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:370: IsUsingOAuth() ?
this code is also repeated

http://codereview.chromium.org/7497069/diff/13002/chrome/browser/sync/signin_...
File chrome/browser/sync/signin_manager.cc (right):

http://codereview.chromium.org/7497069/diff/13002/chrome/browser/sync/signin_...
chrome/browser/sync/signin_manager.cc:26: }
}  // namespace

http://codereview.chromium.org/7497069/diff/13002/chrome/browser/sync/signin_...
chrome/browser/sync/signin_manager.cc:72: if (IsUsingOAuth())
Consider:

return IsUsingOAuth() ? oauth_username_ ; username_;

http://codereview.chromium.org/7497069/diff/13002/chrome/browser/sync/signin_...
chrome/browser/sync/signin_manager.cc:78: if (IsUsingOAuth())
Consider:

(IsUsingOAuth() ? oauth_username_ : username_) = username;

http://codereview.chromium.org/7497069/diff/13002/chrome/browser/sync/signin_...
chrome/browser/sync/signin_manager.cc:285: VLOG(1) << "Sync signin for " <<
email << " is complete.";
DCHECK(IsUsingOAuth()) ?

[Rick Campbell, 2011-08-11 21:04:08]

Please take another look.

http://codereview.chromium.org/7497069/diff/13002/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/7497069/diff/13002/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:76: namespace {
On 2011/08/11 18:03:37, akalin wrote:
> since this is used in 3 places, can you maybe move it to a helper function in
> util/?

Done.

http://codereview.chromium.org/7497069/diff/13002/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:370: IsUsingOAuth() ?
On 2011/08/11 18:03:37, akalin wrote:
> this code is also repeated

Done.

http://codereview.chromium.org/7497069/diff/13002/chrome/browser/sync/signin_...
File chrome/browser/sync/signin_manager.cc (right):

http://codereview.chromium.org/7497069/diff/13002/chrome/browser/sync/signin_...
chrome/browser/sync/signin_manager.cc:26: }
On 2011/08/11 18:03:37, akalin wrote:
> }  // namespace

Moved IsUsingOAuth() to util/

http://codereview.chromium.org/7497069/diff/13002/chrome/browser/sync/signin_...
chrome/browser/sync/signin_manager.cc:72: if (IsUsingOAuth())
On 2011/08/11 18:03:37, akalin wrote:
> Consider:
> 
> return IsUsingOAuth() ? oauth_username_ ; username_;

Done.

http://codereview.chromium.org/7497069/diff/13002/chrome/browser/sync/signin_...
chrome/browser/sync/signin_manager.cc:78: if (IsUsingOAuth())
On 2011/08/11 18:03:37, akalin wrote:
> Consider:
> 
> (IsUsingOAuth() ? oauth_username_ : username_) = username;

As we discussed out-of-band, I'm going to leave this one alone.

http://codereview.chromium.org/7497069/diff/13002/chrome/browser/sync/signin_...
chrome/browser/sync/signin_manager.cc:285: VLOG(1) << "Sync signin for " <<
email << " is complete.";
On 2011/08/11 18:03:37, akalin wrote:
> DCHECK(IsUsingOAuth()) ?

Done.  Added similar DCHECKs for SigninManager::On*

[tim (not reviewing), 2011-08-11 21:45:32]

LGTM

[tim (not reviewing), 2011-08-11 21:45:55]

On 2011/08/11 21:45:32, timsteele wrote:
> LGTM

contingent on Fred's LGTM :)

[akalin, 2011-08-11 21:49:14]

LGTM after following comments

http://codereview.chromium.org/7497069/diff/27019/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/net/syncapi_server_connection_manager.cc
(right):

http://codereview.chromium.org/7497069/diff/27019/chrome/browser/sync/engine/...
chrome/browser/sync/engine/net/syncapi_server_connection_manager.cc:7: #include
"base/command_line.h"
remove this include (and include oauth.h)

http://codereview.chromium.org/7497069/diff/27019/chrome/browser/sync/engine/...
chrome/browser/sync/engine/net/syncapi_server_connection_manager.cc:11: #include
"chrome/common/chrome_switches.h"
remove this include (and include oauth.h)

http://codereview.chromium.org/7497069/diff/27019/chrome/browser/sync/engine/...
chrome/browser/sync/engine/net/syncapi_server_connection_manager.cc:42: if
(CommandLine::ForCurrentProcess()->HasSwitch(
use IsUsingOAuth here.  Or maybe decomp this into another function in oauth.h
(e.g., GetAuthHeaders() or something)

http://codereview.chromium.org/7497069/diff/27019/chrome/browser/sync/util/oa...
File chrome/browser/sync/util/oauth.h (right):

http://codereview.chromium.org/7497069/diff/27019/chrome/browser/sync/util/oa...
chrome/browser/sync/util/oauth.h:9: namespace browser_sync {
space after namespace line

[Rick Campbell, 2011-08-12 09:02:02]

http://codereview.chromium.org/7497069/diff/27019/chrome/browser/sync/engine/...
File chrome/browser/sync/engine/net/syncapi_server_connection_manager.cc
(right):

http://codereview.chromium.org/7497069/diff/27019/chrome/browser/sync/engine/...
chrome/browser/sync/engine/net/syncapi_server_connection_manager.cc:7: #include
"base/command_line.h"
On 2011/08/11 21:49:14, akalin wrote:
> remove this include (and include oauth.h)

Done.

http://codereview.chromium.org/7497069/diff/27019/chrome/browser/sync/engine/...
chrome/browser/sync/engine/net/syncapi_server_connection_manager.cc:11: #include
"chrome/common/chrome_switches.h"
On 2011/08/11 21:49:14, akalin wrote:
> remove this include (and include oauth.h)

Done.

http://codereview.chromium.org/7497069/diff/27019/chrome/browser/sync/engine/...
chrome/browser/sync/engine/net/syncapi_server_connection_manager.cc:42: if
(CommandLine::ForCurrentProcess()->HasSwitch(
On 2011/08/11 21:49:14, akalin wrote:
> use IsUsingOAuth here.  Or maybe decomp this into another function in oauth.h
> (e.g., GetAuthHeaders() or something)

Done.  Also fixed one other stray use of kEnableSyncOAuth in
SyncSetupWizard::GetLoginState.

http://codereview.chromium.org/7497069/diff/27019/chrome/browser/sync/util/oa...
File chrome/browser/sync/util/oauth.h (right):

http://codereview.chromium.org/7497069/diff/27019/chrome/browser/sync/util/oa...
chrome/browser/sync/util/oauth.h:9: namespace browser_sync {
On 2011/08/11 21:49:14, akalin wrote:
> space after namespace line

Done.

[commit-bot: I haz the power, 2011-08-12 10:44:48]

Change committed as 96544