[Sync] Handle errors during first sync gracefully.

Currently when there are errors during first sync(configure) we trigger an  unrecoverable error. 
This change would make the PSS wait for the retry sync and also inform the wizard that we are waiting.
The wizard then can decide to wait and show a screen for that. However currently the wizard would throw the unrecoverable error.

BUG=100076, 111676
TEST=try bots and manual.


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

[lipalani1, 2012-01-25 22:16:49]

Please review.

[lipalani1, 2012-01-26 21:00:33]

On 2012/01/25 22:16:49, lipalani1 wrote:
> Please review.

Ping! I would like to make the cutoff tmw if possible. I just sent the test
cases for review as well.

[Andrew T Wilson (Slow), 2012-01-27 00:19:03]

http://codereview.chromium.org/9235040/diff/6001/chrome/browser/sync/glue/dat...
File chrome/browser/sync/glue/data_type_manager.h (right):

http://codereview.chromium.org/9235040/diff/6001/chrome/browser/sync/glue/dat...
chrome/browser/sync/glue/data_type_manager.h:48: // is being retried.
I'm not sure I actively object to this, but I note that all of these status
codes correspond to an end state (configuration is done, either successful or
not), while RETRY implies that configuration is not yet done.

It just seems weird to send a CONFIGURATION_DONE notification, but with a status
code that says "hah, not actually done". This might be resolved with some
renaming or by sending out a different notification - it's not clear to me that
a notification that we're having to retry the download should piggy-back on the
CONFIGURATION_DONE notification.

http://codereview.chromium.org/9235040/diff/6001/chrome/browser/sync/glue/dat...
File chrome/browser/sync/glue/data_type_manager_impl.cc (right):

http://codereview.chromium.org/9235040/diff/6001/chrome/browser/sync/glue/dat...
chrome/browser/sync/glue/data_type_manager_impl.cc:280:
chrome::NOTIFICATION_SYNC_CONFIGURE_DONE,
See my previous comment - perhaps there's a better way to notify observers of
this state rather than via CONFIGURE_DONE.

http://codereview.chromium.org/9235040/diff/6001/chrome/browser/sync/glue/syn...
File chrome/browser/sync/glue/sync_backend_host.h (right):

http://codereview.chromium.org/9235040/diff/6001/chrome/browser/sync/glue/syn...
chrome/browser/sync/glue/sync_backend_host.h:347: // that we will not notify DTM
until the sync is succesful or in a
nit: succesful->successful

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

http://codereview.chromium.org/9235040/diff/6001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:358: if (wizard_.IsVisible()) {
Instead of forwarding this to the SyncSetupWizard (see my comment in
sync_setup_flow.cc) I think we should do this:

// If we're not doing autostart, then abort sync setup if our configuration
fails.
if (!auto_start_enabled_)
  OnUnrecoverableError(....);

http://codereview.chromium.org/9235040/diff/6001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:359:
wizard_.Step(SyncSetupWizard::CONFIGURE_RETRY);
Soon, SyncSetupWizard will be going away. Instead, the UI will register itself
as an Observer, and will need to query for things like "retry status" via APIs,
rather than relying on the ProfileSyncService directly modifying state in
SyncSetupWizard. This will also allow things like Clank to provide feedback to
the user without actually having to use SyncSetupWizard.

So, can we track and expose this retry state through a member variable and also
notify observers when the status of that variable changes?

http://codereview.chromium.org/9235040/diff/6001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:1371: // Hanlde unrecoverable error.
nit: Hanlde->Handle

http://codereview.chromium.org/9235040/diff/6001/chrome/browser/sync/sync_set...
File chrome/browser/sync/sync_setup_flow.cc (right):

http://codereview.chromium.org/9235040/diff/6001/chrome/browser/sync/sync_set...
chrome/browser/sync/sync_setup_flow.cc:653: // for handling this.
This is a little weird, because now we're in CONFIGURE_RETRY state, and I'm not
sure what other platforms like chromeos ought to do here - force-step to a
different state? I don't think the logic in ShouldAdvance will let us transition
out of CONFIGURE_RETRY to anything but FATAL_ERROR.

Maybe the cleaner way to do this is to not have a CONFIGURE_RETRY state at all,
but to instead check the auto_start_enabled_ state as I suggested previously.

http://codereview.chromium.org/9235040/diff/6001/chrome/browser/sync/sync_set...
File chrome/browser/sync/sync_setup_wizard.h (right):

http://codereview.chromium.org/9235040/diff/6001/chrome/browser/sync/sync_set...
chrome/browser/sync/sync_setup_wizard.h:39: // It is upto each platform to
implement the screen. In default case
upto->up to

[lipalani1, 2012-01-27 02:54:29]

Had a conversation with Drew and we arrived at some solutions.

This patch implements the PSS layer and sync_setup_flow.cc as discussed in the
conversation. The primary motivation for this change is to give something usable
in M18 but this code would have to change to some extend once the major UI
refactor happens.

PTAL.

http://codereview.chromium.org/9235040/diff/6001/chrome/browser/sync/glue/dat...
File chrome/browser/sync/glue/data_type_manager.h (right):

http://codereview.chromium.org/9235040/diff/6001/chrome/browser/sync/glue/dat...
chrome/browser/sync/glue/data_type_manager.h:48: // is being retried.
I agree with you here. I should probably have put a TODO there. While coding it
up I noticed the name was weird. SYNC_CONFIGURE_DONE might be sent for ABORT! 
I decided against introducing a new notification as that seems too much for
this.
The better way would be to rename this but that would involve renaming in about
9 files(17 places). So would prefer to do it as another patch with a TODO in the
place where it is used.
On 2012/01/27 00:19:03, Andrew T Wilson wrote:
> I'm not sure I actively object to this, but I note that all of these status
> codes correspond to an end state (configuration is done, either successful or
> not), while RETRY implies that configuration is not yet done.
> 
> It just seems weird to send a CONFIGURATION_DONE notification, but with a
status
> code that says "hah, not actually done". This might be resolved with some
> renaming or by sending out a different notification - it's not clear to me
that
> a notification that we're having to retry the download should piggy-back on
the
> CONFIGURATION_DONE notification.

http://codereview.chromium.org/9235040/diff/6001/chrome/browser/sync/glue/dat...
File chrome/browser/sync/glue/data_type_manager_impl.cc (right):

http://codereview.chromium.org/9235040/diff/6001/chrome/browser/sync/glue/dat...
chrome/browser/sync/glue/data_type_manager_impl.cc:280:
chrome::NOTIFICATION_SYNC_CONFIGURE_DONE,
Answered above.
On 2012/01/27 00:19:03, Andrew T Wilson wrote:
> See my previous comment - perhaps there's a better way to notify observers of
> this state rather than via CONFIGURE_DONE.

http://codereview.chromium.org/9235040/diff/6001/chrome/browser/sync/glue/syn...
File chrome/browser/sync/glue/sync_backend_host.h (right):

http://codereview.chromium.org/9235040/diff/6001/chrome/browser/sync/glue/syn...
chrome/browser/sync/glue/sync_backend_host.h:347: // that we will not notify DTM
until the sync is succesful or in a
On 2012/01/27 00:19:03, Andrew T Wilson wrote:
> nit: succesful->successful

Done.

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

http://codereview.chromium.org/9235040/diff/6001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:358: if (wizard_.IsVisible()) {
On 2012/01/27 00:19:03, Andrew T Wilson wrote:
> Instead of forwarding this to the SyncSetupWizard (see my comment in
> sync_setup_flow.cc) I think we should do this:
> 
> // If we're not doing autostart, then abort sync setup if our configuration
> fails.
> if (!auto_start_enabled_)
>   OnUnrecoverableError(....);

Done.

http://codereview.chromium.org/9235040/diff/6001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:359:
wizard_.Step(SyncSetupWizard::CONFIGURE_RETRY);
On 2012/01/27 00:19:03, Andrew T Wilson wrote:
> Soon, SyncSetupWizard will be going away. Instead, the UI will register itself
> as an Observer, and will need to query for things like "retry status" via
APIs,
> rather than relying on the ProfileSyncService directly modifying state in
> SyncSetupWizard. This will also allow things like Clank to provide feedback to
> the user without actually having to use SyncSetupWizard.
> 
> So, can we track and expose this retry state through a member variable and
also
> notify observers when the status of that variable changes?

Done.

http://codereview.chromium.org/9235040/diff/6001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:1371: // Hanlde unrecoverable error.
On 2012/01/27 00:19:03, Andrew T Wilson wrote:
> nit: Hanlde->Handle

Done.

http://codereview.chromium.org/9235040/diff/6001/chrome/browser/sync/sync_set...
File chrome/browser/sync/sync_setup_flow.cc (right):

http://codereview.chromium.org/9235040/diff/6001/chrome/browser/sync/sync_set...
chrome/browser/sync/sync_setup_flow.cc:653: // for handling this.
This code is removed.
On 2012/01/27 00:19:03, Andrew T Wilson wrote:
> This is a little weird, because now we're in CONFIGURE_RETRY state, and I'm
not
> sure what other platforms like chromeos ought to do here - force-step to a
> different state? I don't think the logic in ShouldAdvance will let us
transition
> out of CONFIGURE_RETRY to anything but FATAL_ERROR.
> 
> Maybe the cleaner way to do this is to not have a CONFIGURE_RETRY state at
all,
> but to instead check the auto_start_enabled_ state as I suggested previously.

http://codereview.chromium.org/9235040/diff/6001/chrome/browser/sync/sync_set...
File chrome/browser/sync/sync_setup_wizard.h (right):

http://codereview.chromium.org/9235040/diff/6001/chrome/browser/sync/sync_set...
chrome/browser/sync/sync_setup_wizard.h:39: // It is upto each platform to
implement the screen. In default case
Removed.
On 2012/01/27 00:19:03, Andrew T Wilson wrote:
> upto->up to

[tim (not reviewing), 2012-01-27 03:32:38]

http://codereview.chromium.org/9235040/diff/6001/chrome/browser/sync/glue/dat...
File chrome/browser/sync/glue/data_type_manager.h (right):

http://codereview.chromium.org/9235040/diff/6001/chrome/browser/sync/glue/dat...
chrome/browser/sync/glue/data_type_manager.h:48: // is being retried.
On 2012/01/27 02:54:29, lipalani1 wrote:
> I agree with you here. I should probably have put a TODO there. While coding
it
> up I noticed the name was weird. SYNC_CONFIGURE_DONE might be sent for ABORT! 
> I decided against introducing a new notification as that seems too much for
> this.
> The better way would be to rename this but that would involve renaming in
about
> 9 files(17 places). So would prefer to do it as another patch with a TODO in
the
> place where it is used.
> On 2012/01/27 00:19:03, Andrew T Wilson wrote:
> > I'm not sure I actively object to this, but I note that all of these status
> > codes correspond to an end state (configuration is done, either successful
or
> > not), while RETRY implies that configuration is not yet done.
> > 
> > It just seems weird to send a CONFIGURATION_DONE notification, but with a
> status
> > code that says "hah, not actually done". This might be resolved with some
> > renaming or by sending out a different notification - it's not clear to me
> that
> > a notification that we're having to retry the download should piggy-back on
> the
> > CONFIGURATION_DONE notification.
> 

ABORT still implies the DTM is stopped / not proceeding further, so DONE seems
to make sense...

[Andrew T Wilson (Slow), 2012-01-27 17:41:42]

On 2012/01/27 03:32:38, timsteele wrote:
> ABORT still implies the DTM is stopped / not proceeding further, so DONE seems
> to make sense...

That was my thinking as well. I think it's not difficult to add a new
notification, but I'm OK with a TODO() for now in the interest of landing this
today.

[tim (not reviewing), 2012-01-27 17:52:24]

On 2012/01/27 17:41:42, Andrew T Wilson wrote:
> On 2012/01/27 03:32:38, timsteele wrote:
> > ABORT still implies the DTM is stopped / not proceeding further, so DONE
seems
> > to make sense...
> 
> That was my thinking as well. I think it's not difficult to add a new
> notification, but I'm OK with a TODO() for now in the interest of landing this
> today.

OK... so long as there's an accompanying cleanup bug assigned to Lingesh on m19,
then I agree this is fine.

[Andrew T Wilson (Slow), 2012-01-27 18:41:11]

LGTM

http://codereview.chromium.org/9235040/diff/8001/chrome/browser/sync/glue/dat...
File chrome/browser/sync/glue/data_type_manager_impl.cc (right):

http://codereview.chromium.org/9235040/diff/8001/chrome/browser/sync/glue/dat...
chrome/browser/sync/glue/data_type_manager_impl.cc:281: //
NOTIFICATION_SYNC_CONFIGURE_STATUS.
Do we have consensus that a rename is what we want here (and not a separate
notification)?

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

http://codereview.chromium.org/9235040/diff/8001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:360: // an unrecoverable error.
Add a note that the reason we do this is to force the login process to fail and
display a login error to the user.

http://codereview.chromium.org/9235040/diff/8001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:365: // to finish
nit: period at end of sentence.

http://codereview.chromium.org/9235040/diff/8001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:1360: //            initiated by
user..
nit: double '..'

[lipalani1, 2012-01-27 19:35:26]

fyi all nits fixed.

Running try once more and planning to commit right after that.

thanks alot!

http://codereview.chromium.org/9235040/diff/8001/chrome/browser/sync/glue/dat...
File chrome/browser/sync/glue/data_type_manager_impl.cc (right):

http://codereview.chromium.org/9235040/diff/8001/chrome/browser/sync/glue/dat...
chrome/browser/sync/glue/data_type_manager_impl.cc:281: //
NOTIFICATION_SYNC_CONFIGURE_STATUS.
added a todo with a bug for new notificatin.
On 2012/01/27 18:41:11, Andrew T Wilson wrote:
> Do we have consensus that a rename is what we want here (and not a separate
> notification)?

Done.

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

http://codereview.chromium.org/9235040/diff/8001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:360: // an unrecoverable error.
On 2012/01/27 18:41:11, Andrew T Wilson wrote:
> Add a note that the reason we do this is to force the login process to fail
and
> display a login error to the user.

Done.

http://codereview.chromium.org/9235040/diff/8001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:365: // to finish
On 2012/01/27 18:41:11, Andrew T Wilson wrote:
> nit: period at end of sentence.

Done.

http://codereview.chromium.org/9235040/diff/8001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:1360: //            initiated by
user..
On 2012/01/27 18:41:11, Andrew T Wilson wrote:
> nit: double '..'

Done.

[commit-bot: I haz the power, 2012-01-27 22:31:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lipalani@chromium.org/9235040/9016

[commit-bot: I haz the power, 2012-01-28 02:20:58]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2012-01-28 22:44:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lipalani@chromium.org/9235040/9016

[commit-bot: I haz the power, 2012-01-29 08:57:56]

Change committed as 119633