[Sync] Cleanup datatype configuration error handling.

[Sync] Cleanup datatype configuration error handling.

The FailedDataTypeHandler is now informed immediately of failures, including
datatype errors, and is therefore authoritative source for all errors. As such,
partial success is no longer tracked and the various ModelTypeSets for error
types in configure results are removed.

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

[Nicolas Zea, 2014-07-28 20:19:10]

+Haitoa, PTAL

[Nicolas Zea, 2014-07-30 17:32:36]

ping?

[haitaol1, 2014-07-30 23:02:44]

https://codereview.chromium.org/420633002/diff/60001/components/sync_driver/d...
File components/sync_driver/data_type_manager_impl.cc (right):

https://codereview.chromium.org/420633002/diff/60001/components/sync_driver/d...
components/sync_driver/data_type_manager_impl.cc:92:
!failed_data_types_handler_->GetUnreadyErrorTypes().Has(
Is the end result same as before, i.e. unready type is tagged in
failed_data_types_handler_? If so, current code seems more straightforward even
with a little redundancy.

https://codereview.chromium.org/420633002/diff/60001/components/sync_driver/d...
components/sync_driver/data_type_manager_impl.cc:357:
errors[failed_configuration_types.First().Get()] = error;
Do you need to set error for all failed types?

https://codereview.chromium.org/420633002/diff/60001/components/sync_driver/d...
components/sync_driver/data_type_manager_impl.cc:420: if (state_ == CONFIGURED)
Doesn't hurt to always call ProcessReconfigure().

https://codereview.chromium.org/420633002/diff/60001/components/sync_driver/m...
File components/sync_driver/model_association_manager.cc (right):

https://codereview.chromium.org/420633002/diff/60001/components/sync_driver/m...
components/sync_driver/model_association_manager.cc:159: if (error.IsSet()) {
-> error.IsSet() || dtc->state() != DataTypeController::NOT_RUNNING

[Nicolas Zea, 2014-07-30 23:20:36]

Comments addressed, PTAL

https://codereview.chromium.org/420633002/diff/60001/components/sync_driver/d...
File components/sync_driver/data_type_manager_impl.cc (right):

https://codereview.chromium.org/420633002/diff/60001/components/sync_driver/d...
components/sync_driver/data_type_manager_impl.cc:92:
!failed_data_types_handler_->GetUnreadyErrorTypes().Has(
On 2014/07/30 23:02:44, haitaol1 wrote:
> Is the end result same as before, i.e. unready type is tagged in
> failed_data_types_handler_? If so, current code seems more straightforward
even
> with a little redundancy.

It is, but without this condition we'll be spamming the console with the errors.

https://codereview.chromium.org/420633002/diff/60001/components/sync_driver/d...
components/sync_driver/data_type_manager_impl.cc:357:
errors[failed_configuration_types.First().Get()] = error;
On 2014/07/30 23:02:44, haitaol1 wrote:
> Do you need to set error for all failed types?

There's only support for tracking one unrecoverable error at a time right now.
It results in shutting down the syncer, so tracking more than that doesn't seem
necessary.

https://codereview.chromium.org/420633002/diff/60001/components/sync_driver/d...
components/sync_driver/data_type_manager_impl.cc:420: if (state_ == CONFIGURED)
On 2014/07/30 23:02:44, haitaol1 wrote:
> Doesn't hurt to always call ProcessReconfigure().

Done.

https://codereview.chromium.org/420633002/diff/60001/components/sync_driver/m...
File components/sync_driver/model_association_manager.cc (right):

https://codereview.chromium.org/420633002/diff/60001/components/sync_driver/m...
components/sync_driver/model_association_manager.cc:159: if (error.IsSet()) {
On 2014/07/30 23:02:44, haitaol1 wrote:
> -> error.IsSet() || dtc->state() != DataTypeController::NOT_RUNNING

Done.

[Nicolas Zea, 2014-07-31 20:40:22]

PTAL, changed unrecoverable errors to track all types.

[haitaol1, 2014-07-31 20:51:24]

https://codereview.chromium.org/420633002/diff/120001/components/sync_driver/...
File components/sync_driver/data_type_manager_impl.cc (right):

https://codereview.chromium.org/420633002/diff/120001/components/sync_driver/...
components/sync_driver/data_type_manager_impl.cc:358:
errors[failed_configuration_types.First().Get()] = error;
shouldn't iter be used here?

https://codereview.chromium.org/420633002/diff/120001/components/sync_driver/...
File components/sync_driver/failed_data_types_handler.cc (right):

https://codereview.chromium.org/420633002/diff/120001/components/sync_driver/...
components/sync_driver/failed_data_types_handler.cc:144: return
unrecoverable_errors_.begin()->second;
crash if unrecoverable_errors_ is empty?

https://codereview.chromium.org/420633002/diff/120001/components/sync_driver/...
File components/sync_driver/failed_data_types_handler.h (right):

https://codereview.chromium.org/420633002/diff/120001/components/sync_driver/...
components/sync_driver/failed_data_types_handler.h:68: syncer::SyncError
GetUnrecoverableError() const;
Where is this used?

[Nicolas Zea, 2014-07-31 20:55:39]

https://codereview.chromium.org/420633002/diff/120001/components/sync_driver/...
File components/sync_driver/data_type_manager_impl.cc (right):

https://codereview.chromium.org/420633002/diff/120001/components/sync_driver/...
components/sync_driver/data_type_manager_impl.cc:358:
errors[failed_configuration_types.First().Get()] = error;
On 2014/07/31 20:51:24, haitaol1 wrote:
> shouldn't iter be used here?

Done.

https://codereview.chromium.org/420633002/diff/120001/components/sync_driver/...
File components/sync_driver/failed_data_types_handler.cc (right):

https://codereview.chromium.org/420633002/diff/120001/components/sync_driver/...
components/sync_driver/failed_data_types_handler.cc:144: return
unrecoverable_errors_.begin()->second;
On 2014/07/31 20:51:24, haitaol1 wrote:
> crash if unrecoverable_errors_ is empty?

Done.

https://codereview.chromium.org/420633002/diff/120001/components/sync_driver/...
File components/sync_driver/failed_data_types_handler.h (right):

https://codereview.chromium.org/420633002/diff/120001/components/sync_driver/...
components/sync_driver/failed_data_types_handler.h:68: syncer::SyncError
GetUnrecoverableError() const;
On 2014/07/31 20:51:24, haitaol1 wrote:
> Where is this used?

In ProfileSyncService. We use the full error message from the SyncError to
update about:sync.

[haitaol1, 2014-07-31 21:05:03]

lgtm

[Nicolas Zea, 2014-07-31 21:06:01]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-07-31 21:07:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zea@chromium.org/420633002/160001

[commit-bot: I haz the power, 2014-08-01 04:31:15]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/41299)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-01 04:35:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-01 04:35:51]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[Nicolas Zea, 2014-08-04 20:31:13]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-08-04 20:32:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zea@chromium.org/420633002/180001

[commit-bot: I haz the power, 2014-08-04 21:45:16]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/42233)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-04 21:50:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-04 21:50:34]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)

[haitaol1, 2014-08-08 17:28:25]

Sorry for looping back. Have a couple more questions.

https://codereview.chromium.org/420633002/diff/180001/components/sync_driver/...
File components/sync_driver/model_association_manager.cc (right):

https://codereview.chromium.org/420633002/diff/180001/components/sync_driver/...
components/sync_driver/model_association_manager.cc:162: dtc->Stop();
Should the type be stopped regardless whether error is set? E.g.
StopDisabledTypes() calls StopDatatype() with no error

https://codereview.chromium.org/420633002/diff/180001/components/sync_driver/...
components/sync_driver/model_association_manager.cc:337: if
(!desired_types_.Has(type)) {
Should the type be stopped if it's not desired any more?

[Nicolas Zea, 2014-08-08 17:35:34]

https://codereview.chromium.org/420633002/diff/180001/components/sync_driver/...
File components/sync_driver/model_association_manager.cc (right):

https://codereview.chromium.org/420633002/diff/180001/components/sync_driver/...
components/sync_driver/model_association_manager.cc:162: dtc->Stop();
On 2014/08/08 17:28:25, haitaol1 wrote:
> Should the type be stopped regardless whether error is set? E.g.
> StopDisabledTypes() calls StopDatatype() with no error

Yes. This is the codepath used at normal type shutdown time, where we always
want to stop the type.

https://codereview.chromium.org/420633002/diff/180001/components/sync_driver/...
components/sync_driver/model_association_manager.cc:337: if
(!desired_types_.Has(type)) {
On 2014/08/08 17:28:25, haitaol1 wrote:
> Should the type be stopped if it's not desired any more?

Yes. That means the user either explicitly wants to disable it, in which case we
should stop it, or that it encountered an error, in which case we should stop
it.

[Nicolas Zea, 2014-08-08 18:45:23]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2014-08-08 18:47:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zea@chromium.org/420633002/200001

[commit-bot: I haz the power, 2014-08-08 23:16:10]

Change committed as 288464