Server directed error handling backend code.

chrome/browser/sync/sessions/session_state.h

Index: chrome/browser/sync/sessions/session_state.h
diff --git a/chrome/browser/sync/sessions/session_state.h b/chrome/browser/sync/sessions/session_state.h
index 2abd559cb511b3da44249ddb5a71936f684c422b..93e8bed6b8b0e8bef4312a349a239634dfcd8907 100644
--- a/chrome/browser/sync/sessions/session_state.h
+++ b/chrome/browser/sync/sessions/session_state.h
@@ -23,6 +23,7 @@
 #include "chrome/browser/sync/engine/syncer_types.h"
 #include "chrome/browser/sync/engine/syncproto.h"
 #include "chrome/browser/sync/sessions/ordered_commit_set.h"
+#include "chrome/browser/sync/sessions/sync_error.h"

[tim (not reviewing), 2011/08/24 14:47:51]

Given that the convention here is having enums and structs in this file, I
probably would have kept things here... but I won't insist.

[lipalani1, 2011/08/25 06:14:06]

That was my first thought too. But I am adding another 100 lines,so decided to
make it a new file.
On 2011/08/24 14:47:51, timsteele wrote:

[tim (not reviewing), 2011/08/25 15:58:54]

OK, actually a new file makes sense here.
But I think it belongs in /protocol/ since it's roughly translating things from
the .protos...

 #include "chrome/browser/sync/syncable/model_type.h"
 #include "chrome/browser/sync/syncable/model_type_payload_map.h"
 #include "chrome/browser/sync/syncable/syncable.h"
@@ -131,7 +132,8 @@ struct SyncSessionSnapshot {
       bool did_commit_items,
       const SyncSourceInfo& source,
       size_t num_entries,
-      base::Time sync_start_time);
+      base::Time sync_start_time,
+      SyncError sync_error);

[tim (not reviewing), 2011/08/24 14:47:51]

I think this belongs in SyncerStatus or ErrorCounters, probably the latter
though I agree the naming is not ideal.  but snapshot.errors and
snapshot.sync_error smells bad, as does adding yet-another-parameter to this
constructor. The structs above are intended to reduce the impact of adding
things here.

I would add it to ErrorCounters and rename that to just Errors or ErrorState
(browser_sync::sessions::ErrorState).  You can add a TODO for the latter.

[lipalani1, 2011/08/25 06:14:06]

Ha ha went through the same argument in my head about adding to the constructor.
My idea here was to leave it seperate and add a todo to clean up this structure.
For example to take this one step further there is no reason why all of this
should not be a part of SyncerStatus(or a new syncerstatus structure that we
define). It currently has things like num_commits etc. Things like
has_more_to_sync, throttled etc should be a part of it as well as should the
error_counter. I am not fond of making the protocol error part of error_counter
even temporarily. But either way I am not passionate on this. So ok to change if
you feel there is a compelling reason. Otherwise I can leave a todo here to
clean up snapshot and file a bug. Currently it is renamed to
sync_protocol_error.(Also if we go this route, My thinking was this might be a
good starter bug for new people because it will introduce all the important
layers of sync. from syncer to ui, in a gentle(!) manner. ).
On 2011/08/24 14:47:51, timsteele wrote:

[tim (not reviewing), 2011/08/25 15:58:54]

I read this as "well, yes, this sucks, and we should fix it later, but in the
meantime I'll make it worse and add a todo" :)  Sometimes that's a fine argument
and I've definitely used it myself! But in this case I think there's a better
way that is easy.

I think having snap.errors and snap.sync_protocol_error is the most confusing of
the options we have. So I think it belongs in ErrorCounters, e.g. as
errors.protocol_error.  Whether you want to rename ErrorCounters now is up to
you.

   ~SyncSessionSnapshot();
 
   // Caller takes ownership of the returned dictionary.
@@ -154,6 +156,7 @@ struct SyncSessionSnapshot {
   const SyncSourceInfo source;
   const size_t num_entries;
   base::Time sync_start_time;
+  SyncError sync_error;
 };
 
 // Tracks progress of conflicts and their resolution using conflict sets.
@@ -323,6 +326,7 @@ struct AllModelTypeState {
   DirtyOnWrite<ErrorCounters> error_counters;
   SyncCycleControlParameters control_params;
   DirtyOnWrite<int64> num_server_changes_remaining;
+  DirtyOnWrite<SyncError> sync_error;
   OrderedCommitSet commit_set;
 };