Detect sync server communication errors

Detect sync server communication errors

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

This patch adds infrastructure to report errors involving communication
with the sync server back to the SyncSession and SyncScheduler.

It modifies SyncerProtoUtil::PostClientToServerMessage to have it return
an error code describing why it failed.  This function's callers (all of
which are SyncerCommands) use the infrastructure added in the first
patch of this series to return the error value to SyncShare().  From
there, the return values are placed into the session's StatusController
object.  The method SyncSession::Succeeded() method provides an easy way
to determine if the anything went wrong with the current sync session,
and its response is based on the stored return values.

This change has no effect on the client's behaviour.  Although it gives
the client access to lots more information, the client does not use it
for any decision making.

BUG=94670
TEST=


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

[rlarocque, 2012-01-10 00:55:18]

Part 1/3 of the issue 94670 fix was reviewed at
http://codereview.chromium.org/9149017/ and recently landed as r116963.  This is
part 2/3 in the series.

You can see the "big picture", including all three patches squashed together
(minus some fixes from the review of the first patch) at
http://codereview.chromium.org/9036003/.

[tim (not reviewing), 2012-01-10 17:17:33]

http://codereview.chromium.org/9158004/diff/1/chrome/browser/sync/engine/sync...
File chrome/browser/sync/engine/syncer_proto_util.cc (right):

http://codereview.chromium.org/9158004/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncer_proto_util.cc:105: 
nit - extra newline

http://codereview.chromium.org/9158004/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncer_proto_util.cc:353: DCHECK(server_status !=
browser_sync::HttpResponse::NONE);
use DCHECK_NE

http://codereview.chromium.org/9158004/diff/1/chrome/browser/sync/sessions/se...
File chrome/browser/sync/sessions/session_state.h (right):

http://codereview.chromium.org/9158004/diff/1/chrome/browser/sync/sessions/se...
chrome/browser/sync/sessions/session_state.h:110: SyncerError
download_updates_result;
If we only care about the last result, perhaps we should name this
last_download_updates_result to be accurate.

http://codereview.chromium.org/9158004/diff/1/chrome/browser/sync/sessions/st...
File chrome/browser/sync/sessions/status_controller.cc (right):

http://codereview.chromium.org/9158004/diff/1/chrome/browser/sync/sessions/st...
chrome/browser/sync/sessions/status_controller.cc:215:
shared_.error.mutate()->download_updates_result = result;
If you add stuff to this class, it's a good idea to add to
StatusControllerUnittest (to avoid refactorings or code moves causing bugs like
the one you just fixed with conflict_sets_built).

http://codereview.chromium.org/9158004/diff/1/chrome/browser/sync/sessions/sy...
File chrome/browser/sync/sessions/sync_session.cc (right):

http://codereview.chromium.org/9158004/diff/1/chrome/browser/sync/sessions/sy...
chrome/browser/sync/sessions/sync_session.cc:246: bool download_updates_error =
nit - these 3 bools could be const.

http://codereview.chromium.org/9158004/diff/1/chrome/browser/sync/sessions/sy...
File chrome/browser/sync/sessions/sync_session.h (right):

http://codereview.chromium.org/9158004/diff/1/chrome/browser/sync/sessions/sy...
chrome/browser/sync/sessions/sync_session.h:116: // errors.
"any sort of errors" is pretty vague.  I know we can't make it too specific
since it will rot, but a high level summary would be nice (network errors, proto
errors, ...)

[rlarocque, 2012-01-10 20:40:55]

Patch updated.

http://codereview.chromium.org/9158004/diff/1/chrome/browser/sync/engine/sync...
File chrome/browser/sync/engine/syncer_proto_util.cc (right):

http://codereview.chromium.org/9158004/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncer_proto_util.cc:105: 
On 2012/01/10 17:17:34, timsteele wrote:
> nit - extra newline

Done.

http://codereview.chromium.org/9158004/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncer_proto_util.cc:353: DCHECK(server_status !=
browser_sync::HttpResponse::NONE);
On 2012/01/10 17:17:34, timsteele wrote:
> use DCHECK_NE

Done.

http://codereview.chromium.org/9158004/diff/1/chrome/browser/sync/sessions/se...
File chrome/browser/sync/sessions/session_state.h (right):

http://codereview.chromium.org/9158004/diff/1/chrome/browser/sync/sessions/se...
chrome/browser/sync/sessions/session_state.h:110: SyncerError
download_updates_result;
On 2012/01/10 17:17:34, timsteele wrote:
> If we only care about the last result, perhaps we should name this
> last_download_updates_result to be accurate.

Done.

It's not that we only care about the most recent result, it's that it would be
difficult to respond to anything but the last error.  We currently don't have
the ability to handle errors during the sync cycle, so we'd have to save all of
them.  What would we do with a list of errors?  I think the only sensible thing
to do would be to ignore all but the most recent error, since we can't do
anything about the older errors.

The best solution would be to handle errors closer to where they occur, but that
won't be happening for a while.  This is a temporary compromise.

http://codereview.chromium.org/9158004/diff/1/chrome/browser/sync/sessions/st...
File chrome/browser/sync/sessions/status_controller.cc (right):

http://codereview.chromium.org/9158004/diff/1/chrome/browser/sync/sessions/st...
chrome/browser/sync/sessions/status_controller.cc:215:
shared_.error.mutate()->download_updates_result = result;
On 2012/01/10 17:17:34, timsteele wrote:
> If you add stuff to this class, it's a good idea to add to
> StatusControllerUnittest (to avoid refactorings or code moves causing bugs
like
> the one you just fixed with conflict_sets_built).

Done.

I usually wouldn't bother to test getter and setter methods, but that seems to
be the convention for this class.

http://codereview.chromium.org/9158004/diff/1/chrome/browser/sync/sessions/sy...
File chrome/browser/sync/sessions/sync_session.cc (right):

http://codereview.chromium.org/9158004/diff/1/chrome/browser/sync/sessions/sy...
chrome/browser/sync/sessions/sync_session.cc:246: bool download_updates_error =
On 2012/01/10 17:17:34, timsteele wrote:
> nit - these 3 bools could be const.

Done.

http://codereview.chromium.org/9158004/diff/1/chrome/browser/sync/sessions/sy...
File chrome/browser/sync/sessions/sync_session.h (right):

http://codereview.chromium.org/9158004/diff/1/chrome/browser/sync/sessions/sy...
chrome/browser/sync/sessions/sync_session.h:116: // errors.
On 2012/01/10 17:17:34, timsteele wrote:
> "any sort of errors" is pretty vague.  I know we can't make it too specific
> since it will rot, but a high level summary would be nice (network errors,
proto
> errors, ...)

All successes look alike, but there are many kinds of failures.  What I'm trying
to say here is that this returns false if the cycle was not a success, and true
otherwise.

I'll try to reword the comment.

[tim (not reviewing), 2012-01-11 01:09:00]

LGTM

http://codereview.chromium.org/9158004/diff/1/chrome/browser/sync/sessions/st...
File chrome/browser/sync/sessions/status_controller.cc (right):

http://codereview.chromium.org/9158004/diff/1/chrome/browser/sync/sessions/st...
chrome/browser/sync/sessions/status_controller.cc:215:
shared_.error.mutate()->download_updates_result = result;
On 2012/01/10 20:40:56, rlarocque wrote:
> On 2012/01/10 17:17:34, timsteele wrote:
> > If you add stuff to this class, it's a good idea to add to
> > StatusControllerUnittest (to avoid refactorings or code moves causing bugs
> like
> > the one you just fixed with conflict_sets_built).
> 
> Done.
> 
> I usually wouldn't bother to test getter and setter methods, but that seems to
> be the convention for this class.

Right... I made it that way because this file is particularly susceptible to
copy/paste errors.

[commit-bot: I haz the power, 2012-01-11 19:10:20]

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

[commit-bot: I haz the power, 2012-01-11 21:28:00]

Change committed as 117285