sync: centralize sync startup decisions in TryStart.

sync: centralize sync startup decisions in TryStart.

Also addresses an issue on Chrome OS where we'd unnecessarily spin up
the DataTypeManager if the user cancelled out of a sync setup (after a
dashboard clear), and adds some comments.

Lastly some tests were firing TOKEN_LOADING_FINISHED on a fake / mock
TokenService, but calling TokensLoadedFromDB was still returning
false -- fixed that (needed for this patch).

BUG=80194, 128692
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170576

[tim (not reviewing), 2012-11-27 08:07:18]

Drew, please review!
Kochi & Raghu - please sanity my chrome os comments in sync_setup_handler and
the test file.
Nicolas - just FYI :)

[kochi, 2012-11-29 07:54:37]

Tim, could you try git cl upload again?

On the patch set 2, the unified diffs are working but SxS diffs
seems corrupt.

[tim (not reviewing), 2012-11-29 16:42:48]

re-uploaded! hope that works.

zea@, can you take over for atwilson?

[Nicolas Zea, 2012-11-29 19:43:08]

LGTM

https://chromiumcodereview.appspot.com/11348220/diff/10014/chrome/browser/sig...
File chrome/browser/signin/token_service.h (right):

https://chromiumcodereview.appspot.com/11348220/diff/10014/chrome/browser/sig...
chrome/browser/signin/token_service.h:199: protected:
newline above

https://chromiumcodereview.appspot.com/11348220/diff/10014/chrome/browser/syn...
File chrome/browser/sync/profile_sync_service.cc (right):

https://chromiumcodereview.appspot.com/11348220/diff/10014/chrome/browser/syn...
chrome/browser/sync/profile_sync_service.cc:274: if (HasSyncSetupCompleted() ||
setup_in_progress_ || auto_start_enabled_) {
Presumably this is necessary for when we bring up the backend and download
encryption info right? Maybe add comment about that below?

[tim (not reviewing), 2012-11-29 21:00:33]

Thanks!  I'll wait to let Kochi take a better look at it before committing.

https://chromiumcodereview.appspot.com/11348220/diff/10014/chrome/browser/sig...
File chrome/browser/signin/token_service.h (right):

https://chromiumcodereview.appspot.com/11348220/diff/10014/chrome/browser/sig...
chrome/browser/signin/token_service.h:199: protected:
On 2012/11/29 19:43:08, Nicolas Zea wrote:
> newline above

Done.

https://chromiumcodereview.appspot.com/11348220/diff/10014/chrome/browser/syn...
File chrome/browser/sync/profile_sync_service.cc (right):

https://chromiumcodereview.appspot.com/11348220/diff/10014/chrome/browser/syn...
chrome/browser/sync/profile_sync_service.cc:274: if (HasSyncSetupCompleted() ||
setup_in_progress_ || auto_start_enabled_) {
On 2012/11/29 19:43:08, Nicolas Zea wrote:
> Presumably this is necessary for when we bring up the backend and download
> encryption info right? Maybe add comment about that below?

Correct! Added comment.

[kochi, 2012-11-30 07:42:01]

lgtm with nits.

https://chromiumcodereview.appspot.com/11348220/diff/7004/chrome/browser/sign...
File chrome/browser/signin/token_service.cc (right):

https://chromiumcodereview.appspot.com/11348220/diff/7004/chrome/browser/sign...
chrome/browser/signin/token_service.cc:196: }
Why not inline (define in header)?
(maybe this is just a preference, but I'm curious)

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

https://chromiumcodereview.appspot.com/11348220/diff/7004/chrome/browser/sync...
chrome/browser/sync/profile_sync_service.cc:273: (!auto_start_enabled_ &&
token_service->TokensLoadedFromDB())) {
How about reversing these conditions just for return? i.e.

if (!IsSyncTokenAvailable() &&
    !(!auto_start_enabled_ && token_service->TokensLoadedFromDB())
  return;
...
if (!HasSyncSetupCompleted() && !setup_in_progress_ &&
    !auto_start_enabled_)
  return;

  // All green!
  StartUp();

Logically all these conditions can be combined in one
if (), but for the clarity, it may be better to discompose
the conditions and comment for each.  Compilers should be
able to optimize.

https://chromiumcodereview.appspot.com/11348220/diff/7004/chrome/browser/sync...
File chrome/browser/sync/profile_sync_service_startup_unittest.cc (right):

https://chromiumcodereview.appspot.com/11348220/diff/7004/chrome/browser/sync...
chrome/browser/sync/profile_sync_service_startup_unittest.cc:57:
TokenService::set_tokens_loaded(true);
nit: Do you need TokenService:: scope specifier here?

[Andrew T Wilson (Slow), 2012-11-30 14:52:14]

LGTM with nits.

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

https://chromiumcodereview.appspot.com/11348220/diff/7004/chrome/browser/sync...
chrome/browser/sync/profile_sync_service.cc:274: // Make sure it's safe to start
-- the user has either declared intent (
A dangling parenthesis? YOU MONSTER

https://chromiumcodereview.appspot.com/11348220/diff/7004/chrome/browser/sync...
chrome/browser/sync/profile_sync_service.cc:1724: // Previously-configured sync
has been re-enabled, so start sync now.
Comment is now obsolete since we don't know that sync has been configured (we no
longer check HasSyncSetupCompleted() in this routine). Maybe this should say
"Sync is no longer disabled by policy - try starting it up if appropriate."

https://chromiumcodereview.appspot.com/11348220/diff/7004/chrome/browser/sync...
chrome/browser/sync/profile_sync_service.cc:1778: } // Fall through.
Falling through is fine, although the code is simple enough now that you could
just duplicate those 4 lines.

[tim (not reviewing), 2012-11-30 18:51:13]

Thanks guys. Comments addressed. Will check the CQ box.

https://chromiumcodereview.appspot.com/11348220/diff/7004/chrome/browser/sign...
File chrome/browser/signin/token_service.cc (right):

https://chromiumcodereview.appspot.com/11348220/diff/7004/chrome/browser/sign...
chrome/browser/signin/token_service.cc:196: }
On 2012/11/30 07:42:02, Takayoshi Kochi wrote:
> Why not inline (define in header)?
> (maybe this is just a preference, but I'm curious)

No solid reason. I guess just overzealous application of
http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in....
 I changed / inlined it.

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

https://chromiumcodereview.appspot.com/11348220/diff/7004/chrome/browser/sync...
chrome/browser/sync/profile_sync_service.cc:273: (!auto_start_enabled_ &&
token_service->TokensLoadedFromDB())) {
On 2012/11/30 07:42:02, Takayoshi Kochi wrote:
> How about reversing these conditions just for return? i.e.
> 
> if (!IsSyncTokenAvailable() &&
>     !(!auto_start_enabled_ && token_service->TokensLoadedFromDB())
>   return;
> ...
> if (!HasSyncSetupCompleted() && !setup_in_progress_ &&
>     !auto_start_enabled_)
>   return;
> 
>   // All green!
>   StartUp();
> 
> Logically all these conditions can be combined in one
> if (), but for the clarity, it may be better to discompose
> the conditions and comment for each.  Compilers should be
> able to optimize.

I had played around with this but all the negatives made it feel equally
confusing to me :)  But, I changed it... willing to give it a whirl and let it
sink in!

https://chromiumcodereview.appspot.com/11348220/diff/7004/chrome/browser/sync...
chrome/browser/sync/profile_sync_service.cc:274: // Make sure it's safe to start
-- the user has either declared intent (
On 2012/11/30 14:52:15, Andrew T Wilson wrote:
> A dangling parenthesis? YOU MONSTER

Hah. Shame on me. Fixed.

https://chromiumcodereview.appspot.com/11348220/diff/7004/chrome/browser/sync...
chrome/browser/sync/profile_sync_service.cc:1724: // Previously-configured sync
has been re-enabled, so start sync now.
On 2012/11/30 14:52:15, Andrew T Wilson wrote:
> Comment is now obsolete since we don't know that sync has been configured (we
no
> longer check HasSyncSetupCompleted() in this routine). Maybe this should say
> "Sync is no longer disabled by policy - try starting it up if appropriate." 

Done.

https://chromiumcodereview.appspot.com/11348220/diff/7004/chrome/browser/sync...
chrome/browser/sync/profile_sync_service.cc:1778: } // Fall through.
On 2012/11/30 14:52:15, Andrew T Wilson wrote:
> Falling through is fine, although the code is simple enough now that you could
> just duplicate those 4 lines.

Done.

https://chromiumcodereview.appspot.com/11348220/diff/7004/chrome/browser/sync...
File chrome/browser/sync/profile_sync_service_startup_unittest.cc (right):

https://chromiumcodereview.appspot.com/11348220/diff/7004/chrome/browser/sync...
chrome/browser/sync/profile_sync_service_startup_unittest.cc:57:
TokenService::set_tokens_loaded(true);
On 2012/11/30 07:42:02, Takayoshi Kochi wrote:
> nit: Do you need TokenService:: scope specifier here?

Ah, no.  Fixed!

[commit-bot: I haz the power, 2012-11-30 18:52:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tim@chromium.org/11348220/2005

[commit-bot: I haz the power, 2012-11-30 18:52:36]

Presubmit check for 11348220-2005 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui/webui

Presubmit checks took 1.2s to calculate.

[tim (not reviewing), 2012-11-30 19:48:46]

Howdy James, could I get an OWNERS review for ui/webui/*?

[James Hawkins, 2012-11-30 19:54:08]

LGTM with nits.

https://chromiumcodereview.appspot.com/11348220/diff/2005/chrome/browser/ui/w...
File chrome/browser/ui/webui/sync_setup_handler_unittest.cc (right):

https://chromiumcodereview.appspot.com/11348220/diff/2005/chrome/browser/ui/w...
chrome/browser/ui/webui/sync_setup_handler_unittest.cc:501: // kick off
initialization, but not download user data types. It will try to
Does 'it' refer to the backend here?

https://chromiumcodereview.appspot.com/11348220/diff/2005/chrome/browser/ui/w...
chrome/browser/ui/webui/sync_setup_handler_unittest.cc:564: // Same as above,
except cancel after the sync backend has initialized but
Optional nit: Copy/paste part of the test description 'from above', as tests can
move around, be deleted, etc. then this would not necessarily point to the right
description.

[tim (not reviewing), 2012-11-30 20:08:08]

Thanks! Addressed comments, trying CQ again.

https://chromiumcodereview.appspot.com/11348220/diff/2005/chrome/browser/ui/w...
File chrome/browser/ui/webui/sync_setup_handler_unittest.cc (right):

https://chromiumcodereview.appspot.com/11348220/diff/2005/chrome/browser/ui/w...
chrome/browser/ui/webui/sync_setup_handler_unittest.cc:501: // kick off
initialization, but not download user data types. It will try to
On 2012/11/30 19:54:08, James Hawkins wrote:
> Does 'it' refer to the backend here?

Yeah.  Changed to make it more obvious.

https://chromiumcodereview.appspot.com/11348220/diff/2005/chrome/browser/ui/w...
chrome/browser/ui/webui/sync_setup_handler_unittest.cc:564: // Same as above,
except cancel after the sync backend has initialized but
On 2012/11/30 19:54:08, James Hawkins wrote:
> Optional nit: Copy/paste part of the test description 'from above', as tests
can
> move around, be deleted, etc. then this would not necessarily point to the
right
> description.

Done.

[commit-bot: I haz the power, 2012-11-30 20:08:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tim@chromium.org/11348220/5008

[commit-bot: I haz the power, 2012-11-30 22:25:56]

Change committed as 170576