Add return values to SyncerCommand

Add return values to SyncerCommand

This is part 1 of 3 in a series of patches to remove the unnecessary
SYNC_CYCLE_CONTINUATION sync cycle that follows every sync commit.

In this patch, we add a return value to all SyncerCommands.  This will
make it easier for us to tell when a command has failed, and why.

Currently, the error detection does not work very well.  We don't have
much support for errors that occur on worker threads.  Many commands will
return an error only in case of directory lookup failure, but return
NO_ERROR otherwise.

This is OK because we're not aiming to fully implement error detection
in this commit.  This provides a base we can build on in future commits.

BUG=94670, 109422
TEST=


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

[rlarocque, 2012-01-05 21:30:26]

Please review.

This is part of the series towards removing the unnecessary
SYNC_CYCLE_CONTINUATION.  The squashed set of patches is reviewable at
http://codereview.chromium.org/9036003/.

This commit changes a lot of function signatures, but doesn't do much else. 
Sync's behaviour should remain unchanged.

[lipalani1, 2012-01-05 22:59:41]

The second comment is the most important one.

http://codereview.chromium.org/9113024/diff/1/chrome/browser/sync/engine/clea...
File chrome/browser/sync/engine/clear_data_command.cc (right):

http://codereview.chromium.org/9113024/diff/1/chrome/browser/sync/engine/clea...
chrome/browser/sync/engine/clear_data_command.cc:61: //
Change it to a TODO

http://codereview.chromium.org/9113024/diff/1/chrome/browser/sync/engine/mode...
File chrome/browser/sync/engine/model_changing_syncer_command.cc (right):

http://codereview.chromium.org/9113024/diff/1/chrome/browser/sync/engine/mode...
chrome/browser/sync/engine/model_changing_syncer_command.cc:47: // multiple
models at once.
I think you should return from here on the first instance of the error.

Any error that has surfaced here indicates no forward progress could not be made
in the sync cycle. If indeed forward progress could be made(like the error is
something benign) we should not return them because then in all places  down the
call stack(and up in the call stack as well) is this a benign error or not.

We should probably add the comment to the place where the error is defined as
well.

http://codereview.chromium.org/9113024/diff/1/chrome/browser/sync/engine/proc...
File chrome/browser/sync/engine/process_commit_response_command.cc (right):

http://codereview.chromium.org/9113024/diff/1/chrome/browser/sync/engine/proc...
chrome/browser/sync/engine/process_commit_response_command.cc:200: int
commit_count = static_cast<int>(proj.size());
Why was this moved up here?

[rlarocque, 2012-01-06 00:24:31]

Patch updated.

http://codereview.chromium.org/9113024/diff/1/chrome/browser/sync/engine/clea...
File chrome/browser/sync/engine/clear_data_command.cc (right):

http://codereview.chromium.org/9113024/diff/1/chrome/browser/sync/engine/clea...
chrome/browser/sync/engine/clear_data_command.cc:61: //
On 2012/01/05 22:59:41, lipalani1 wrote:
> Change it to a TODO

Done.

http://codereview.chromium.org/9113024/diff/1/chrome/browser/sync/engine/mode...
File chrome/browser/sync/engine/model_changing_syncer_command.cc (right):

http://codereview.chromium.org/9113024/diff/1/chrome/browser/sync/engine/mode...
chrome/browser/sync/engine/model_changing_syncer_command.cc:47: // multiple
models at once.
On 2012/01/05 22:59:41, lipalani1 wrote:
> I think you should return from here on the first instance of the error.
> 
> Any error that has surfaced here indicates no forward progress could not be
made
> in the sync cycle. If indeed forward progress could be made(like the error is
> something benign) we should not return them because then in all places  down
the
> call stack(and up in the call stack as well) is this a benign error or not.
> 
> We should probably add the comment to the place where the error is defined as
> well.

I agree this is probably the right thing to do.  I don't like the idea of having
one model affect another but, in this case, that's what makes the most sense. 
However, I don't want to do this yet.  

We don't currently kick out of the state machine if we detect an unrecoverable
error.  Any error handling we implement here would affect only this
SyncerCommand.  Other SyncerCommands would proceed as normal (unless they, too,
triggered UnrecoverableErrors).

Adding error handling in syncer.cc is beyond the scope of this change.  This
change is big enough as is, and I like the fact that it has no effect on syncer
behaviour.

http://codereview.chromium.org/9113024/diff/1/chrome/browser/sync/engine/proc...
File chrome/browser/sync/engine/process_commit_response_command.cc (right):

http://codereview.chromium.org/9113024/diff/1/chrome/browser/sync/engine/proc...
chrome/browser/sync/engine/process_commit_response_command.cc:200: int
commit_count = static_cast<int>(proj.size());
On 2012/01/05 22:59:41, lipalani1 wrote:
> Why was this moved up here?

Probably rebase conflicts.  Will undo.

[tim (not reviewing), 2012-01-06 16:33:42]

Nice change.  Mostly nits, with a few meatier suggestions.

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/c...
File chrome/browser/sync/engine/clear_data_command.cc (right):

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/c...
chrome/browser/sync/engine/clear_data_command.cc:60: // this code is
unreachable.  We should do something to clean up this mess.
This should be tagged with a bug, such as 71616.  The TODO should probably be
for Lingesh.

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/d...
File chrome/browser/sync/engine/download_updates_command.cc (right):

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/d...
chrome/browser/sync/engine/download_updates_command.cc:108: return NO_ERROR; //
TODO(rlarocque): Return an error here.
I'm assuming you aren't already because PostClientToServerMessage can fail for
several reasons? (If so, it'd be useful to at least add that to the comment
here).  Perhaps we should add a CLIENT_TO_SERVER_POST_FAILED for now so we are a
little more accurate, file a bug, and iterate on that (a sketch of a plan would
be by passing in the lookup to PostClientToServerMessage to avoid
DIRECTORY_OPEN_FAILED, examining the protocol_error in status to avoid having to
return proto errors, thus reducing the errors possibly returned from
PostClientToServerMessage to those of PostAndProcessHeaders.)

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/m...
File chrome/browser/sync/engine/model_changing_syncer_command.cc (right):

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/m...
chrome/browser/sync/engine/model_changing_syncer_command.cc:20: if (result !=
NO_ERROR) {
In general most of the sync code uses no-brace-on-single-line if. The only
"rule" is to at least make files consistent with themselves. So please change
either this one or your new code on line 48.

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/m...
chrome/browser/sync/engine/model_changing_syncer_command.cc:47: // multiple
models at once.
Indeed.  Can you file a bug to track this?

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/m...
File chrome/browser/sync/engine/model_changing_syncer_command.h (right):

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/m...
chrome/browser/sync/engine/model_changing_syncer_command.h:37: // wrapper so
implementations don't worry about storing work_session
style nit - Add a capital and period to that comment please.

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/p...
File chrome/browser/sync/engine/post_commit_message_command.cc (right):

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/p...
chrome/browser/sync/engine/post_commit_message_command.cc:45: return NO_ERROR;
// TODO(rlarocque): Return an error here.
See my earlier comment about PostClientToServerMessage errors.

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/p...
File chrome/browser/sync/engine/process_commit_response_command.cc (right):

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/p...
chrome/browser/sync/engine/process_commit_response_command.cc:98: // TODO(sync):
What if we didn't try to commit anything?
Is this still a legit comment?

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/r...
File chrome/browser/sync/engine/resolve_conflicts_command.cc (right):

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/r...
chrome/browser/sync/engine/resolve_conflicts_command.cc:25: CHECK(resolver);
This use of CHECK is not in accordance with Chromium general practices. Unless
I'm missing something and/or you are you trying to hunt something specific down.
The DCHECK was there to catch developer error when coding (i.e. I accidentally
refactor code away that was setting the resolver in the context), and NOT for
uber paranoid runtime errors / handling malformed input / security
vulnerability.
http://dev.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE...

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_command.h (right):

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_command.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
FYI, you'll have to change these again :)

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_command.h:37: SyncerError
Execute(sessions::SyncSession* session);
I think we should slap WARN_UNUSED_RESULT on this.  You'll have to edit
syncer.cc, but for now something like if (cmd.Execute() == UNINITIALIZED)
NOTREACHED() should be sufficient if there's nothing more intelligent to do.

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_error.h (right):

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_error.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
2012

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_error.h:11: enum SyncerError {
Please add a comment above here describing what this enum is for and how it
differs from other, similarly named error enums. (Or make it a file comment, up
above the ifdefs. Up to you.)

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_error.h:12: UNINITIALIZED = 0,       //
default value
style nit - Comments must begin with a capital letter and end with a period.

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_error.h:12: UNINITIALIZED = 0,       //
default value
Maybe INVALID?  UNINITIALIZED seems ambiguous...

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/glue/ui_...
File chrome/browser/sync/glue/ui_model_worker_unittest.cc (right):

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/glue/ui_...
chrome/browser/sync/glue/ui_model_worker_unittest.cc:36: return
browser_sync::NO_ERROR;
Part of me wishes we prefixed the values with SYNCER_ERROR_ (e.g.
SYNCER_ERROR_NONE), because I can think of at least 4 things that NO_ERROR could
mean in browser_sync :/ But yes, that will get lengthy...

[rlarocque, 2012-01-06 19:44:20]

New patch uploaded.

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/c...
File chrome/browser/sync/engine/clear_data_command.cc (right):

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/c...
chrome/browser/sync/engine/clear_data_command.cc:60: // this code is
unreachable.  We should do something to clean up this mess.
On 2012/01/06 16:33:42, timsteele wrote:
> This should be tagged with a bug, such as 71616.  The TODO should probably be
> for Lingesh.

Done.

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/d...
File chrome/browser/sync/engine/download_updates_command.cc (right):

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/d...
chrome/browser/sync/engine/download_updates_command.cc:108: return NO_ERROR; //
TODO(rlarocque): Return an error here.
On 2012/01/06 16:33:42, timsteele wrote:
> I'm assuming you aren't already because PostClientToServerMessage can fail for
> several reasons? (If so, it'd be useful to at least add that to the comment
> here).  Perhaps we should add a CLIENT_TO_SERVER_POST_FAILED for now so we are
a
> little more accurate, file a bug, and iterate on that (a sketch of a plan
would
> be by passing in the lookup to PostClientToServerMessage to avoid
> DIRECTORY_OPEN_FAILED, examining the protocol_error in status to avoid having
to
> return proto errors, thus reducing the errors possibly returned from
> PostClientToServerMessage to those of PostAndProcessHeaders.)  

This will be fixed in the next patch in this series.  Right now there's no harm
in returning NO_ERROR, since the return values are ignored.  The next patch will
add a return value to PostClientToServerMessage(), and will return it here.

The fix I'm planning doesn't look exactly like what you're suggesting, but
should be equally effective.

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/m...
File chrome/browser/sync/engine/model_changing_syncer_command.cc (right):

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/m...
chrome/browser/sync/engine/model_changing_syncer_command.cc:20: if (result !=
NO_ERROR) {
On 2012/01/06 16:33:42, timsteele wrote:
> In general most of the sync code uses no-brace-on-single-line if. The only
> "rule" is to at least make files consistent with themselves. So please change
> either this one or your new code on line 48.

Done.  Removed braces from this if statement.

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/m...
chrome/browser/sync/engine/model_changing_syncer_command.cc:47: // multiple
models at once.
On 2012/01/06 16:33:42, timsteele wrote:
> Indeed.  Can you file a bug to track this?

Done.  I've opened crbug.com/109422 to track this.  I probably should have
opened a bug like that long ago.  It's getting harder to relate this patch to
crbug.com/94670.

See also: Lingesh's comment in the previous version of this patch.

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/m...
File chrome/browser/sync/engine/model_changing_syncer_command.h (right):

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/m...
chrome/browser/sync/engine/model_changing_syncer_command.h:37: // wrapper so
implementations don't worry about storing work_session
On 2012/01/06 16:33:42, timsteele wrote:
> style nit - Add a capital and period to that comment please.

Done.

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/p...
File chrome/browser/sync/engine/post_commit_message_command.cc (right):

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/p...
chrome/browser/sync/engine/post_commit_message_command.cc:45: return NO_ERROR;
// TODO(rlarocque): Return an error here.
On 2012/01/06 16:33:42, timsteele wrote:
> See my earlier comment about PostClientToServerMessage errors.

My earlier response applies here, too.

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/p...
File chrome/browser/sync/engine/process_commit_response_command.cc (right):

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/p...
chrome/browser/sync/engine/process_commit_response_command.cc:98: // TODO(sync):
What if we didn't try to commit anything?
On 2012/01/06 16:33:42, timsteele wrote:
> Is this still a legit comment?

I doubt it.  We try not to post a commit request unless we have items to commit.
 But I'm not 100% sure that it's safe to remove.

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/r...
File chrome/browser/sync/engine/resolve_conflicts_command.cc (right):

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/r...
chrome/browser/sync/engine/resolve_conflicts_command.cc:25: CHECK(resolver);
On 2012/01/06 16:33:42, timsteele wrote:
> This use of CHECK is not in accordance with Chromium general practices. Unless
> I'm missing something and/or you are you trying to hunt something specific
down.
> The DCHECK was there to catch developer error when coding (i.e. I accidentally
> refactor code away that was setting the resolver in the context), and NOT for
> uber paranoid runtime errors / handling malformed input / security
> vulnerability.
>
http://dev.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE...
> 

Good point.  

I had changed this because I didn't like the code that handles cases where the
resolver was NULL.  Normally I wouldn't bother to change this, but in this case
not removing the return statement would mean I need to return *something*.  I
didn't want to add a RESOVLER_LOOKUP_FAILED to the SyncerError enum, so I
changed the DCHECK and remove the "if (!resolver)" check.

According to the style guidelines, it's recommended to not handle DCHECK
failures, so I guess I'll leave the DCHECK as is and remove the "if
(!resolver)".

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_command.h (right):

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_command.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2012/01/06 16:33:42, timsteele wrote:
> FYI, you'll have to change these again :)

Done.

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_command.h:37: SyncerError
Execute(sessions::SyncSession* session);
On 2012/01/06 16:33:42, timsteele wrote:
> I think we should slap WARN_UNUSED_RESULT on this.  You'll have to edit
> syncer.cc, but for now something like if (cmd.Execute() == UNINITIALIZED)
> NOTREACHED() should be sufficient if there's nothing more intelligent to do.

I don't see a point in adding WARN_UNUSED_RESULT if we're going to hack around
it.  Maybe we should wait until the error detection and handling is more fully
baked?

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_error.h (right):

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_error.h:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2012/01/06 16:33:42, timsteele wrote:
> 2012

Done.

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_error.h:11: enum SyncerError {
On 2012/01/06 16:33:42, timsteele wrote:
> Please add a comment above here describing what this enum is for and how it
> differs from other, similarly named error enums. (Or make it a file comment,
up
> above the ifdefs. Up to you.)

Done.

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_error.h:12: UNINITIALIZED = 0,       //
default value
On 2012/01/06 16:33:42, timsteele wrote:
> Maybe INVALID?  UNINITIALIZED seems ambiguous...

The way I see it, there is a difference.  INVALID means that the value doesn't
make sense.  UNINITIALIZED means that no one has bothered to change the value
from its default.  I would have made separate enum values for both of these,
except that we currently have no use for INVALID.  

Unfortunately, we're going to need check for UNINITIALIZED in a few cases.  The
current structure of the code is such that we will need to return a value for
"PostCommitMessageCommand", regardless of whether or not it is actually
executed.  We can check that return value against UNINITIALIZED to see whether
or not we attempted to post a commit during that sync cycle.

[tim (not reviewing), 2012-01-06 20:07:13]

Thanks! Just one last comment below, and also, you'll get presubmit errors for
all modified files that still have 2011 in the license header (or at least, you
should). It's that time of year where we ask for license updates as files get
changed :)

Once you have the try failures sorted out, it wouldn't hurt to throw it at a
clang trybot (unless you already build w/ clang).

Assuming Lingesh is happy w/ this as well, LGTM!

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/r...
File chrome/browser/sync/engine/resolve_conflicts_command.cc (right):

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/r...
chrome/browser/sync/engine/resolve_conflicts_command.cc:25: CHECK(resolver);
On 2012/01/06 19:44:21, rlarocque wrote:
> On 2012/01/06 16:33:42, timsteele wrote:
> > This use of CHECK is not in accordance with Chromium general practices.
Unless
> > I'm missing something and/or you are you trying to hunt something specific
> down.
> > The DCHECK was there to catch developer error when coding (i.e. I
accidentally
> > refactor code away that was setting the resolver in the context), and NOT
for
> > uber paranoid runtime errors / handling malformed input / security
> > vulnerability.
> >
>
http://dev.chromium.org/developers/coding-style#TOC-CHECK-vs-DCHECK-and-NOTRE...
> > 
> 
> Good point.  
> 
> I had changed this because I didn't like the code that handles cases where the
> resolver was NULL.  Normally I wouldn't bother to change this, but in this
case
> not removing the return statement would mean I need to return *something*.  I
> didn't want to add a RESOVLER_LOOKUP_FAILED to the SyncerError enum, so I
> changed the DCHECK and remove the "if (!resolver)" check.
> 
> According to the style guidelines, it's recommended to not handle DCHECK
> failures, so I guess I'll leave the DCHECK as is and remove the "if
> (!resolver)".

Sounds good.

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_command.h (right):

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_command.h:37: SyncerError
Execute(sessions::SyncSession* session);
On 2012/01/06 19:44:21, rlarocque wrote:
> On 2012/01/06 16:33:42, timsteele wrote:
> > I think we should slap WARN_UNUSED_RESULT on this.  You'll have to edit
> > syncer.cc, but for now something like if (cmd.Execute() == UNINITIALIZED)
> > NOTREACHED() should be sufficient if there's nothing more intelligent to do.
> 
> I don't see a point in adding WARN_UNUSED_RESULT if we're going to hack around
> it.  Maybe we should wait until the error detection and handling is more fully
> baked?

I admit, that's true.  But I do think long term it makes sense, unless things
change considerably (as you may be alluding to with the fully-baked comment). 
If you can't come up with something useful to do now with the result values, I
guess this is fine.  Consider a TODO.

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_error.h (right):

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_error.h:12: UNINITIALIZED = 0,       //
default value
On 2012/01/06 19:44:21, rlarocque wrote:
> On 2012/01/06 16:33:42, timsteele wrote:
> > Maybe INVALID?  UNINITIALIZED seems ambiguous...
> 
> The way I see it, there is a difference.  INVALID means that the value doesn't
> make sense.  UNINITIALIZED means that no one has bothered to change the value
> from its default.  I would have made separate enum values for both of these,
> except that we currently have no use for INVALID.  
> 
> Unfortunately, we're going to need check for UNINITIALIZED in a few cases. 
The
> current structure of the code is such that we will need to return a value for
> "PostCommitMessageCommand", regardless of whether or not it is actually
> executed.  We can check that return value against UNINITIALIZED to see whether
> or not we attempted to post a commit during that sync cycle.  

Fair enough.  Well, maybe "UNSET" or a synonym then?  The point I was aiming at
was if we're looking at code / debugging (or looking at values in a debugger and
talking about them in person),

SyncerError e;

is uninitialized.  But

SyncerError e(UNINITIALIZED);

is too.

[rlarocque, 2012-01-06 21:25:33]

Done.  

The latest patch should at least fix the Linux trybots.  It might fix the win
trybots, too.  

I've also renamed UNINITIALIZED to UNSET as you suggested, and updated lots of
copyright headers.

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_command.h (right):

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_command.h:37: SyncerError
Execute(sessions::SyncSession* session);
On 2012/01/06 20:07:13, timsteele wrote:
> On 2012/01/06 19:44:21, rlarocque wrote:
> > On 2012/01/06 16:33:42, timsteele wrote:
> > > I think we should slap WARN_UNUSED_RESULT on this.  You'll have to edit
> > > syncer.cc, but for now something like if (cmd.Execute() == UNINITIALIZED)
> > > NOTREACHED() should be sufficient if there's nothing more intelligent to
do.
> > 
> > I don't see a point in adding WARN_UNUSED_RESULT if we're going to hack
around
> > it.  Maybe we should wait until the error detection and handling is more
fully
> > baked?
> 
> I admit, that's true.  But I do think long term it makes sense, unless things
> change considerably (as you may be alluding to with the fully-baked comment). 
> If you can't come up with something useful to do now with the result values, I
> guess this is fine.  Consider a TODO.

I think the best way to track this is with crbug.com/109422.  It might be worth
an OKR, too.

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/s...
File chrome/browser/sync/engine/syncer_error.h (right):

http://codereview.chromium.org/9113024/diff/2002/chrome/browser/sync/engine/s...
chrome/browser/sync/engine/syncer_error.h:12: UNINITIALIZED = 0,       //
default value
On 2012/01/06 20:07:13, timsteele wrote:
> On 2012/01/06 19:44:21, rlarocque wrote:
> > On 2012/01/06 16:33:42, timsteele wrote:
> > > Maybe INVALID?  UNINITIALIZED seems ambiguous...
> > 
> > The way I see it, there is a difference.  INVALID means that the value
doesn't
> > make sense.  UNINITIALIZED means that no one has bothered to change the
value
> > from its default.  I would have made separate enum values for both of these,
> > except that we currently have no use for INVALID.  
> > 
> > Unfortunately, we're going to need check for UNINITIALIZED in a few cases. 
> The
> > current structure of the code is such that we will need to return a value
for
> > "PostCommitMessageCommand", regardless of whether or not it is actually
> > executed.  We can check that return value against UNINITIALIZED to see
whether
> > or not we attempted to post a commit during that sync cycle.  
> 
> Fair enough.  Well, maybe "UNSET" or a synonym then?  The point I was aiming
at
> was if we're looking at code / debugging (or looking at values in a debugger
and
> talking about them in person),
> 
> SyncerError e;
> 
> is uninitialized.  But
> 
> SyncerError e(UNINITIALIZED);
> 
> is too.

Good point.  I'll change it to UNSET.

[lipalani1, 2012-01-06 23:16:30]

LGTM from my side.

On Richard's answer about aborting the syncer I agree this change list should
not be expanded do that. In that case, assuming we are going in this direction,
we should change the todo in model_changing_syncer_command to read "we should
abort right away" or something to this meaning.

[rlarocque, 2012-01-07 00:05:18]

On 2012/01/06 23:16:30, lipalani1 wrote:
> LGTM from my side.
> 
> On Richard's answer about aborting the syncer I agree this change list should
> not be expanded do that. In that case, assuming we are going in this
direction,
> we should change the todo in model_changing_syncer_command to read "we should
> abort right away" or something to this meaning.

I'd prefer to leave it unspecified for now.  I'm only 90% sure that aborting
right away is the right solution.  We can discuss it on crbug.com/109422, which
the comment now refers to.  

====

The latest upload should fix the last of the trybot errors.  Apparently NO_ERROR
is #defined in some Windows header files, so I decided to rename it to
SYNCER_OK.  I also had to add an exception to the DEPS file in sync/glue to
allow the files in there to include "syncer_error.h".  

I consider these to be non-trivial changes, so I'll wait for another LGTM before
committing.

[rlarocque, 2012-01-09 21:33:24]

> The latest upload should fix the last of the trybot errors.  Apparently
NO_ERROR
> is #defined in some Windows header files, so I decided to rename it to
> SYNCER_OK.  I also had to add an exception to the DEPS file in sync/glue to
> allow the files in there to include "syncer_error.h".  
> 
> I consider these to be non-trivial changes, so I'll wait for another LGTM
before
> committing.

Tim did not agree with the DEPS file change and recommended moving the
syncer_error
files to internal_api/includes/ instead.  This is implemented in patch 7.

[tim (not reviewing), 2012-01-09 21:36:06]

lgtm

[commit-bot: I haz the power, 2012-01-09 21:45:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/9113024/18001

[commit-bot: I haz the power, 2012-01-10 00:06:54]

Change committed as 116963