[Sync] Introduce ModelError for USS error handling.

[Sync] Introduce ModelError for USS error handling.

SyncError is confusing to use in model code, because it has features
(error type and model type) that are only needed for inner sync
machinery. This change introduces the simplified ModelError, which only
has a location and a message. If the need arises, it could also get an
error type/desired recovery action value as well, but I'm doubtful
that's necessary and I'm sure we can make something clearer than
SyncError's.

- SyncError replaced by ModelError everywhere below the processor.
- SMTP no longer implements SyncErrorFactory and now just has a
  ReportError method (with one convenience overload).
- SMTP now expects ReportError to be called instead of having error
  objects as callback parameters. This streamlines error handling quite
  a bit.
- Added error handling for Get(All)Data in AutocompleteSyncBridge.

I plan to convert everything between the processor and the controller to
use ModelError in a followup CL.

BUG=673883
Review-Url: https://codereview.chromium.org/2618483003
Cr-Commit-Position: refs/heads/master@{#442401}
Committed: https://chromium.googlesource.com/chromium/src/+/04ada5a89fbd87369e96d434aa39aee90d20809c

[maxbogue, 2017-01-04 19:16:37]

The CQ bit was checked by maxbogue@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-04 19:16:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-04 19:24:14]

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

[commit-bot: I haz the power, 2017-01-04 19:24:15]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[maxbogue, 2017-01-04 23:15:31]

The CQ bit was checked by maxbogue@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-04 23:15:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-05 00:14:02]

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

[commit-bot: I haz the power, 2017-01-05 00:14:02]

Dry run: This issue passed the CQ dry run.

[maxbogue, 2017-01-05 00:18:46]

maxbogue@chromium.org changed reviewers:
+ pavely@chromium.org

[maxbogue, 2017-01-05 00:18:48]

Sky, PTAL!

[skym, 2017-01-05 16:36:12]

Description was changed from

==========
[Sync] Make ModelTypeChangeProcessor extend DataTypeErrorHandler.

Now sync bridges have a way to report errors. Copy() is a hack for some
weird directory situations that should probably be removed in the
future, so I just put NOTREACHED() for the USS classes.

BUG=673883
==========

to

==========
[Sync] Make ModelTypeChangeProcessor extend DataTypeErrorHandler.

Now sync bridges have a way to report errors. Copy() is a hack for some
weird directory situations that should probably be removed in the
future, so I just put NOTREACHED() for the USS classes.

BUG=673883
==========

[skym, 2017-01-05 16:36:12]

skym@chromium.org changed reviewers:
+ skym@chromium.org
- pavely@chromium.org

[skym, 2017-01-05 17:03:49]

I don't see the actual benefit of implementing DataTypeErrorHandler, other than
it does a kinda similar thing to what we need. No one is holding onto a
reference to the proc as a DataTypeErrorHandler object. The bridge just needs a
way to report errors. Should we diverge from existing interfaces? WDYT?

https://codereview.chromium.org/2618483003/diff/20001/components/sync/device_...
File components/sync/device_info/device_info_sync_bridge.cc (right):

https://codereview.chromium.org/2618483003/diff/20001/components/sync/device_...
components/sync/device_info/device_info_sync_bridge.cc:480:
change_processor()->CreateAndUploadError(FROM_HERE, msg, type()),
What if, we instead abandon the old error interfaces and write new ones. We
don't have to NOTREACHED in Copy(). We don't have to supply a type to
CreateAndUploadError(). I don't really like the existence of
CreateAndUploadError(). First, it shouldn't upload. that should be done in
OnUnrecoverableError() or OnMetadataLoaded(). Except AndUpload is in the name.

But, I don't think CreateAndUploadError() is even needed if we were to remove
SyncErrors from OnMetadataLoaded(). Can we instead have the bridge just call
OnUnrecoverableError() and never get around to OnMetadataLoaded? Making sure
OnMetadataLoaded() is always called is a hassle for the bridge, and can be error
prone when encountering errors.

Hmm, you don't even need to start with a new interface. Just create a one off
method in the Proc.

void SharedModelTypeProcessor::ReportUnrecoverableError(const
tracked_objects::Location& location, const std::string& message) {
  ...
}

https://codereview.chromium.org/2618483003/diff/20001/components/sync/model_i...
File components/sync/model_impl/shared_model_type_processor.cc (right):

https://codereview.chromium.org/2618483003/diff/20001/components/sync/model_i...
components/sync/model_impl/shared_model_type_processor.cc:61: ConnectIfReady();
While you're in here, would be nice to explain why we connect if we're in an
error state. I think this is at first intuitively backwards.

https://codereview.chromium.org/2618483003/diff/20001/components/sync/model_i...
components/sync/model_impl/shared_model_type_processor.cc:155: ModelType type) {
Doesn't seem like type is very useful to be part of this interface. I guess you
could DCHECK that type == type_.

https://codereview.chromium.org/2618483003/diff/20001/components/sync/model_i...
components/sync/model_impl/shared_model_type_processor.cc:164: NOTREACHED();
I think this probably deserves a comment. Could explain why we're in this
situation, why Copy() doesn't make sense for this class.

[maxbogue, 2017-01-07 00:09:11]

Description was changed from

==========
[Sync] Make ModelTypeChangeProcessor extend DataTypeErrorHandler.

Now sync bridges have a way to report errors. Copy() is a hack for some
weird directory situations that should probably be removed in the
future, so I just put NOTREACHED() for the USS classes.

BUG=673883
==========

to

==========
[Sync] Introduce ModelError for USS error handling.

SyncError is confusing to use in model code, because it has features
(error type and model type) that are only needed for inner sync
machinery. This change introduces the simplified ModelError, which only
has a location and a message. If the need arises, it could also get an
error type/desired recovery action value as well, but I'm doubtful
that's necessary and I'm sure we can make something clearer than
SyncError's.

- SyncError replaced by ModelError everywhere below the processor.
- SMTP no longer implements SyncErrorFactory and now just has a
  ReportError method (with one convenience overload).
- SMTP now expects ReportError to be called instead of having error
  objects as callback parameters. This streamlines error handling quite
  a bit.
- Added error handling for Get(All)Data in AutocompleteSyncBridge.

I plan to convert everything between the processor and the controller to
use ModelError in a followup CL.

BUG=673883
==========

[maxbogue, 2017-01-07 00:10:54]

The CQ bit was checked by maxbogue@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-07 00:11:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[maxbogue, 2017-01-07 00:12:39]

I changed basically everything, PTAL! Bots haven't run though so they may
explode.

https://codereview.chromium.org/2618483003/diff/20001/components/sync/device_...
File components/sync/device_info/device_info_sync_bridge.cc (right):

https://codereview.chromium.org/2618483003/diff/20001/components/sync/device_...
components/sync/device_info/device_info_sync_bridge.cc:480:
change_processor()->CreateAndUploadError(FROM_HERE, msg, type()),
On 2017/01/05 17:03:49, skym wrote:
> What if, we instead abandon the old error interfaces and write new ones. We
> don't have to NOTREACHED in Copy(). We don't have to supply a type to
> CreateAndUploadError(). I don't really like the existence of
> CreateAndUploadError(). First, it shouldn't upload. that should be done in
> OnUnrecoverableError() or OnMetadataLoaded(). Except AndUpload is in the name.
> 
> But, I don't think CreateAndUploadError() is even needed if we were to remove
> SyncErrors from OnMetadataLoaded(). Can we instead have the bridge just call
> OnUnrecoverableError() and never get around to OnMetadataLoaded? Making sure
> OnMetadataLoaded() is always called is a hassle for the bridge, and can be
error
> prone when encountering errors.
> 
> Hmm, you don't even need to start with a new interface. Just create a one off
> method in the Proc.
> 
> void SharedModelTypeProcessor::ReportUnrecoverableError(const
> tracked_objects::Location& location, const std::string& message) {
>   ...
> }

Done!

https://codereview.chromium.org/2618483003/diff/20001/components/sync/model_i...
File components/sync/model_impl/shared_model_type_processor.cc (right):

https://codereview.chromium.org/2618483003/diff/20001/components/sync/model_i...
components/sync/model_impl/shared_model_type_processor.cc:61: ConnectIfReady();
On 2017/01/05 17:03:49, skym wrote:
> While you're in here, would be nice to explain why we connect if we're in an
> error state. I think this is at first intuitively backwards.

n/a, I think?

https://codereview.chromium.org/2618483003/diff/20001/components/sync/model_i...
components/sync/model_impl/shared_model_type_processor.cc:155: ModelType type) {
On 2017/01/05 17:03:49, skym wrote:
> Doesn't seem like type is very useful to be part of this interface. I guess
you
> could DCHECK that type == type_.

n/a

https://codereview.chromium.org/2618483003/diff/20001/components/sync/model_i...
components/sync/model_impl/shared_model_type_processor.cc:164: NOTREACHED();
On 2017/01/05 17:03:49, skym wrote:
> I think this probably deserves a comment. Could explain why we're in this
> situation, why Copy() doesn't make sense for this class.

n/a

[commit-bot: I haz the power, 2017-01-07 00:13:22]

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

[commit-bot: I haz the power, 2017-01-07 00:13:23]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[maxbogue, 2017-01-07 00:25:15]

The CQ bit was checked by maxbogue@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-07 00:25:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-07 00:27:56]

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

[commit-bot: I haz the power, 2017-01-07 00:27:56]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[maxbogue, 2017-01-07 01:57:18]

The CQ bit was checked by maxbogue@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-07 01:57:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-07 02:04:54]

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

[commit-bot: I haz the power, 2017-01-07 02:04:55]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[maxbogue, 2017-01-07 02:16:52]

The CQ bit was checked by maxbogue@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-07 02:17:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-07 02:23:25]

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

[commit-bot: I haz the power, 2017-01-07 02:23:26]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[maxbogue, 2017-01-07 02:55:24]

The CQ bit was checked by maxbogue@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-07 02:55:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-07 03:52:28]

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

[commit-bot: I haz the power, 2017-01-07 03:52:29]

Dry run: This issue passed the CQ dry run.

[skym, 2017-01-09 18:20:29]

lgtm

Love the changes! I'm really liking things. The only thing I really care about
is keeping comments in bridge/proc.h files up to date with the requirements.

If you decide to go with base::Optional in this CL, I'd like a chance to do
another review pass. Otherwise:

https://codereview.chromium.org/2618483003/diff/120001/chrome/browser/sync/te...
File chrome/browser/sync/test/integration/two_client_uss_sync_test.cc (right):

https://codereview.chromium.org/2618483003/diff/120001/chrome/browser/sync/te...
chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:72:
syncer::ModelError error = FakeModelTypeSyncBridge::ApplySyncChanges(
Seems like there should be an #include .../model_error.h in this file. Can you
audit these changes to make sure you didn't miss any others?

https://codereview.chromium.org/2618483003/diff/120001/components/autofill/co...
File components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc
(right):

https://codereview.chromium.org/2618483003/diff/120001/components/autofill/co...
components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:129: for
(const auto& key : storage_keys) {
Might as well only make this after we avoid the error case. Though if you do the
predicate suggestion below then you cannot do this.

https://codereview.chromium.org/2618483003/diff/120001/components/autofill/co...
components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:134: if
(!GetAutofillTable()->GetAllAutofillEntries(&entries)) {
I see you added the logic here but not the unit tests! :p

https://codereview.chromium.org/2618483003/diff/120001/components/autofill/co...
components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:154: if
(!GetAutofillTable()->GetAllAutofillEntries(&entries)) {
This is looking very repeated to above. You could create a predicate function
that filters out results for GetData(...) and no-ops for GetAllData(...). But if
you don't feel like it, I'm okay with that too.

https://codereview.chromium.org/2618483003/diff/120001/components/sync/model/...
File components/sync/model/fake_model_type_change_processor.cc (right):

https://codereview.chromium.org/2618483003/diff/120001/components/sync/model/...
components/sync/model/fake_model_type_change_processor.cc:25:
DCHECK(!error_.IsSet());
Can you add a comment here explaining why this is here? And what unittests have
done wrong if they hit this DCHECK?

https://codereview.chromium.org/2618483003/diff/120001/components/sync/model/...
components/sync/model/fake_model_type_change_processor.cc:55: error_ = error;
Is overriding a set error with an unset or set error a problem?

https://codereview.chromium.org/2618483003/diff/120001/components/sync/model/...
components/sync/model/fake_model_type_change_processor.cc:61:
ReportError(ModelError(location, message));
One one hand, pure virtual classes are nice. On the other hand, is there really
a use case for the implementation of this method to not do exactly this?

https://codereview.chromium.org/2618483003/diff/120001/components/sync/model/...
File components/sync/model/fake_model_type_sync_bridge.h (right):

https://codereview.chromium.org/2618483003/diff/120001/components/sync/model/...
components/sync/model/fake_model_type_sync_bridge.h:144: ModelError
bridge_error_;
This could just as easily be a boolean flag now, right?

https://codereview.chromium.org/2618483003/diff/120001/components/sync/model/...
File components/sync/model/model_error.h (right):

https://codereview.chromium.org/2618483003/diff/120001/components/sync/model/...
components/sync/model/model_error.h:29: // The location of the error this object
represents. Can only be called if the
Hmm, this is actually looking a lot like an optional.
https://cs.chromium.org/chromium/src/base/optional.h?q=optional&sq=package:ch...

WDYT?

https://codereview.chromium.org/2618483003/diff/120001/components/sync/model/...
File components/sync/model/model_type_change_processor.h (right):

https://codereview.chromium.org/2618483003/diff/120001/components/sync/model/...
components/sync/model/model_type_change_processor.h:54: virtual void
OnMetadataLoaded(std::unique_ptr<MetadataBatch> batch) = 0;
Would be nice to talk about about how the bridge needs to call this (unless they
call ReportError) in this file as well, to make it as clear to model type
implementers as possible.

https://codereview.chromium.org/2618483003/diff/120001/components/sync/model/...
components/sync/model/model_type_change_processor.h:78: virtual void
ReportError(const ModelError& error) = 0;
Same as above.

https://codereview.chromium.org/2618483003/diff/120001/components/sync/model/...
File components/sync/model/model_type_sync_bridge.h (right):

https://codereview.chromium.org/2618483003/diff/120001/components/sync/model/...
components/sync/model/model_type_sync_bridge.h:31: // with metadata through
ModelTypeChangeProcessor::OnMetadataLoaded() as soon as
This CL is changing this requirement, right? If you provide an error, you don't
have to call OnMetadataLoaded, right? So can you update this comment to make
that clear?

https://codereview.chromium.org/2618483003/diff/120001/components/sync/model/...
components/sync/model/model_type_sync_bridge.h:79: virtual void
GetData(StorageKeyList storage_keys, DataCallback callback) = 0;
Can you update this to make it clear that callback does not need to be invoked
when there's an error and you call ModelTypeChangeProcessor::ReportError(..)

https://codereview.chromium.org/2618483003/diff/120001/components/sync/model/...
components/sync/model/model_type_sync_bridge.h:82: virtual void
GetAllData(DataCallback callback) = 0;
Same.

Also, is it valid for the processor to have multiple outstanding
GetAllData/GetData at the same time? It will become ambiguous which one is being
errored out now. Do we require 1:1 errors or if the bridge has errored once, is
it allowed to ignore further requests?

https://codereview.chromium.org/2618483003/diff/120001/components/sync/model_...
File components/sync/model_impl/shared_model_type_processor.cc (right):

https://codereview.chromium.org/2618483003/diff/120001/components/sync/model_...
components/sync/model_impl/shared_model_type_processor.cc:647: SyncError
SharedModelTypeProcessor::ModelToSyncError(ModelError error) const {
Is there a reason you cannot take a const ModelError& error?

Also, if this method took ModelType as a param it could be in the anonymous
namespace instead of an instanced method.

https://codereview.chromium.org/2618483003/diff/120001/components/sync/model_...
File components/sync/model_impl/shared_model_type_processor_unittest.cc (right):

https://codereview.chromium.org/2618483003/diff/120001/components/sync/model_...
components/sync/model_impl/shared_model_type_processor_unittest.cc:920: //
Helper function to turn a ModelError into a SyncError.
Was this mistakenly pasted in here?

[maxbogue, 2017-01-09 21:27:57]

The CQ bit was checked by maxbogue@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-09 21:28:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[maxbogue, 2017-01-09 21:29:33]

PTAnL at all the comment tweaks!

https://codereview.chromium.org/2618483003/diff/120001/chrome/browser/sync/te...
File chrome/browser/sync/test/integration/two_client_uss_sync_test.cc (right):

https://codereview.chromium.org/2618483003/diff/120001/chrome/browser/sync/te...
chrome/browser/sync/test/integration/two_client_uss_sync_test.cc:72:
syncer::ModelError error = FakeModelTypeSyncBridge::ApplySyncChanges(
On 2017/01/09 18:20:27, skym wrote:
> Seems like there should be an #include .../model_error.h in this file. Can you
> audit these changes to make sure you didn't miss any others?

Done.

https://codereview.chromium.org/2618483003/diff/120001/components/autofill/co...
File components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc
(right):

https://codereview.chromium.org/2618483003/diff/120001/components/autofill/co...
components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:129: for
(const auto& key : storage_keys) {
On 2017/01/09 18:20:28, skym wrote:
> Might as well only make this after we avoid the error case. Though if you do
the
> predicate suggestion below then you cannot do this.

Done.

https://codereview.chromium.org/2618483003/diff/120001/components/autofill/co...
components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:134: if
(!GetAutofillTable()->GetAllAutofillEntries(&entries)) {
On 2017/01/09 18:20:27, skym wrote:
> I see you added the logic here but not the unit tests! :p

Taking you up on your offline offer to add them yourself :)

https://codereview.chromium.org/2618483003/diff/120001/components/autofill/co...
components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:154: if
(!GetAutofillTable()->GetAllAutofillEntries(&entries)) {
On 2017/01/09 18:20:27, skym wrote:
> This is looking very repeated to above. You could create a predicate function
> that filters out results for GetData(...) and no-ops for GetAllData(...). But
if
> you don't feel like it, I'm okay with that too.

Passing on this for now.

https://codereview.chromium.org/2618483003/diff/120001/components/sync/model/...
File components/sync/model/fake_model_type_change_processor.cc (right):

https://codereview.chromium.org/2618483003/diff/120001/components/sync/model/...
components/sync/model/fake_model_type_change_processor.cc:25:
DCHECK(!error_.IsSet());
On 2017/01/09 18:20:28, skym wrote:
> Can you add a comment here explaining why this is here? And what unittests
have
> done wrong if they hit this DCHECK?

Done.

https://codereview.chromium.org/2618483003/diff/120001/components/sync/model/...
components/sync/model/fake_model_type_change_processor.cc:55: error_ = error;
On 2017/01/09 18:20:28, skym wrote:
> Is overriding a set error with an unset or set error a problem?

Changing how this works to just have a boolean expect_error_ that gets checked
here, instead of storing the error itself.

https://codereview.chromium.org/2618483003/diff/120001/components/sync/model/...
components/sync/model/fake_model_type_change_processor.cc:61:
ReportError(ModelError(location, message));
On 2017/01/09 18:20:28, skym wrote:
> One one hand, pure virtual classes are nice. On the other hand, is there
really
> a use case for the implementation of this method to not do exactly this?

Not necessarily, but pure virtual classes are nice and we'll never have more
than these couple implementations of it so I'm leaving it this way.

https://codereview.chromium.org/2618483003/diff/120001/components/sync/model/...
File components/sync/model/fake_model_type_sync_bridge.h (right):

https://codereview.chromium.org/2618483003/diff/120001/components/sync/model/...
components/sync/model/fake_model_type_sync_bridge.h:144: ModelError
bridge_error_;
On 2017/01/09 18:20:28, skym wrote:
> This could just as easily be a boolean flag now, right?

Done.

https://codereview.chromium.org/2618483003/diff/120001/components/sync/model/...
File components/sync/model/model_error.h (right):

https://codereview.chromium.org/2618483003/diff/120001/components/sync/model/...
components/sync/model/model_error.h:29: // The location of the error this object
represents. Can only be called if the
On 2017/01/09 18:20:28, skym wrote:
> Hmm, this is actually looking a lot like an optional.
>
https://cs.chromium.org/chromium/src/base/optional.h?q=optional&sq=package:ch...
> 
> WDYT?

I like the idea, but I'd like to leave it for a follow-up CL to try to
implement.

https://codereview.chromium.org/2618483003/diff/120001/components/sync/model/...
File components/sync/model/model_type_change_processor.h (right):

https://codereview.chromium.org/2618483003/diff/120001/components/sync/model/...
components/sync/model/model_type_change_processor.h:54: virtual void
OnMetadataLoaded(std::unique_ptr<MetadataBatch> batch) = 0;
On 2017/01/09 18:20:28, skym wrote:
> Would be nice to talk about about how the bridge needs to call this (unless
they
> call ReportError) in this file as well, to make it as clear to model type
> implementers as possible.

Done.

https://codereview.chromium.org/2618483003/diff/120001/components/sync/model/...
components/sync/model/model_type_change_processor.h:78: virtual void
ReportError(const ModelError& error) = 0;
On 2017/01/09 18:20:28, skym wrote:
> Same as above.

Done.

https://codereview.chromium.org/2618483003/diff/120001/components/sync/model/...
File components/sync/model/model_type_sync_bridge.h (right):

https://codereview.chromium.org/2618483003/diff/120001/components/sync/model/...
components/sync/model/model_type_sync_bridge.h:31: // with metadata through
ModelTypeChangeProcessor::OnMetadataLoaded() as soon as
On 2017/01/09 18:20:28, skym wrote:
> This CL is changing this requirement, right? If you provide an error, you
don't
> have to call OnMetadataLoaded, right? So can you update this comment to make
> that clear?

Done.

https://codereview.chromium.org/2618483003/diff/120001/components/sync/model/...
components/sync/model/model_type_sync_bridge.h:79: virtual void
GetData(StorageKeyList storage_keys, DataCallback callback) = 0;
On 2017/01/09 18:20:28, skym wrote:
> Can you update this to make it clear that callback does not need to be invoked
> when there's an error and you call ModelTypeChangeProcessor::ReportError(..)

Done.

https://codereview.chromium.org/2618483003/diff/120001/components/sync/model/...
components/sync/model/model_type_sync_bridge.h:82: virtual void
GetAllData(DataCallback callback) = 0;
On 2017/01/09 18:20:28, skym wrote:
> Same.
> 
> Also, is it valid for the processor to have multiple outstanding
> GetAllData/GetData at the same time? It will become ambiguous which one is
being
> errored out now. Do we require 1:1 errors or if the bridge has errored once,
is
> it allowed to ignore further requests?

Just by the nature of things, I think we'll get 1:1 ReportError calls for
outstanding callbacks if everything goes to hell. That's fine; the
ModelTypeController's the only thing that really has to deal with that.

I'm not 100% sure how well it /does/ handle that right now, but this isn't
making anything worse. I think it may just log some histograms we might not want
it to.

https://codereview.chromium.org/2618483003/diff/120001/components/sync/model_...
File components/sync/model_impl/shared_model_type_processor.cc (right):

https://codereview.chromium.org/2618483003/diff/120001/components/sync/model_...
components/sync/model_impl/shared_model_type_processor.cc:647: SyncError
SharedModelTypeProcessor::ModelToSyncError(ModelError error) const {
On 2017/01/09 18:20:28, skym wrote:
> Is there a reason you cannot take a const ModelError& error?
> 
> Also, if this method took ModelType as a param it could be in the anonymous
> namespace instead of an instanced method.

Made it const&. It'll be going away in a follow-up anyways so not gonna make it
anon.

https://codereview.chromium.org/2618483003/diff/120001/components/sync/model_...
File components/sync/model_impl/shared_model_type_processor_unittest.cc (right):

https://codereview.chromium.org/2618483003/diff/120001/components/sync/model_...
components/sync/model_impl/shared_model_type_processor_unittest.cc:920: //
Helper function to turn a ModelError into a SyncError.
On 2017/01/09 18:20:28, skym wrote:
> Was this mistakenly pasted in here?

Sure looks like it O_O

[maxbogue, 2017-01-09 21:32:57]

maxbogue@chromium.org changed reviewers:
+ mathp@chromium.org, olivierrobin@chromium.org

[maxbogue, 2017-01-09 21:32:58]

+olivierrobin for components/reading_list/
+mathp for components/autofill/

[skym, 2017-01-09 21:45:50]

Still lgtm

[Mathieu, 2017-01-09 22:13:00]

autofill lgtm

[Olivier, 2017-01-09 22:14:24]

reading_list LGTM

[commit-bot: I haz the power, 2017-01-09 22:49:30]

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

[commit-bot: I haz the power, 2017-01-09 22:49:31]

Dry run: This issue passed the CQ dry run.

[maxbogue, 2017-01-09 23:01:00]

The CQ bit was checked by maxbogue@chromium.org

[commit-bot: I haz the power, 2017-01-09 23:03:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-09 23:35:26]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484002860532160,
"parent_rev": "0fe8b9755b0de413d00ee8a9368d9c4f65821737", "commit_rev":
"04ada5a89fbd87369e96d434aa39aee90d20809c"}

[commit-bot: I haz the power, 2017-01-09 23:35:57]

Description was changed from

==========
[Sync] Introduce ModelError for USS error handling.

SyncError is confusing to use in model code, because it has features
(error type and model type) that are only needed for inner sync
machinery. This change introduces the simplified ModelError, which only
has a location and a message. If the need arises, it could also get an
error type/desired recovery action value as well, but I'm doubtful
that's necessary and I'm sure we can make something clearer than
SyncError's.

- SyncError replaced by ModelError everywhere below the processor.
- SMTP no longer implements SyncErrorFactory and now just has a
  ReportError method (with one convenience overload).
- SMTP now expects ReportError to be called instead of having error
  objects as callback parameters. This streamlines error handling quite
  a bit.
- Added error handling for Get(All)Data in AutocompleteSyncBridge.

I plan to convert everything between the processor and the controller to
use ModelError in a followup CL.

BUG=673883
==========

to

==========
[Sync] Introduce ModelError for USS error handling.

SyncError is confusing to use in model code, because it has features
(error type and model type) that are only needed for inner sync
machinery. This change introduces the simplified ModelError, which only
has a location and a message. If the need arises, it could also get an
error type/desired recovery action value as well, but I'm doubtful
that's necessary and I'm sure we can make something clearer than
SyncError's.

- SyncError replaced by ModelError everywhere below the processor.
- SMTP no longer implements SyncErrorFactory and now just has a
  ReportError method (with one convenience overload).
- SMTP now expects ReportError to be called instead of having error
  objects as callback parameters. This streamlines error handling quite
  a bit.
- Added error handling for Get(All)Data in AutocompleteSyncBridge.

I plan to convert everything between the processor and the controller to
use ModelError in a followup CL.

BUG=673883

Review-Url: https://codereview.chromium.org/2618483003
Cr-Commit-Position: refs/heads/master@{#442401}
Committed:
https://chromium.googlesource.com/chromium/src/+/04ada5a89fbd87369e96d434aa39...
==========

[commit-bot: I haz the power, 2017-01-09 23:35:58]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/04ada5a89fbd87369e96d434aa39...