sync: some fixups for --sync-enabled-deferred-init

sync: some fixups for --sync-enabled-deferred-init

TBR=dhollowa@chromium.org
^ web_data_service_factory.cc

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

[tim (not reviewing), 2013-05-15 16:15:55]

On 2013/05/12 22:50:22, timsteele wrote:

ping

[haitaol1, 2013-05-15 20:20:31]

https://codereview.chromium.org/14735010/diff/1/chrome/browser/sync/profile_s...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/14735010/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:508: if (start_up_time_.is_null()) {
Do you have to put all the block in if? Is it ok to set only start_up_time_ if
it's null?

[tim (not reviewing), 2013-05-15 23:18:13]

https://codereview.chromium.org/14735010/diff/1/chrome/browser/sync/profile_s...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/14735010/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:508: if (start_up_time_.is_null()) {
On 2013/05/15 20:20:31, haitaol1 wrote:
> Do you have to put all the block in if? Is it ok to set only start_up_time_ if
> it's null?

It is currently used to report the time spent deferred since the first call to
StartUp, which will be ProfileSyncService Initialization, which is done on
construction by the factory, which will eventually be done very early on when
the Profile is created.

[haitaol1, 2013-05-16 00:05:57]

https://codereview.chromium.org/14735010/diff/1/chrome/browser/sync/profile_s...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/14735010/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:508: if (start_up_time_.is_null()) {
Lock-free shutdown may be an issue in future because start_up_time_ is not null
if backend is shut down before initialized. Can you move 
start_up_time_ = base::Time();
in OnBackendInitialized() to ShutdownImpl()?

On 2013/05/15 23:18:14, timsteele wrote:
> On 2013/05/15 20:20:31, haitaol1 wrote:
> > Do you have to put all the block in if? Is it ok to set only start_up_time_
if
> > it's null?
> 
> It is currently used to report the time spent deferred since the first call to
> StartUp, which will be ProfileSyncService Initialization, which is done on
> construction by the factory, which will eventually be done very early on when
> the Profile is created.

https://codereview.chromium.org/14735010/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:525: sync_global_error_.get());
sync_global_error_ is NULL if defined(OS_ANDROID). Is that right?

[haitaol1, 2013-05-16 00:06:59]

https://codereview.chromium.org/14735010/diff/1/chrome/browser/sync/profile_s...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/14735010/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:508: if (start_up_time_.is_null()) {
Actually, that could happen today, right?

On 2013/05/16 00:05:57, haitaol1 wrote:
> Lock-free shutdown may be an issue in future because start_up_time_ is not
null
> if backend is shut down before initialized. Can you move 
> start_up_time_ = base::Time();
> in OnBackendInitialized() to ShutdownImpl()?
> 
> On 2013/05/15 23:18:14, timsteele wrote:
> > On 2013/05/15 20:20:31, haitaol1 wrote:
> > > Do you have to put all the block in if? Is it ok to set only
start_up_time_
> if
> > > it's null?
> > 
> > It is currently used to report the time spent deferred since the first call
to
> > StartUp, which will be ProfileSyncService Initialization, which is done on
> > construction by the factory, which will eventually be done very early on
when
> > the Profile is created.
>

[tim (not reviewing), 2013-05-16 00:17:24]

https://codereview.chromium.org/14735010/diff/1/chrome/browser/sync/profile_s...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/14735010/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:508: if (start_up_time_.is_null()) {
On 2013/05/16 00:06:59, haitaol1 wrote:
> Actually, that could happen today, right?
> 
> On 2013/05/16 00:05:57, haitaol1 wrote:
> > Lock-free shutdown may be an issue in future because start_up_time_ is not
> null
> > if backend is shut down before initialized. Can you move 
> > start_up_time_ = base::Time();
> > in OnBackendInitialized() to ShutdownImpl()?
> > 
> > On 2013/05/15 23:18:14, timsteele wrote:
> > > On 2013/05/15 20:20:31, haitaol1 wrote:
> > > > Do you have to put all the block in if? Is it ok to set only
> start_up_time_
> > if
> > > > it's null?
> > > 
> > > It is currently used to report the time spent deferred since the first
call
> to
> > > StartUp, which will be ProfileSyncService Initialization, which is done on
> > > construction by the factory, which will eventually be done very early on
> when
> > > the Profile is created.
> > 
> 

OnBackendInitialized should always be called if we've managed to ask the
SyncManager to Init itself, so I don't think this causes a problem in practice. 
But it seems fine to move it, so I did.

https://codereview.chromium.org/14735010/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:525: sync_global_error_.get());
On 2013/05/16 00:05:57, haitaol1 wrote:
> sync_global_error_ is NULL if defined(OS_ANDROID). Is that right?

I believe so, because we don't have errors showing up as Tools menu items on
Android.

[haitaol1, 2013-05-16 16:17:33]

lgtm

https://codereview.chromium.org/14735010/diff/1/chrome/browser/sync/profile_s...
File chrome/browser/sync/profile_sync_service.cc (right):

https://codereview.chromium.org/14735010/diff/1/chrome/browser/sync/profile_s...
chrome/browser/sync/profile_sync_service.cc:508: if (start_up_time_.is_null()) {
SyncBackendHost::StopSyncingForShutdown nulls frontend_. So I don't think
OnBackendInitialized is guaranteed to be called. Actually, another case where
start_up_time_ is not reset on shutdown is when shutdown is called but backend
is in deferred startup mode. 

On 2013/05/16 00:17:25, timsteele wrote:
> On 2013/05/16 00:06:59, haitaol1 wrote:
> > Actually, that could happen today, right?
> > 
> > On 2013/05/16 00:05:57, haitaol1 wrote:
> > > Lock-free shutdown may be an issue in future because start_up_time_ is not
> > null
> > > if backend is shut down before initialized. Can you move 
> > > start_up_time_ = base::Time();
> > > in OnBackendInitialized() to ShutdownImpl()?
> > > 
> > > On 2013/05/15 23:18:14, timsteele wrote:
> > > > On 2013/05/15 20:20:31, haitaol1 wrote:
> > > > > Do you have to put all the block in if? Is it ok to set only
> > start_up_time_
> > > if
> > > > > it's null?
> > > > 
> > > > It is currently used to report the time spent deferred since the first
> call
> > to
> > > > StartUp, which will be ProfileSyncService Initialization, which is done
on
> > > > construction by the factory, which will eventually be done very early on
> > when
> > > > the Profile is created.
> > > 
> > 
> 
> OnBackendInitialized should always be called if we've managed to ask the
> SyncManager to Init itself, so I don't think this causes a problem in
practice. 
> But it seems fine to move it, so I did.

[commit-bot: I haz the power, 2013-05-16 16:40:40]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[tim (not reviewing), 2013-05-16 16:56:09]

+zea for OWNERS stamp.

[tim (not reviewing), 2013-05-16 16:56:35]

On 2013/05/16 16:56:09, timsteele wrote:
> +zea for OWNERS stamp.

err, full committer review, I should say :)

[Nicolas Zea, 2013-05-16 18:09:37]

lgtm

[commit-bot: I haz the power, 2013-05-16 18:17:12]

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

[commit-bot: I haz the power, 2013-05-16 18:27:43]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-05-16 18:37:09]

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

[tim (not reviewing), 2013-05-17 01:29:26]

Committed patchset #2 manually as r200686 (presubmit successful).