[Sync] Refactor sync datatype error handling.

[Sync] Refactor sync datatype error handling.

This introduces SyncError's, which are a convenient way of passing around an
error location, type, and message. All datatypes have been refactored to use
this, including in the AssociateModels code. A future change will use this
to add support for continuing sync even when a datatype fails to start.
In addition, eventually a future change will convert the UnrecoverableError
handler to use SyncError's as well as have the datatype controller's and
datatype manager surface SyncError's to the PSS.

BUG=87645
TEST=

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

[Nicolas Zea, 2011-07-20 01:19:47]

It's not as bad as it looks, I swear! Nearly all of these files have couple line
changes.

The bulk of the logic is in the sync_error* files and the datatype
manager/controller files. As mentioned in the commit message, eventually I
want to surface the SyncError information all the way to the PSS, as well as
support disabling failing datatypes and continuing syncing, but those will be
in separate patches.

[akalin, 2011-07-20 23:43:05]

http://codereview.chromium.org/7453014/diff/8001/chrome/browser/sync/api/sync...
File chrome/browser/sync/api/sync_error.cc (right):

http://codereview.chromium.org/7453014/diff/8001/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_error.cc:25: print_log_error();
Hmm so an error message will be logged every time the SyncError is copied
around?  That seems like it'll generate a lot, esp. if you pass between threads.

http://codereview.chromium.org/7453014/diff/8001/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_error.cc:38: return (bool)location_.get();
i think this will still trigger a warning on Win.  Try:

return location_.get() != NULL

http://codereview.chromium.org/7453014/diff/8001/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_error.cc:42: return *location_;
DCHECK(is_set()) here, too?  Probably should be a CHECK() anyway

http://codereview.chromium.org/7453014/diff/8001/chrome/browser/sync/api/sync...
File chrome/browser/sync/api/sync_error.h (right):

http://codereview.chromium.org/7453014/diff/8001/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_error.h:36: void reset(const
tracked_objects::Location& location,
reset -> Reset

http://codereview.chromium.org/7453014/diff/8001/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_error.h:41: bool is_set() const;
is_set -> IsSet()

Consider IsInitialized()

http://codereview.chromium.org/7453014/diff/8001/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_error.h:50: scoped_ptr<tracked_objects::Location>
location_;
Comment that you use a scoped_ptr because Location isn't assignable

[akalin, 2011-07-20 23:43:59]

http://codereview.chromium.org/7453014/diff/8001/chrome/browser/sync/syncable...
File chrome/browser/sync/syncable/model_type.cc (right):

http://codereview.chromium.org/7453014/diff/8001/chrome/browser/sync/syncable...
chrome/browser/sync/syncable/model_type.cc:250: if (*iter >=
FIRST_REAL_MODEL_TYPE && *iter < MODEL_TYPE_COUNT)
Hmm.  What is the reason for this?

[Nicolas Zea, 2011-07-21 20:09:50]

Comments addressed, PTAL

http://codereview.chromium.org/7453014/diff/8001/chrome/browser/sync/api/sync...
File chrome/browser/sync/api/sync_error.cc (right):

http://codereview.chromium.org/7453014/diff/8001/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_error.cc:25: print_log_error();
On 2011/07/20 23:43:05, akalin wrote:
> Hmm so an error message will be logged every time the SyncError is copied
> around?  That seems like it'll generate a lot, esp. if you pass between
threads.

Done.

http://codereview.chromium.org/7453014/diff/8001/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_error.cc:38: return (bool)location_.get();
On 2011/07/20 23:43:05, akalin wrote:
> i think this will still trigger a warning on Win.  Try:
> 
> return location_.get() != NULL

Done.

http://codereview.chromium.org/7453014/diff/8001/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_error.cc:42: return *location_;
On 2011/07/20 23:43:05, akalin wrote:
> DCHECK(is_set()) here, too?  Probably should be a CHECK() anyway

Done.

http://codereview.chromium.org/7453014/diff/8001/chrome/browser/sync/api/sync...
File chrome/browser/sync/api/sync_error.h (right):

http://codereview.chromium.org/7453014/diff/8001/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_error.h:36: void reset(const
tracked_objects::Location& location,
On 2011/07/20 23:43:05, akalin wrote:
> reset -> Reset

Done.

http://codereview.chromium.org/7453014/diff/8001/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_error.h:41: bool is_set() const;
On 2011/07/20 23:43:05, akalin wrote:
> is_set -> IsSet()
> 
> Consider IsInitialized()

Done.

http://codereview.chromium.org/7453014/diff/8001/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_error.h:50: scoped_ptr<tracked_objects::Location>
location_;
On 2011/07/20 23:43:05, akalin wrote:
> Comment that you use a scoped_ptr because Location isn't assignable

Done.

http://codereview.chromium.org/7453014/diff/8001/chrome/browser/sync/syncable...
File chrome/browser/sync/syncable/model_type.cc (right):

http://codereview.chromium.org/7453014/diff/8001/chrome/browser/sync/syncable...
chrome/browser/sync/syncable/model_type.cc:250: if (*iter >=
FIRST_REAL_MODEL_TYPE && *iter < MODEL_TYPE_COUNT)
On 2011/07/20 23:43:59, akalin wrote:
> Hmm.  What is the reason for this?

Yeah, thinking about it more this isn't the right approach. Done.

[akalin, 2011-07-21 23:03:53]

Few more

http://codereview.chromium.org/7453014/diff/16001/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/sync_error.cc (right):

http://codereview.chromium.org/7453014/diff/16001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_error.cc:46: DCHECK(IsInitialized());
should probably be a CHECK, too, for consistency

http://codereview.chromium.org/7453014/diff/16001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_error.cc:51: DCHECK(IsInitialized());
here too

http://codereview.chromium.org/7453014/diff/16001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_error.cc:58: ERROR).stream(),
how does this work?  I think you need to do logging::LOG_ERROR instead of ERROR.
 Maybe ERROR is defined in some system header.  Ugh.

http://codereview.chromium.org/7453014/diff/16001/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/sync_error.h (right):

http://codereview.chromium.org/7453014/diff/16001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_error.h:11: #include "base/tracked.h"
can just forward-declare Location

http://codereview.chromium.org/7453014/diff/16001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_error.h:33: 
Just realized you need to implement the assignment operator, too.

http://codereview.chromium.org/7453014/diff/16001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_error.h:35: // May be called irrespective of
whether is_set() is true.
is_set -> IsInitialized()

http://codereview.chromium.org/7453014/diff/16001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_error.h:47: private:
add line before private:

http://codereview.chromium.org/7453014/diff/16001/chrome/browser/sync/syncabl...
File chrome/browser/sync/syncable/model_type.h (right):

http://codereview.chromium.org/7453014/diff/16001/chrome/browser/sync/syncabl...
chrome/browser/sync/syncable/model_type.h:166: bool IsDataTypeValid(ModelType
model_type);
I don't know if "valid" is the right adjective.  Maybe "IsRealDataType"?

http://codereview.chromium.org/7453014/diff/12003/chrome/browser/prefs/pref_m...
File chrome/browser/prefs/pref_model_associator.h (right):

http://codereview.chromium.org/7453014/diff/12003/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_model_associator.h:19: #include
"chrome/browser/sync/api/sync_error.h"
forward declare instead?  Or just not #include, since it's part of the inherited
interface

http://codereview.chromium.org/7453014/diff/12003/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/sync_change_processor.h (right):

http://codereview.chromium.org/7453014/diff/12003/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_change_processor.h:24: // Returns: False if an
error occured, true otherwise.
False -> false

[Nicolas Zea, 2011-07-21 23:43:26]

PTAL

http://codereview.chromium.org/7453014/diff/16001/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/sync_error.cc (right):

http://codereview.chromium.org/7453014/diff/16001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_error.cc:46: DCHECK(IsInitialized());
On 2011/07/21 23:03:53, akalin wrote:
> should probably be a CHECK, too, for consistency

Done.

http://codereview.chromium.org/7453014/diff/16001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_error.cc:51: DCHECK(IsInitialized());
On 2011/07/21 23:03:53, akalin wrote:
> here too

Done.

http://codereview.chromium.org/7453014/diff/16001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_error.cc:58: ERROR).stream(),
On 2011/07/21 23:03:53, akalin wrote:
> how does this work?  I think you need to do logging::LOG_ERROR instead of
ERROR.
>  Maybe ERROR is defined in some system header.  Ugh.

ERROR is a macro from logging.h that turns into 0. And apparently
logging::LOG_INFO is an enum that turns into 0. So I was sending the wrong
severity. Sneaky.

http://codereview.chromium.org/7453014/diff/16001/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/sync_error.h (right):

http://codereview.chromium.org/7453014/diff/16001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_error.h:11: #include "base/tracked.h"
On 2011/07/21 23:03:53, akalin wrote:
> can just forward-declare Location

Done.

http://codereview.chromium.org/7453014/diff/16001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_error.h:33: 
On 2011/07/21 23:03:53, akalin wrote:
> Just realized you need to implement the assignment operator, too.

I suppose it doesn't hurt to, but is it really needed? Done anyways.

http://codereview.chromium.org/7453014/diff/16001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_error.h:35: // May be called irrespective of
whether is_set() is true.
On 2011/07/21 23:03:53, akalin wrote:
> is_set -> IsInitialized()

Done.

http://codereview.chromium.org/7453014/diff/16001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_error.h:47: private:
On 2011/07/21 23:03:53, akalin wrote:
> add line before private:

Done.

http://codereview.chromium.org/7453014/diff/16001/chrome/browser/sync/syncabl...
File chrome/browser/sync/syncable/model_type.h (right):

http://codereview.chromium.org/7453014/diff/16001/chrome/browser/sync/syncabl...
chrome/browser/sync/syncable/model_type.h:166: bool IsDataTypeValid(ModelType
model_type);
On 2011/07/21 23:03:53, akalin wrote:
> I don't know if "valid" is the right adjective.  Maybe "IsRealDataType"?

Done.

http://codereview.chromium.org/7453014/diff/12003/chrome/browser/prefs/pref_m...
File chrome/browser/prefs/pref_model_associator.h (right):

http://codereview.chromium.org/7453014/diff/12003/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_model_associator.h:19: #include
"chrome/browser/sync/api/sync_error.h"
On 2011/07/21 23:03:53, akalin wrote:
> forward declare instead?  Or just not #include, since it's part of the
inherited
> interface

Done.

http://codereview.chromium.org/7453014/diff/12003/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/sync_change_processor.h (right):

http://codereview.chromium.org/7453014/diff/12003/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_change_processor.h:24: // Returns: False if an
error occured, true otherwise.
On 2011/07/21 23:03:53, akalin wrote:
> False -> false

Done.

[akalin, 2011-07-22 00:10:04]

http://codereview.chromium.org/7453014/diff/16001/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/sync_error.h (right):

http://codereview.chromium.org/7453014/diff/16001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_error.h:33: 
On 2011/07/21 23:43:26, nzea wrote:
> On 2011/07/21 23:03:53, akalin wrote:
> > Just realized you need to implement the assignment operator, too.
> 
> I suppose it doesn't hurt to, but is it really needed? Done anyways.

Yeah, because there is a (scoped) pointer variable, so the default assignment
operator would result in a double-free.

http://codereview.chromium.org/7453014/diff/14004/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/sync_error.cc (right):

http://codereview.chromium.org/7453014/diff/14004/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_error.cc:16: : location_(new
tracked_objects::Location(location)),
I think we can omit the initializer list and just have:

Reset(location, message, type);

here

http://codereview.chromium.org/7453014/diff/14004/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_error.cc:23: *this = other;
Create a private member fn Copy(const SyncError&) which calls Init()
appropriately and call that here

http://codereview.chromium.org/7453014/diff/14004/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_error.cc:27: if (this == &other) {
Self-assignment should be legal for operator=.  Just define Copy() as above and
do:

Copy(other);
return *this;

http://codereview.chromium.org/7453014/diff/14004/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_error.cc:40: location_.reset(new
tracked_objects::Location(location));
decomp everything but the PrintLogError() into private member fn Init(...) and
call that.

http://codereview.chromium.org/7453014/diff/14004/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/sync_error.h (right):

http://codereview.chromium.org/7453014/diff/14004/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_error.h:16: }
}  // namespace tracked_objects

http://codereview.chromium.org/7453014/diff/14004/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_error.h:25: // Default constructor refers to "no
error", and IsInitialized() will return
I wonder if it would be better to have all the functions that return bool and
fill in a SyncError* on false, instead return a SyncError, and change
IsInitialized() to HasError(), and have the caller check that instead.  Might be
less clumsy.  What do you think?

http://codereview.chromium.org/7453014/diff/14004/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_error.h:40: // May be called irrespective of
whether is_set() is true.
is_set -> IsInitialized()

http://codereview.chromium.org/7453014/diff/14004/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_error.h:57: // Scoped_ptr is necessary because
Location objects aren't assignable.
Scoped -> scoped

http://codereview.chromium.org/7453014/diff/14004/chrome/browser/sync/syncabl...
File chrome/browser/sync/syncable/model_type.h (right):

http://codereview.chromium.org/7453014/diff/14004/chrome/browser/sync/syncabl...
chrome/browser/sync/syncable/model_type.h:165: // Returns true if |model_type|
is a valid datatype
Change comment to "...is a real data type."

[Nicolas Zea, 2011-07-25 20:57:27]

Comments addressed. As discussed, I've also switched back to IsSet(), instead of
HasError.

http://codereview.chromium.org/7453014/diff/14004/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/sync_error.cc (right):

http://codereview.chromium.org/7453014/diff/14004/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_error.cc:16: : location_(new
tracked_objects::Location(location)),
On 2011/07/22 00:10:05, akalin wrote:
> I think we can omit the initializer list and just have:
> 
> Reset(location, message, type);
> 
> here

Done.

http://codereview.chromium.org/7453014/diff/14004/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_error.cc:23: *this = other;
On 2011/07/22 00:10:05, akalin wrote:
> Create a private member fn Copy(const SyncError&) which calls Init()
> appropriately and call that here

Done.

http://codereview.chromium.org/7453014/diff/14004/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_error.cc:27: if (this == &other) {
On 2011/07/22 00:10:05, akalin wrote:
> Self-assignment should be legal for operator=.  Just define Copy() as above
and
> do:
> 
> Copy(other);
> return *this;

Done.

http://codereview.chromium.org/7453014/diff/14004/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_error.cc:40: location_.reset(new
tracked_objects::Location(location));
On 2011/07/22 00:10:05, akalin wrote:
> decomp everything but the PrintLogError() into private member fn Init(...) and
> call that.

Done.

http://codereview.chromium.org/7453014/diff/14004/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/sync_error.h (right):

http://codereview.chromium.org/7453014/diff/14004/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_error.h:16: }
On 2011/07/22 00:10:05, akalin wrote:
> }  // namespace tracked_objects

Done.

http://codereview.chromium.org/7453014/diff/14004/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_error.h:25: // Default constructor refers to "no
error", and IsInitialized() will return
On 2011/07/22 00:10:05, akalin wrote:
> I wonder if it would be better to have all the functions that return bool and
> fill in a SyncError* on false, instead return a SyncError, and change
> IsInitialized() to HasError(), and have the caller check that instead.  Might
be
> less clumsy.  What do you think?

Yeah, I  like that better. Done.

http://codereview.chromium.org/7453014/diff/14004/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_error.h:40: // May be called irrespective of
whether is_set() is true.
On 2011/07/22 00:10:05, akalin wrote:
> is_set -> IsInitialized()

Done.

http://codereview.chromium.org/7453014/diff/14004/chrome/browser/sync/api/syn...
chrome/browser/sync/api/sync_error.h:57: // Scoped_ptr is necessary because
Location objects aren't assignable.
On 2011/07/22 00:10:05, akalin wrote:
> Scoped -> scoped

Done.

http://codereview.chromium.org/7453014/diff/14004/chrome/browser/sync/syncabl...
File chrome/browser/sync/syncable/model_type.h (right):

http://codereview.chromium.org/7453014/diff/14004/chrome/browser/sync/syncabl...
chrome/browser/sync/syncable/model_type.h:165: // Returns true if |model_type|
is a valid datatype
On 2011/07/22 00:10:05, akalin wrote:
> Change comment to "...is a real data type."

Done.

[akalin, 2011-07-25 22:43:13]

More comments

http://codereview.chromium.org/7453014/diff/5063/chrome/browser/prefs/pref_mo...
File chrome/browser/prefs/pref_model_associator.cc (right):

http://codereview.chromium.org/7453014/diff/5063/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.cc:165: } else {
i think it's cleaner to leave off the else (since the if claus returns)

http://codereview.chromium.org/7453014/diff/5063/chrome/browser/sync/api/sync...
File chrome/browser/sync/api/sync_change_processor.h (right):

http://codereview.chromium.org/7453014/diff/5063/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_change_processor.h:25: // Returns: false if an
error occured, true otherwise.
fix comment

http://codereview.chromium.org/7453014/diff/5063/chrome/browser/sync/api/sync...
File chrome/browser/sync/api/sync_error.cc (right):

http://codereview.chromium.org/7453014/diff/5063/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_error.cc:10: SyncError::SyncError() {
It would be good to initialize all variables, especially type_.  Suggest making
a private (or maybe public?) member function Clear() and calling that here.

http://codereview.chromium.org/7453014/diff/5063/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_error.cc:25: if (this == &other) {
I don't think we should bother explicitly checking for self-assignment

http://codereview.chromium.org/7453014/diff/5063/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_error.cc:38: location_.reset();
Move this code to Clear() and call that here.

http://codereview.chromium.org/7453014/diff/5063/chrome/browser/sync/glue/mod...
File chrome/browser/sync/glue/model_associator.h (right):

http://codereview.chromium.org/7453014/diff/5063/chrome/browser/sync/glue/mod...
chrome/browser/sync/glue/model_associator.h:32: virtual bool
AssociateModels(SyncError* error) = 0;
Change these to return SyncErrors too?  (and subclasses)

[Nicolas Zea, 2011-07-25 23:08:19]

PTAL

http://codereview.chromium.org/7453014/diff/5063/chrome/browser/prefs/pref_mo...
File chrome/browser/prefs/pref_model_associator.cc (right):

http://codereview.chromium.org/7453014/diff/5063/chrome/browser/prefs/pref_mo...
chrome/browser/prefs/pref_model_associator.cc:165: } else {
On 2011/07/25 22:43:13, akalin wrote:
> i think it's cleaner to leave off the else (since the if claus returns)

Done.

http://codereview.chromium.org/7453014/diff/5063/chrome/browser/sync/api/sync...
File chrome/browser/sync/api/sync_change_processor.h (right):

http://codereview.chromium.org/7453014/diff/5063/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_change_processor.h:25: // Returns: false if an
error occured, true otherwise.
On 2011/07/25 22:43:13, akalin wrote:
> fix comment

Done.

http://codereview.chromium.org/7453014/diff/5063/chrome/browser/sync/api/sync...
File chrome/browser/sync/api/sync_error.cc (right):

http://codereview.chromium.org/7453014/diff/5063/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_error.cc:10: SyncError::SyncError() {
On 2011/07/25 22:43:13, akalin wrote:
> It would be good to initialize all variables, especially type_.  Suggest
making
> a private (or maybe public?) member function Clear() and calling that here.

Done.

http://codereview.chromium.org/7453014/diff/5063/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_error.cc:25: if (this == &other) {
On 2011/07/25 22:43:13, akalin wrote:
> I don't think we should bother explicitly checking for self-assignment

There's no point in copying if we're not changing anything, so why not shortcut
it and save the memory allocations? Otherwise we're doing location_.reset(new
Location(*location_.get())).

http://codereview.chromium.org/7453014/diff/5063/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_error.cc:38: location_.reset();
On 2011/07/25 22:43:13, akalin wrote:
> Move this code to Clear() and call that here.

Done.

http://codereview.chromium.org/7453014/diff/5063/chrome/browser/sync/glue/mod...
File chrome/browser/sync/glue/model_associator.h (right):

http://codereview.chromium.org/7453014/diff/5063/chrome/browser/sync/glue/mod...
chrome/browser/sync/glue/model_associator.h:32: virtual bool
AssociateModels(SyncError* error) = 0;
On 2011/07/25 22:43:13, akalin wrote:
> Change these to return SyncErrors too?  (and subclasses)

Since this is already an established (and now deprecated API) that relies on
[Dis]AssociateModels returning true/false on success/error, and because we're
planning on switching everything over to the new API anyways, I'd prefer to do
the less intrusive change for now (just adding an extra parameter), if that's
ok. I've added a TODO for once we have less datatypes reliant on this approach.

[akalin, 2011-07-25 23:49:32]

LGTM

http://codereview.chromium.org/7453014/diff/5063/chrome/browser/sync/api/sync...
File chrome/browser/sync/api/sync_error.cc (right):

http://codereview.chromium.org/7453014/diff/5063/chrome/browser/sync/api/sync...
chrome/browser/sync/api/sync_error.cc:25: if (this == &other) {
On 2011/07/25 23:08:19, nzea wrote:
> On 2011/07/25 22:43:13, akalin wrote:
> > I don't think we should bother explicitly checking for self-assignment
> 
> There's no point in copying if we're not changing anything, so why not
shortcut
> it and save the memory allocations? Otherwise we're doing location_.reset(new
> Location(*location_.get())).

Yeah, but it's not a very common case, so I don't think it's worth the extra
code to handle it.

http://codereview.chromium.org/7453014/diff/5063/chrome/browser/sync/glue/mod...
File chrome/browser/sync/glue/model_associator.h (right):

http://codereview.chromium.org/7453014/diff/5063/chrome/browser/sync/glue/mod...
chrome/browser/sync/glue/model_associator.h:32: virtual bool
AssociateModels(SyncError* error) = 0;
On 2011/07/25 23:08:19, nzea wrote:
> On 2011/07/25 22:43:13, akalin wrote:
> > Change these to return SyncErrors too?  (and subclasses)
> 
> Since this is already an established (and now deprecated API) that relies on
> [Dis]AssociateModels returning true/false on success/error, and because we're
> planning on switching everything over to the new API anyways, I'd prefer to do
> the less intrusive change for now (just adding an extra parameter), if that's
> ok. I've added a TODO for once we have less datatypes reliant on this
approach.

Yeah, for practicality's sake I guess we can leave it like this.  Although I
have a feeling the old-style APIs are gonna stick around for a while. :|

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

http://codereview.chromium.org/7453014/diff/8007/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:1281: std::string message =
I get nervous with type-unsafe APIs like Stringprintf and c_str().  Can you use
stringstreams here instead?

[Nicolas Zea, 2011-07-26 00:23:46]

Committing once trybots go green.

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

http://codereview.chromium.org/7453014/diff/8007/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.cc:1281: std::string message =
On 2011/07/25 23:49:32, akalin wrote:
> I get nervous with type-unsafe APIs like Stringprintf and c_str().  Can you
use
> stringstreams here instead?

Style guide discourages streams when not used directly for logs. Gone ahead and
worked around the stringprintf/c_str usage though by adding a helper util for
converting ConfigureStatus to string.