sync: Loop committing items without downloading updates

sync: Loop committing items without downloading updates

Since its inception sync has required all commits to be preceded by a
GetUpdates request.  This was done to try to ensure we detect and resolve
conflicts on the client, rather than having two conflicting commits collide at
the server.  It could never work perfectly, but it was likely to work in most
cases and the server would bounce the commit if it didn't.

Now we have a new server that doesn't always give us the latest version of a
node when we ask for it, especially when the request was not solicited by the
server (ie. not triggered by notification).  The update before commit is now
much less likely to detect conflicts.  Even worse, the useless update requests
can be surprisingly expensive in certain scenarios.

This change allows us to avoid fetching updates between 'batches' of commits.
This should improve performance in the case where we have lots of items to
commit (ie. first time sync) and reduce load on the server.

The implementation in this CL has some far-reaching effects.  This is in part
because I decided to refactor the commit code, but major changes would have
been required with or without the refactor.  Highlights include:
- The commit-related SyncerCommands are now paramaterized with local variables,
allowing us to remove many members fromt the SyncSession classes.
- Several syncer states have been collapsed into one COMMIT state.
- The unsynced_handles counter has been removed for now.  Following this
change, it would have always been zero unless an error was encountered during
the commit.  The functions that reference it expect different behaviour and
would have been broken by this change.
- The code to put extensions activity data back into the
ExtensionsActivityMonitor in case of failure has been moved into
ProcessCommitResponseCommand.  This fixes a bug where the data would have been
lost if the number of commit responses did not match the number of commit
requests.
- A CONFLICT response from the server is now treated as a transient error.
If we had continued to treat it as a success we might risk going into an
infinite loop.  See code for details.
- The "purposeless anachronism" conflicting_new_folder_ids_ has been removed.

BUG=91696
TEST=

[rlarocque, 2012-04-13 02:02:22]

Here's the first draft of the change.  I haven't yet tested it as fully as I
ought to.  I also have some more refactoring to do, but for now I want to keep
the diffs simple.

http://codereview.chromium.org/10038041/diff/1/sync/engine/post_commit_messag...
File sync/engine/post_commit_message_command.cc (left):

http://codereview.chromium.org/10038041/diff/1/sync/engine/post_commit_messag...
sync/engine/post_commit_message_command.cc:23: if
(session->status_controller().commit_ids().empty())
This if statement is no longer necessary, we break out of the loop in syncer.cc
if this condition is true.

http://codereview.chromium.org/10038041/diff/1/sync/engine/post_commit_messag...
sync/engine/post_commit_message_command.cc:30: if (result != SYNCER_OK) {
I believe this is the wrong place to put this code.  There are other cases where
our changes don't get through that are detected in ProcessCommitResponse.  For
now I think it's safer to assert ProcessCommitResponse is always reponsible for
clearing the SYNCING bits no matter what, rather than having this command handle
some failures while letting ProcessCommitResponse handles the success and
remaining failure cases.

http://codereview.chromium.org/10038041/diff/1/sync/engine/post_commit_messag...
File sync/engine/post_commit_message_command.cc (right):

http://codereview.chromium.org/10038041/diff/1/sync/engine/post_commit_messag...
sync/engine/post_commit_message_command.cc:27: return
SyncerProtoUtil::PostClientToServerMessage(
Seeing how little is left of this class, I intend to remove it and inline this
function.  That will happen either in a future CL, or a later revision of this
CL.  I want to keep the diffs simple for now.

http://codereview.chromium.org/10038041/diff/1/sync/engine/process_commit_res...
File sync/engine/process_commit_response_command.cc (right):

http://codereview.chromium.org/10038041/diff/1/sync/engine/process_commit_res...
sync/engine/process_commit_response_command.cc:200: return
SERVER_RETURN_TRANSIENT_ERROR;
You could argue that there should be some other return value to explicitly
support going back to GetUpdates.  There's no need to add a delay here.

I would counter-argue that this code path is deprecated, so any hack that
doesn't send us into an infinite loop is good enough.  Adding more code to
handle this case correctly has a non-zero risk; I don't think it's worth it.

http://codereview.chromium.org/10038041/diff/1/sync/engine/syncer.cc
File sync/engine/syncer.cc (right):

http://codereview.chromium.org/10038041/diff/1/sync/engine/syncer.cc#newcode191
sync/engine/syncer.cc:191: case COMMIT: {
I intend to move this block of code into a function.  This revision of the CL is
intended to keep the diff size manageable, so I haven't moved it yet.

http://codereview.chromium.org/10038041/diff/1/sync/engine/syncer.cc#newcode195
sync/engine/syncer.cc:195: size_t batch_size =
session->context()->max_commit_batch_size();
Nick warned me about this.  He suggested that the server might want to change
the commit batch size in response to certain client behaviours.  Obviously this
line prevents us from responding to those requests.

It turns out this is not a regression.  The syncer will currently respond to the
server's request to set the commit batch size only if the ClientCommand came in
on a GetUpdates response.  

Adding support for handling ClientCommands on all response messages is something
that makes sense to me, but we would have to start by turning
ProcessClientCommand into a function.  That's not something I want to tackle in
this CL.

Let me know if it makes sense to add support for this.  I will open a bug and
try to get it fixed by M20.

http://codereview.chromium.org/10038041/diff/1/sync/sessions/ordered_commit_s...
File sync/sessions/ordered_commit_set.h (right):

http://codereview.chromium.org/10038041/diff/1/sync/sessions/ordered_commit_s...
sync/sessions/ordered_commit_set.h:69: Projections::const_iterator i =
projections_.find(group);
Because operator[]() is non-const.