sync: Add location info to unrecoverable error.

sync: Add location info to unrecoverable errors, and remove the UnrecoverableErrorHandler from model associators since it's illegal to use from there anyway.

BUG=42695
TEST=data type controller unittests


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

[skrul, 2010-06-07 17:57:25]

LGTM
Very awesome!  I'm so glad you did this.

http://codereview.chromium.org/2002012/diff/56052/41007
File chrome/browser/sync/glue/autofill_change_processor.cc (right):

http://codereview.chromium.org/2002012/diff/56052/41007#newcode103
chrome/browser/sync/glue/autofill_change_processor.cc:103: std::string err =
"Failed to overwrite label for node";
nit: add a space as the last character of this string

http://codereview.chromium.org/2002012/diff/56052/41015
File chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc (right):

http://codereview.chromium.org/2002012/diff/56052/41015#newcode31
chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc:31: using
testing::Invoke;
I think this is no longer used, plz remove.

http://codereview.chromium.org/2002012/diff/56052/41016
File chrome/browser/sync/glue/bookmark_model_associator.cc (right):

http://codereview.chromium.org/2002012/diff/56052/41016#newcode456
chrome/browser/sync/glue/bookmark_model_associator.cc:456: LOG(ERROR) << "Could
not lookup bookmark node.";
So this is the one place where I thought we needed to have an
OnUnrecoverableErrorHandler in a ModelAssociator... it is OK to make this
change?  Of course, IMHO we could combine all the ModelAssociators and
ChangeProcessors into one class anyway since they are already practically
married, why not move in together?

http://codereview.chromium.org/2002012/diff/56052/41018
File chrome/browser/sync/glue/data_type_controller_mock.h (right):

http://codereview.chromium.org/2002012/diff/56052/41018#newcode23
chrome/browser/sync/glue/data_type_controller_mock.h:23: const std::string&
message));
the style in this particular mock seems to not include parameter names.  Can you
remove 'message'?

http://codereview.chromium.org/2002012/diff/56052/41029
File chrome/browser/sync/glue/preference_data_type_controller_unittest.cc
(right):

http://codereview.chromium.org/2002012/diff/56052/41029#newcode25
chrome/browser/sync/glue/preference_data_type_controller_unittest.cc:25: using
testing::Invoke;
remove if no longer used.

http://codereview.chromium.org/2002012/diff/56052/41035
File chrome/browser/sync/glue/theme_data_type_controller_unittest.cc (right):

http://codereview.chromium.org/2002012/diff/56052/41035#newcode25
chrome/browser/sync/glue/theme_data_type_controller_unittest.cc:25: using
testing::Invoke;
remove if not needed.

http://codereview.chromium.org/2002012/diff/56052/41046
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/2002012/diff/56052/41046#newcode424
chrome/browser/sync/profile_sync_service.cc:424: LOG(ERROR) << location;
It would be great if this message could be exposed on about:sync and also
included in the google feedback stuff.

http://codereview.chromium.org/2002012/diff/56052/41047
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/2002012/diff/56052/41047#newcode211
chrome/browser/sync/profile_sync_service.h:211: tracked_objects::Location
unrecoverable_error_location() {
can this return a const ref?

http://codereview.chromium.org/2002012/diff/56052/41049
File chrome/browser/sync/profile_sync_service_mock.h (right):

http://codereview.chromium.org/2002012/diff/56052/41049#newcode34
chrome/browser/sync/profile_sync_service_mock.h:34:
MOCK_METHOD2(OnUnrecoverableError, void(const tracked_objects::Location&,
add parameter name to match existing style.

[tim (not reviewing), 2010-06-08 01:29:17]

http://codereview.chromium.org/2002012/diff/56052/41007
File chrome/browser/sync/glue/autofill_change_processor.cc (right):

http://codereview.chromium.org/2002012/diff/56052/41007#newcode103
chrome/browser/sync/glue/autofill_change_processor.cc:103: std::string err =
"Failed to overwrite label for node";
On 2010/06/07 17:57:26, skrul wrote:
> nit: add a space as the last character of this string

Done.

http://codereview.chromium.org/2002012/diff/56052/41015
File chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc (right):

http://codereview.chromium.org/2002012/diff/56052/41015#newcode31
chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc:31: using
testing::Invoke;
On 2010/06/07 17:57:26, skrul wrote:
> I think this is no longer used, plz remove.

Done.

http://codereview.chromium.org/2002012/diff/56052/41016
File chrome/browser/sync/glue/bookmark_model_associator.cc (right):

http://codereview.chromium.org/2002012/diff/56052/41016#newcode456
chrome/browser/sync/glue/bookmark_model_associator.cc:456: LOG(ERROR) << "Could
not lookup bookmark node.";
On 2010/06/07 17:57:26, skrul wrote:
> So this is the one place where I thought we needed to have an
> OnUnrecoverableErrorHandler in a ModelAssociator... it is OK to make this
> change?  Of course, IMHO we could combine all the ModelAssociators and
> ChangeProcessors into one class anyway since they are already practically
> married, why not move in together?
> 

Hmm, I'm still looking at this... maybe we do need it here.  I'm not that
excited at the idea of combining the two files, a) because there's a lot of code
to read / compile / wrap your head around in each and b) if we actually don't
want to use the error handler from the model associator it's a lot more
straightforward to isolate the code into a class that doesn't have access to
one, no?

http://codereview.chromium.org/2002012/diff/56052/41018
File chrome/browser/sync/glue/data_type_controller_mock.h (right):

http://codereview.chromium.org/2002012/diff/56052/41018#newcode23
chrome/browser/sync/glue/data_type_controller_mock.h:23: const std::string&
message));
On 2010/06/07 17:57:26, skrul wrote:
> the style in this particular mock seems to not include parameter names.  Can
you
> remove 'message'?

Done.

http://codereview.chromium.org/2002012/diff/56052/41029
File chrome/browser/sync/glue/preference_data_type_controller_unittest.cc
(right):

http://codereview.chromium.org/2002012/diff/56052/41029#newcode25
chrome/browser/sync/glue/preference_data_type_controller_unittest.cc:25: using
testing::Invoke;
On 2010/06/07 17:57:26, skrul wrote:
> remove if no longer used.

Done.

http://codereview.chromium.org/2002012/diff/56052/41035
File chrome/browser/sync/glue/theme_data_type_controller_unittest.cc (right):

http://codereview.chromium.org/2002012/diff/56052/41035#newcode25
chrome/browser/sync/glue/theme_data_type_controller_unittest.cc:25: using
testing::Invoke;
On 2010/06/07 17:57:26, skrul wrote:
> remove if not needed.

Done.

http://codereview.chromium.org/2002012/diff/56052/41046
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/2002012/diff/56052/41046#newcode424
chrome/browser/sync/profile_sync_service.cc:424: LOG(ERROR) << location;
On 2010/06/07 17:57:26, skrul wrote:
> It would be great if this message could be exposed on about:sync and also
> included in the google feedback stuff.

It will be on about:sync (http://codereview.chromium.org/1987009).  Will follow
up w/ you re: feedback stuff.

http://codereview.chromium.org/2002012/diff/56052/41047
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/2002012/diff/56052/41047#newcode211
chrome/browser/sync/profile_sync_service.h:211: tracked_objects::Location
unrecoverable_error_location() {
On 2010/06/07 17:57:26, skrul wrote:
> can this return a const ref?

The ternary expression below can return a local variable, so not as written.

http://codereview.chromium.org/2002012/diff/56052/41049
File chrome/browser/sync/profile_sync_service_mock.h (right):

http://codereview.chromium.org/2002012/diff/56052/41049#newcode34
chrome/browser/sync/profile_sync_service_mock.h:34:
MOCK_METHOD2(OnUnrecoverableError, void(const tracked_objects::Location&,
On 2010/06/07 17:57:26, skrul wrote:
> add parameter name to match existing style.

Done.

[tim (not reviewing), 2010-06-08 23:02:46]

On 2010/06/08 23:02:29, timsteele wrote:
> 
new snapshot up

[skrul, 2010-06-08 23:22:42]

LGTM yay factories.

And I like the template removal, but the pref merge change I'm trying to get in
removes the usage of the TestModelAssociatior stuff from all the unit tests
except for bookmarks.  I've been trying to get rid of all the "subclass to make
testable" instances and this was on my hit list :)  Feel free to submit as is
though, I'll deal with the merge.