sync: Loop committing items without downloading updates (v2)

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.

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 from the SyncSession
  classes.
- Several syncer states have been collapsed into one COMMIT state
  which redirects to the new BuildAndPostCommits() function.
- The PostCommitMessageCommand had been reduced to one line, which
  has now been inlined into the BuildAndPostCommits() function.
- 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 around to
  avoid double-counting if we have to post many commit messages.
- 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 comment in code for details.
- The "purposeless anachronism" conflicting_new_folder_ids_ has been
  removed.

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

[rlarocque, 2012-04-25 00:38:54]

Carried over from http://codereview.chromium.org/10038041/.  The rebase included
a lot of noise, so I decided to start a separate review.

Patch set 1 is similar to the patch available at the end of the previous review.
 Notable changes include a rebase, and moving around ExtensionsActivityMonitor
uploading code to make it compatible with the changes made to it in the "abort
sync cycles" patch.

The second patch set refactors some code.  It's the first version of this patch
that I would describe as "not ugly".  We could to take the refactoring further
by moving more code into sync/engine/commit.cc or inlining code from
non-ModelChanging syncer commands into the BuildAndPostCommits() function,
though think it would be better to leave those tasks to another CL.

[tim (not reviewing), 2012-04-25 06:48:13]

Hey Richard, I Had a few prelim questions, before looking too deep.

http://codereview.chromium.org/10210009/diff/2001/sync/engine/commit.cc
File sync/engine/commit.cc (right):

http://codereview.chromium.org/10210009/diff/2001/sync/engine/commit.cc#newcode5
sync/engine/commit.cc:5: #include "sync/engine/commit.h"
Can we add a commit_unittest to test high level BuildAndPostCommits
functionality specifically?

http://codereview.chromium.org/10210009/diff/2001/sync/engine/commit.cc#newco...
sync/engine/commit.cc:26: for ( ; ; ) {
What is this loop for?  It's not obvious to me.  Depending on what this is for
it might be better to have a while loop that explains its own existence?

http://codereview.chromium.org/10210009/diff/2001/sync/engine/commit.cc#newco...
sync/engine/commit.cc:77: }
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Write_...

One concern I have is that I think stepwise refining this routine now or in the
future is more cumbersome than the average Chrome component.  I wonder if this
might be better written as a class with a single public entry point, and as many
private helpers as needed that can all access (for example) the state in your
outermost scope.  

The motivation is that while I think we should try to break this up already (it
does exceed the 40-line style guide guilt-laying rule!), if we ever need to add
or expand some of the code above we're doomed to keep appending to this
function, float helper functions in the namespace for the world to (ab)use, or
add a bunch of local / file static functions. The latter is the probably the
best option, but even though logically all the steps share state we'll need to
plumb that list to each function, and so I think 'append' will happen most of
the time. Unless it is the natural / easy choice to do something else.

e.g. a case I've hit before is where I want to add a little code to do a bunch
of sanity-checks / DCHECKs. I'll be tempted to push it into a function I can
reuse, to keep the main routine concise. But if I have to pass a few lines worth
of arguments to that function each time, it's probably not worth it and I won't
bother.

http://codereview.chromium.org/10210009/diff/2001/sync/engine/commit.cc#newco...
sync/engine/commit.cc:77: }
Does this address / maintain somehow the early-exit-requested behavior between
commands?

[rlarocque, 2012-04-25 18:13:22]

We might need to continue this discussion in person.

http://codereview.chromium.org/10210009/diff/2001/sync/engine/commit.cc
File sync/engine/commit.cc (right):

http://codereview.chromium.org/10210009/diff/2001/sync/engine/commit.cc#newcode5
sync/engine/commit.cc:5: #include "sync/engine/commit.h"
On 2012/04/25 06:48:13, timsteele wrote:
> Can we add a commit_unittest to test high level BuildAndPostCommits
> functionality specifically?

This function is well covered by the SyncerTests.  Those tests have not been
lost in the refactor.

If we were starting from nothing, I agree that it would make more sense to write
those tests against this function rather than the syncer.  However, I don't see
a need to rewrite them right now.

http://codereview.chromium.org/10210009/diff/2001/sync/engine/commit.cc#newco...
sync/engine/commit.cc:26: for ( ; ; ) {
On 2012/04/25 06:48:13, timsteele wrote:
> What is this loop for?  It's not obvious to me.  Depending on what this is for
> it might be better to have a while loop that explains its own existence?

The initial idea was to loop over the set of all commit batches.  I think you
have a point, though.  This is more analogous to popping items of a stack one by
one than iterating through a list, since we are modifying the container as we
iterate.  A while loop would be clearer.  

I think it will still have to be a "while(1)" loop, though.  We don't know until
line 42 whether or not we should continue iterating.  I don't see any way to
squash all the lines between here and there into a single expression.

http://codereview.chromium.org/10210009/diff/2001/sync/engine/commit.cc#newco...
sync/engine/commit.cc:77: }
On 2012/04/25 06:48:13, timsteele wrote:
> Does this address / maintain somehow the early-exit-requested behavior between
> commands?

I believe it does.  If we receive an early exit request, the
ServerConnectionManager will be told to abort any pending network requests and
return an error.  This should allow us to exit the function fairly quickly.

http://codereview.chromium.org/10210009/diff/2001/sync/engine/commit.cc#newco...
sync/engine/commit.cc:77: }
On 2012/04/25 06:48:13, timsteele wrote:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Write_...
> 
> One concern I have is that I think stepwise refining this routine now or in
the
> future is more cumbersome than the average Chrome component.  I wonder if this
> might be better written as a class with a single public entry point, and as
many
> private helpers as needed that can all access (for example) the state in your
> outermost scope.  
> 
> The motivation is that while I think we should try to break this up already
(it
> does exceed the 40-line style guide guilt-laying rule!), if we ever need to
add
> or expand some of the code above we're doomed to keep appending to this
> function, float helper functions in the namespace for the world to (ab)use, or
> add a bunch of local / file static functions. The latter is the probably the
> best option, but even though logically all the steps share state we'll need to
> plumb that list to each function, and so I think 'append' will happen most of
> the time. Unless it is the natural / easy choice to do something else.
> 

This is exactly the sort of thing I want to encourage.  The alternative is to
store data as members that all these functions would have access to, making it
unclear which functions have access to which data.  Classes are meant to hide
the representation of data from the outside world, not to expose that data to a
set of functions on the inside.  

> e.g. a case I've hit before is where I want to add a little code to do a bunch
> of sanity-checks / DCHECKs. I'll be tempted to push it into a function I can
> reuse, to keep the main routine concise. But if I have to pass a few lines
worth
> of arguments to that function each time, it's probably not worth it and I
won't
> bother.  

If you need to pass a few lines worth of arguments to the function, and the
function is used from exactly one context, then I think it should be inlined. 
Moving it into a separate function won't make the code any less complex,
especially if this new function has access to exactly the same data as it did in
the original function context.  

=====

To bring this back to the code in question, here's a list of refactorings I had
in mind that should make this function less complex:
- Convert GetCommitIdsCommand and BuildCommitCommand from SyncerCommands into
functions.  That should reduce some boilerplate code (ie. the instantiation of
these objects before we call their functions), and eventually reduce our
reliance on the SyncSession.
- Replace last_process_commit_response and last_post_commit_response with a
single commit_response indicator.  This function could then return a single
value and let the Syncer handle setting it.
- Move the ModelNeutral part of ProcessCommitResponse into a helper function in
this context so we can simplify some of the early-exit logic (ie. no more
running ProcessCommitResponse after a commit fails).

My point here is that there are some simple steps we can take to improve this
function.  I just don't want this diff to get any bigger than it already is.

[tim (not reviewing), 2012-04-26 06:47:54]

(Still preliminary, I'll try to give this a full look tomorrow!)

http://codereview.chromium.org/10210009/diff/2001/sync/engine/commit.cc
File sync/engine/commit.cc (right):

http://codereview.chromium.org/10210009/diff/2001/sync/engine/commit.cc#newco...
sync/engine/commit.cc:77: }
On 2012/04/25 18:13:22, rlarocque wrote:
> On 2012/04/25 06:48:13, timsteele wrote:
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Write_...
> > 
> > One concern I have is that I think stepwise refining this routine now or in
> the
> > future is more cumbersome than the average Chrome component.  I wonder if
this
> > might be better written as a class with a single public entry point, and as
> many
> > private helpers as needed that can all access (for example) the state in
your
> > outermost scope.  
> > 
> > The motivation is that while I think we should try to break this up already
> (it
> > does exceed the 40-line style guide guilt-laying rule!), if we ever need to
> add
> > or expand some of the code above we're doomed to keep appending to this
> > function, float helper functions in the namespace for the world to (ab)use,
or
> > add a bunch of local / file static functions. The latter is the probably the
> > best option, but even though logically all the steps share state we'll need
to
> > plumb that list to each function, and so I think 'append' will happen most
of
> > the time. Unless it is the natural / easy choice to do something else.
> > 
> 
> This is exactly the sort of thing I want to encourage.  The alternative is to
> store data as members that all these functions would have access to, making it
> unclear which functions have access to which data.  Classes are meant to hide
> the representation of data from the outside world, not to expose that data to
a
> set of functions on the inside. 

Yeah, I'm not saying I wholesale disagree with that. But status_controller, dir,
and batch_size are scoped for every part of this function to use as you've
written it. So I don't think you would change much besides except
ease-of-creating subfunctions by making those three member variables of a class.
> 
> > e.g. a case I've hit before is where I want to add a little code to do a
bunch
> > of sanity-checks / DCHECKs. I'll be tempted to push it into a function I can
> > reuse, to keep the main routine concise. But if I have to pass a few lines
> worth
> > of arguments to that function each time, it's probably not worth it and I
> won't
> > bother.  
> 
> If you need to pass a few lines worth of arguments to the function, and the
> function is used from exactly one context, then I think it should be inlined. 
> Moving it into a separate function won't make the code any less complex,
> especially if this new function has access to exactly the same data as it did
in
> the original function context.  
> 
> =====
> 
> To bring this back to the code in question, here's a list of refactorings I
had
> in mind that should make this function less complex:
> - Convert GetCommitIdsCommand and BuildCommitCommand from SyncerCommands into
> functions.  That should reduce some boilerplate code (ie. the instantiation of
> these objects before we call their functions), and eventually reduce our
> reliance on the SyncSession.
> - Replace last_process_commit_response and last_post_commit_response with a
> single commit_response indicator.  This function could then return a single
> value and let the Syncer handle setting it.
> - Move the ModelNeutral part of ProcessCommitResponse into a helper function
in
> this context so we can simplify some of the early-exit logic (ie. no more
> running ProcessCommitResponse after a commit fails).
> 
> My point here is that there are some simple steps we can take to improve this
> function.  I just don't want this diff to get any bigger than it already is.

[rlarocque, 2012-05-08 00:48:32]

Updated.

Note: I should have renamed commit.* to committer.* when I defined the class.  I
decided not to rename it right now in order to make it easier to compare diffs. 
I will fix it before I commit this change.

http://codereview.chromium.org/10210009/diff/2001/sync/engine/commit.cc
File sync/engine/commit.cc (right):

http://codereview.chromium.org/10210009/diff/2001/sync/engine/commit.cc#newco...
sync/engine/commit.cc:77: }
> Yeah, I'm not saying I wholesale disagree with that. But status_controller,
dir,
> and batch_size are scoped for every part of this function to use as you've
> written it. So I don't think you would change much besides except
> ease-of-creating subfunctions by making those three member variables of a
class.

On that point, I agree.  The batch_size and dir definitions were written when I
was trying to implement this in a different way.  They're not required for the
current approach, and the code is clearer if I move or remove them.  

That leaves status_controller, which is useful as a way to save typing when
saving the results of the SyncerCommands.  I intend to remove those status
controller members in a follow-up patch, so I won't need the status_controller
definition for much longer.

======

Although there is no current need for class-scope variables, I will
pre-emptively embed this function inside a "class Committer".  If I'm right
(that we don't need class-scope variables) the Committer class will remain
memberless for the foreseeable future, no harm done.  If I'm wrong, then it's
easy to add members as required.

[rlarocque, 2012-05-11 00:28:14]

I've uploaded a new patch.  Changes include:

- Remove some unused variables.
- Remove the Committer class, as discussed offline.
- Rearrange the loop exit condition, as discussed offline.

I'm having second thoughts about the loop exit condition changes.  It looks a
bit uglier in practice than it seemed when we were discussing it.

[rlarocque, 2012-05-11 00:52:16]

http://codereview.chromium.org/10210009/diff/26002/sync/engine/commit.cc
File sync/engine/commit.cc (right):

http://codereview.chromium.org/10210009/diff/26002/sync/engine/commit.cc#newc...
sync/engine/commit.cc:77: } while (items_to_commit_count > batch_size);
I realized after posting this patch that there is a strange edge case here.

Consider crbug.com/94675 in this context.  If we delete a folder that contains
30 items, the items_to_commit_count will be 31 (30 items + the folder).  If
batch_size is 25, then this condition will be true and we will return to the top
of the loop.  

This would be not entirely correct.  The deletions will be sent over the network
as a single folder delete, which would fit in a single batch.  After a single
commit, we would be fully synced.  The extra iteration would be unnecessary.  

There's no harm in this, since we will exit on the next iteration on line 44. 
It's not very obvious to people reading the code, though, so I should add a
comment documenting this behaviour.

[tim (not reviewing), 2012-05-11 18:42:15]

http://codereview.chromium.org/10210009/diff/26002/sync/engine/build_commit_c...
File sync/engine/build_commit_command.h (right):

http://codereview.chromium.org/10210009/diff/26002/sync/engine/build_commit_c...
sync/engine/build_commit_command.h:23: BuildCommitCommand(const
sessions::OrderedCommitSet& batch_commit_set,
Comments for parameters.

http://codereview.chromium.org/10210009/diff/26002/sync/engine/build_commit_c...
sync/engine/build_commit_command.h:54: ClientToServerMessage* commit_message_;
Comments would be nice (at least covering ownership / lifetime for the raw *).

http://codereview.chromium.org/10210009/diff/26002/sync/engine/commit.cc
File sync/engine/commit.cc (right):

http://codereview.chromium.org/10210009/diff/26002/sync/engine/commit.cc#newc...
sync/engine/commit.cc:24: size_t items_to_commit_count = 0;
Perhaps this should be inside the loop.

http://codereview.chromium.org/10210009/diff/26002/sync/engine/commit.cc#newc...
sync/engine/commit.cc:36: GetCommitIdsCommand get_commit_ids_command(batch_size,
&commit_set,
This line is kind of taunting me to suggest an optimization where we don't need
to re-scan on each iteration, but I wonder how bad it is in practice (as you
mentioned yesterday, it's a whole lot better now without the extra GU in the
loop!).

Most of the ideas I can think of that would help are not exactly trivial. We
could snapshot each commit batch, and only re-scan the directory + filter if the
commit response changed anything that might affect IsEntryReadyForCommit
(encryption, throttled types). It's a bit tricky. We could keep an index on
"unready" items... humph. It's probably not worth spending too much thought on
right now. I doubt it really affects the steady state anyway (though it
certainly affects re-encryption and first-time-sync).

http://codereview.chromium.org/10210009/diff/26002/sync/engine/commit.cc#newc...
sync/engine/commit.cc:54:
SyncerProtoUtil::PostClientToServerMessage(commit_message,
As general practice all over the code, we ought to involve the session delegate
any time we are considering re-POSTing.

Previously, there was a chance for the scheduler to intercept between subsequent
GUs (admittedly, only by early exit requested) and subsequent [GU Commit] units,
either for backoff or early-exit requested.  Now, it looks like the atomic unit
is [GU Commit*]. I'm worried about having a tight loop around
PostClientToServerMessage without any interceptability at all. We've been burned
by tight loops in the past and it makes it harder to implement features related
to rate limiting since there's no single choke point.

For now the call could simply test early exit requested, but could be extended
in the future (for example if we chose to implement a doomsday-avoidance feature
like Lingesh proposed.)

It would also be nice to have a test verifying we boot out of here in certain
situations, depending on the delegate.

http://codereview.chromium.org/10210009/diff/26002/sync/engine/commit.cc#newc...
sync/engine/commit.cc:77: } while (items_to_commit_count > batch_size);
On 2012/05/11 00:52:16, rlarocque wrote:
> I realized after posting this patch that there is a strange edge case here.
> 
> Consider crbug.com/94675 in this context.  If we delete a folder that contains
> 30 items, the items_to_commit_count will be 31 (30 items + the folder).  If
> batch_size is 25, then this condition will be true and we will return to the
top
> of the loop.  
> 
> This would be not entirely correct.  The deletions will be sent over the
network
> as a single folder delete, which would fit in a single batch.  After a single
> commit, we would be fully synced.  The extra iteration would be unnecessary.  
> 
> There's no harm in this, since we will exit on the next iteration on line 44. 
> It's not very obvious to people reading the code, though, so I should add a
> comment documenting this behaviour.

A comment explaining what happens if local changes happen during a cycle might
be nice too (i.e. a nudge will get scheduled).

http://codereview.chromium.org/10210009/diff/26002/sync/engine/commit.h
File sync/engine/commit.h (right):

http://codereview.chromium.org/10210009/diff/26002/sync/engine/commit.h#newco...
sync/engine/commit.h:17: SyncerError BuildAndPostCommits(sessions::SyncSession*
session);
Comment this function.

http://codereview.chromium.org/10210009/diff/26002/sync/engine/get_commit_ids...
File sync/engine/get_commit_ids_command.cc (right):

http://codereview.chromium.org/10210009/diff/26002/sync/engine/get_commit_ids...
sync/engine/get_commit_ids_command.cc:29: :
requested_commit_batch_size_(commit_batch_size),
indent of : is off.

http://codereview.chromium.org/10210009/diff/26002/sync/engine/get_commit_ids...
File sync/engine/get_commit_ids_command.h (right):

http://codereview.chromium.org/10210009/diff/26002/sync/engine/get_commit_ids...
sync/engine/get_commit_ids_command.h:29: size_t* unsynced_item_count);
Comments would be nice here.

http://codereview.chromium.org/10210009/diff/26002/sync/engine/get_commit_ids...
sync/engine/get_commit_ids_command.h:118: size_t requested_commit_batch_size_;
const?

http://codereview.chromium.org/10210009/diff/26002/sync/engine/get_commit_ids...
sync/engine/get_commit_ids_command.h:119: sessions::OrderedCommitSet*
commit_set_;
Comments about ownership / lifetime wouldn't hurt.

http://codereview.chromium.org/10210009/diff/26002/sync/engine/process_commit...
File sync/engine/process_commit_response_command.cc (right):

http://codereview.chromium.org/10210009/diff/26002/sync/engine/process_commit...
sync/engine/process_commit_response_command.cc:112:
.ActiveGroupRestrictionIncludesModel(syncable::BOOKMARKS)) {
ActiveGroupRestrictionIncludesModel isn't used anywhere afaik (except within the
controller itself) and should be removed along with any other exposed mention of
group_restriction in status controller(bug 121521).  Im not sure why this method
in particular wasn't private already...

This bookmark commit activity case is one of few holes in the abstraction that
haven't been patched up, but we should keep it tied to bookmarks and not allow
the hole to get any larger with use of this more generic function.

http://codereview.chromium.org/10210009/diff/26002/sync/engine/process_commit...
sync/engine/process_commit_response_command.cc:200: return
SERVER_RETURN_TRANSIENT_ERROR;
Is this within the scope of this CL or a separate issue?  It's not immediately
clear to me why we need to change the behavior here for your CL to work. 
Particularly if we were already in backoff for unrelated reasons it seems like
this will cause unnecessary wait whereas before we would resume normal
operation.  Can you point out the benefit so it's more clear?

http://codereview.chromium.org/10210009/diff/26002/sync/engine/process_commit...
sync/engine/process_commit_response_command.cc:500: // here.
Good comment, though I think part of the ex-PostCommitMessageCommand comment
surrounding this code is still useful. The part about not to be confused with
IS_UNSYNCED might belong in syncable.h.

http://codereview.chromium.org/10210009/diff/26002/sync/sessions/ordered_comm...
File sync/sessions/ordered_commit_set.h (right):

http://codereview.chromium.org/10210009/diff/26002/sync/sessions/ordered_comm...
sync/sessions/ordered_commit_set.h:68: browser_sync::ModelSafeGroup group) const
{
We should probably move this to the .cc file.

http://codereview.chromium.org/10210009/diff/26002/sync/sessions/session_state.h
File sync/sessions/session_state.h (left):

http://codereview.chromium.org/10210009/diff/26002/sync/sessions/session_stat...
sync/sessions/session_state.h:122: bool did_commit_items,
Where is this captured, again?

http://codereview.chromium.org/10210009/diff/26002/sync/sessions/session_stat...
sync/sessions/session_state.h:164: int64 unsynced_count_;
You left this here, but removed it everywhere else (it seems).

How do we tell now from about:sync how many items were unsynced at the start of
the last cycle, and whether any of those were committed?

http://codereview.chromium.org/10210009/diff/26002/sync/sessions/sync_session...
File sync/sessions/sync_session_unittest.cc (left):

http://codereview.chromium.org/10210009/diff/26002/sync/sessions/sync_session...
sync/sessions/sync_session_unittest.cc:195: TEST_F(SyncSessionTest,
MoreToSyncIfUnsyncedGreaterThanCommitted) {
It would be nice to have a targeted specifically at commit.cc that it does loop
as expected if there are multiple batches.

[rlarocque, 2012-05-14 23:10:43]

I'm still investigating what the current test coverage looks like.  I'll address
those comments later.  

This new patch set should address the other issues, though.

http://codereview.chromium.org/10210009/diff/26002/sync/engine/build_commit_c...
File sync/engine/build_commit_command.h (right):

http://codereview.chromium.org/10210009/diff/26002/sync/engine/build_commit_c...
sync/engine/build_commit_command.h:23: BuildCommitCommand(const
sessions::OrderedCommitSet& batch_commit_set,
On 2012/05/11 18:42:16, timsteele wrote:
> Comments for parameters.

Done.

http://codereview.chromium.org/10210009/diff/26002/sync/engine/build_commit_c...
sync/engine/build_commit_command.h:54: ClientToServerMessage* commit_message_;
On 2012/05/11 18:42:16, timsteele wrote:
> Comments would be nice (at least covering ownership / lifetime for the raw *).

Done.

http://codereview.chromium.org/10210009/diff/26002/sync/engine/commit.cc
File sync/engine/commit.cc (right):

http://codereview.chromium.org/10210009/diff/26002/sync/engine/commit.cc#newc...
sync/engine/commit.cc:24: size_t items_to_commit_count = 0;
On 2012/05/11 18:42:16, timsteele wrote:
> Perhaps this should be inside the loop.

I tried.  It won't work.  The while() condition of the do loop doesn't have
access to that scope.

http://codereview.chromium.org/10210009/diff/26002/sync/engine/commit.cc#newc...
sync/engine/commit.cc:36: GetCommitIdsCommand get_commit_ids_command(batch_size,
&commit_set,
On 2012/05/11 18:42:16, timsteele wrote:
> This line is kind of taunting me to suggest an optimization where we don't
need
> to re-scan on each iteration, but I wonder how bad it is in practice (as you
> mentioned yesterday, it's a whole lot better now without the extra GU in the
> loop!).
> 
> Most of the ideas I can think of that would help are not exactly trivial. We
> could snapshot each commit batch, and only re-scan the directory + filter if
the
> commit response changed anything that might affect IsEntryReadyForCommit
> (encryption, throttled types). It's a bit tricky. We could keep an index on
> "unready" items... humph. It's probably not worth spending too much thought on
> right now. I doubt it really affects the steady state anyway (though it
> certainly affects re-encryption and first-time-sync).

I agree that we can do better.  At first, I tried being a bit more ambitious
here and implementing some of those optimizations.  It turns out that there are
some non-obvious difficulties, so I gave up and decided to mimic the "classic"
behaviour.  

I would be interested in revisiting this code after this CL lands.

http://codereview.chromium.org/10210009/diff/26002/sync/engine/commit.cc#newc...
sync/engine/commit.cc:54:
SyncerProtoUtil::PostClientToServerMessage(commit_message,
On 2012/05/11 18:42:16, timsteele wrote:
> As general practice all over the code, we ought to involve the session
delegate
> any time we are considering re-POSTing.
> 
> Previously, there was a chance for the scheduler to intercept between
subsequent
> GUs (admittedly, only by early exit requested) and subsequent [GU Commit]
units,
> either for backoff or early-exit requested.  Now, it looks like the atomic
unit
> is [GU Commit*]. I'm worried about having a tight loop around
> PostClientToServerMessage without any interceptability at all. We've been
burned
> by tight loops in the past and it makes it harder to implement features
related
> to rate limiting since there's no single choke point.
> 
> For now the call could simply test early exit requested, but could be extended
> in the future (for example if we chose to implement a doomsday-avoidance
feature
> like Lingesh proposed.)
> 

The ServerConnectionManager would handle that sort of thing.  As I've written
it, this code implicitly relies on the SCM for early exit.  I'd argue that the
SCM (or, better yet, URLFetcher) would be a much better place to implement
doomsday-avoidance.  I think almost everything that should be done for every
POST would be better off if it were implemented as close to the POST as
possible.

> It would also be nice to have a test verifying we boot out of here in certain
> situations, depending on the delegate.

I thought this code had inherited some of the existing tests for that scenario. 
I'll look into it.

http://codereview.chromium.org/10210009/diff/26002/sync/engine/commit.h
File sync/engine/commit.h (right):

http://codereview.chromium.org/10210009/diff/26002/sync/engine/commit.h#newco...
sync/engine/commit.h:17: SyncerError BuildAndPostCommits(sessions::SyncSession*
session);
On 2012/05/11 18:42:16, timsteele wrote:
> Comment this function.

Done.

http://codereview.chromium.org/10210009/diff/26002/sync/engine/get_commit_ids...
File sync/engine/get_commit_ids_command.cc (right):

http://codereview.chromium.org/10210009/diff/26002/sync/engine/get_commit_ids...
sync/engine/get_commit_ids_command.cc:29: :
requested_commit_batch_size_(commit_batch_size),
On 2012/05/11 18:42:16, timsteele wrote:
> indent of : is off.

Done.

http://codereview.chromium.org/10210009/diff/26002/sync/engine/get_commit_ids...
File sync/engine/get_commit_ids_command.h (right):

http://codereview.chromium.org/10210009/diff/26002/sync/engine/get_commit_ids...
sync/engine/get_commit_ids_command.h:29: size_t* unsynced_item_count);
On 2012/05/11 18:42:16, timsteele wrote:
> Comments would be nice here.

Done.

http://codereview.chromium.org/10210009/diff/26002/sync/engine/get_commit_ids...
sync/engine/get_commit_ids_command.h:118: size_t requested_commit_batch_size_;
On 2012/05/11 18:42:16, timsteele wrote:
> const?

Done.

http://codereview.chromium.org/10210009/diff/26002/sync/engine/get_commit_ids...
sync/engine/get_commit_ids_command.h:119: sessions::OrderedCommitSet*
commit_set_;
On 2012/05/11 18:42:16, timsteele wrote:
> Comments about ownership / lifetime wouldn't hurt.

Done.

http://codereview.chromium.org/10210009/diff/26002/sync/engine/process_commit...
File sync/engine/process_commit_response_command.cc (right):

http://codereview.chromium.org/10210009/diff/26002/sync/engine/process_commit...
sync/engine/process_commit_response_command.cc:112:
.ActiveGroupRestrictionIncludesModel(syncable::BOOKMARKS)) {
On 2012/05/11 18:42:16, timsteele wrote:
> ActiveGroupRestrictionIncludesModel isn't used anywhere afaik (except within
the
> controller itself) and should be removed along with any other exposed mention
of
> group_restriction in status controller(bug 121521).  Im not sure why this
method
> in particular wasn't private already...
> 
> This bookmark commit activity case is one of few holes in the abstraction that
> haven't been patched up, but we should keep it tied to bookmarks and not allow
> the hole to get any larger with use of this more generic function.

Works for me.  I'll re-add the HasBookmarkCommitActivity() function.

I wouldn't mind getting rid of this special case altogether, but that's a task
for future CLs.

http://codereview.chromium.org/10210009/diff/26002/sync/engine/process_commit...
sync/engine/process_commit_response_command.cc:200: return
SERVER_RETURN_TRANSIENT_ERROR;
On 2012/05/11 18:42:16, timsteele wrote:
> Is this within the scope of this CL or a separate issue?  It's not immediately
> clear to me why we need to change the behavior here for your CL to work. 
> Particularly if we were already in backoff for unrelated reasons it seems like
> this will cause unnecessary wait whereas before we would resume normal
> operation.  Can you point out the benefit so it's more clear?

This sort of thing highlights why this CL is dangerous.  

If this code remains unchanged, then we will return SYNCER_OK if we see a
conflicting commit.  We will return to the loop in commit.cc and keep iterating.
 A CONFLICT response does not modify our local items in any way, so
GetCommitIdsCommand will continue to return some items to commit.  We'll end up
attempting to commit the same conflicting items again, receiving a CONFLICT
response, and retrying indefinitely.

The SERVER_RETURN_TRANSIENT_ERROR return here will allow us to exit the loop and
try to resolve conflicts.

http://codereview.chromium.org/10210009/diff/26002/sync/engine/process_commit...
sync/engine/process_commit_response_command.cc:500: // here.
On 2012/05/11 18:42:16, timsteele wrote:
> Good comment, though I think part of the ex-PostCommitMessageCommand comment
> surrounding this code is still useful. The part about not to be confused with
> IS_UNSYNCED might belong in syncable.h.

Done.

http://codereview.chromium.org/10210009/diff/26002/sync/sessions/ordered_comm...
File sync/sessions/ordered_commit_set.h (right):

http://codereview.chromium.org/10210009/diff/26002/sync/sessions/ordered_comm...
sync/sessions/ordered_commit_set.h:68: browser_sync::ModelSafeGroup group) const
{
On 2012/05/11 18:42:16, timsteele wrote:
> We should probably move this to the .cc file.

Done.

http://codereview.chromium.org/10210009/diff/26002/sync/sessions/session_state.h
File sync/sessions/session_state.h (left):

http://codereview.chromium.org/10210009/diff/26002/sync/sessions/session_stat...
sync/sessions/session_state.h:122: bool did_commit_items,
On 2012/05/11 18:42:16, timsteele wrote:
> Where is this captured, again?

It was based on control_params.items_comitted, which was set at the end of
PostCommitMessageCommand.  This makes it much less useful than its name implies;
it gives us no indication as to whether or not the server accepted our commits.

http://codereview.chromium.org/10210009/diff/26002/sync/sessions/session_stat...
sync/sessions/session_state.h:164: int64 unsynced_count_;
On 2012/05/11 18:42:16, timsteele wrote:
> You left this here, but removed it everywhere else (it seems).
> 
> How do we tell now from about:sync how many items were unsynced at the start
of
> the last cycle, and whether any of those were committed?

Oops.  I meant to remove it here, too.

This CL leaves about:sync in rough shape.  I'll try to fix it in follow-up CLs. 
Nicolas's work to expose a fresher unsynced_count will help.  I'd also like to
add some strictly increasing counts of number of items committed.

It's important to note that we only trigger a refresh of about:sync after a sync
cycle is completed.  For cycles that commit many items, we will need to refresh
the page manually.  Previously, it would refresh itself with every commit batch.

[rlarocque, 2012-05-15 21:54:31]

You were right about the tests.  There were no unit tests remaining that tested
the commit loop.

In the latest patch set, I've modified an existing test and added two additional
tests to improve coverage.

[tim (not reviewing), 2012-05-15 22:25:00]

Although I spent a lot of time reviewing on this second pass, apparently all I
wound up with was a bunch of comments about comments :)  So looking pretty good
overall.

http://codereview.chromium.org/10210009/diff/26002/sync/engine/commit.cc
File sync/engine/commit.cc (right):

http://codereview.chromium.org/10210009/diff/26002/sync/engine/commit.cc#newc...
sync/engine/commit.cc:54:
SyncerProtoUtil::PostClientToServerMessage(commit_message,
On 2012/05/14 23:10:43, rlarocque wrote:
> On 2012/05/11 18:42:16, timsteele wrote:
> > As general practice all over the code, we ought to involve the session
> delegate
> > any time we are considering re-POSTing.
> > 
> > Previously, there was a chance for the scheduler to intercept between
> subsequent
> > GUs (admittedly, only by early exit requested) and subsequent [GU Commit]
> units,
> > either for backoff or early-exit requested.  Now, it looks like the atomic
> unit
> > is [GU Commit*]. I'm worried about having a tight loop around
> > PostClientToServerMessage without any interceptability at all. We've been
> burned
> > by tight loops in the past and it makes it harder to implement features
> related
> > to rate limiting since there's no single choke point.
> > 
> > For now the call could simply test early exit requested, but could be
extended
> > in the future (for example if we chose to implement a doomsday-avoidance
> feature
> > like Lingesh proposed.)
> > 
> 
> The ServerConnectionManager would handle that sort of thing.  As I've written
> it, this code implicitly relies on the SCM for early exit.  I'd argue that the
> SCM (or, better yet, URLFetcher) would be a much better place to implement
> doomsday-avoidance.  I think almost everything that should be done for every
> POST would be better off if it were implemented as close to the POST as
> possible.

My main point was that in general the delegate is going to have app-layer data /
knowledge that the http layer SCM won't, so it typically needs to be consulted
from wherever the decision to post or not is being made.

As long as we don't diverge too far from what we have today (e.g. with exit
requested status) we can deal with this later. In the meantime it'd be nice to
maintain peace of mind in that we're not regressing :)

> 
> > It would also be nice to have a test verifying we boot out of here in
certain
> > situations, depending on the delegate.
> 
> I thought this code had inherited some of the existing tests for that
scenario. 
> I'll look into it.

http://codereview.chromium.org/10210009/diff/26002/sync/sessions/session_state.h
File sync/sessions/session_state.h (left):

http://codereview.chromium.org/10210009/diff/26002/sync/sessions/session_stat...
sync/sessions/session_state.h:164: int64 unsynced_count_;
On 2012/05/14 23:10:43, rlarocque wrote:
> On 2012/05/11 18:42:16, timsteele wrote:
> > You left this here, but removed it everywhere else (it seems).
> > 
> > How do we tell now from about:sync how many items were unsynced at the start
> of
> > the last cycle, and whether any of those were committed?
> 
> Oops.  I meant to remove it here, too.
> 
> This CL leaves about:sync in rough shape.  I'll try to fix it in follow-up
CLs. 
> Nicolas's work to expose a fresher unsynced_count will help.  I'd also like to
> add some strictly increasing counts of number of items committed.
> 
> It's important to note that we only trigger a refresh of about:sync after a
sync
> cycle is completed.  For cycles that commit many items, we will need to
refresh
> the page manually.  Previously, it would refresh itself with every commit
batch.
>  

OK.  Can you make sure there are bugs documenting the gaps / regressions?

http://codereview.chromium.org/10210009/diff/20006/sync/engine/commit.cc
File sync/engine/commit.cc (right):

http://codereview.chromium.org/10210009/diff/20006/sync/engine/commit.cc#newc...
sync/engine/commit.cc:61: SyncerError BuildAndPostCommits(Syncer* syncer,
Hmm. Now it feels like we're trying to code around the fact that this function
should be on Syncer itself.  But if we did that here we'd just be taking a bunch
of code that was previously factored out and expanding it into syncer.cc, which
is a step backwards, because we can't let syncer.cc become a dumping ground
again :/ So .. okay. 

One solution would be to move the loop up to syncer and have this function
return whether you should continue invoking. The output of GetCommitIdsCommand
tells you whether you should continue. This would produce code similar to the
download-updates loop in syncer.cc and keep all the looping in one place, but I
know that's not the direction you'd like to take the code. I'm just noting my
concern that this path seems less defensive as it lacks a single choke-point,
and relies more heavily on keeping isolated pockets of code from diverging and
ensuring they always do the right thing / ask all the stakeholders before
proceeding.

I do like factoring out PrepareCommitMessage as you've done here. Although you
now have a few things allocated for the entire scope of BuildAndProcessCommits
(i.e. commit_set), and in an earlier patchset the argument against using a class
was that there was no such state. Not a big deal, just seems slightly more
plausible that future work / factoring on this code might benefit from being
part of a class.

http://codereview.chromium.org/10210009/diff/20006/sync/engine/commit.cc#newc...
sync/engine/commit.cc:74:
SyncerProtoUtil::PostClientToServerMessage(commit_message,
indent

http://codereview.chromium.org/10210009/diff/20006/sync/engine/get_commit_ids...
File sync/engine/get_commit_ids_command.h (right):

http://codereview.chromium.org/10210009/diff/20006/sync/engine/get_commit_ids...
sync/engine/get_commit_ids_command.h:27: 
The instantiate / execute / cleanup pattern is characteristic of SyncerCommand
and documented there with an example usage snippet. We shouldn't duplicate this
comment everywhere (or worse, only in some cases and not others). 

Feel free to augment the comment in SyncerCommand, but don't add / copy this
around here.  Also see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=File_C...,
the "duplicate comments diverge" part applies in spirit.

http://codereview.chromium.org/10210009/diff/20006/sync/engine/get_commit_ids...
sync/engine/get_commit_ids_command.h:33: 
Constructors are permitted to have parameters. The defensive perspective of this
comment is pretty awkward / confusing. It doesn't provide functional benefit to
a reader over individual descriptions for batch_size and ordered_commit_set, and
just talks about implementation details that don't concern the caller. I think
it's best to remove it, or at least make it un-defensive / work-aroundy sounding
to reduce undue confusion. See also
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
- it's a stretch to say it applies here, but the "people reading the code know
what ctors are for" is partially relevant in my mind.

http://codereview.chromium.org/10210009/diff/20006/sync/engine/process_commit...
File sync/engine/process_commit_response_command.cc (right):

http://codereview.chromium.org/10210009/diff/20006/sync/engine/process_commit...
sync/engine/process_commit_response_command.cc:197: 
Despite my earlier rant about defensive sounding comments being bad, this is a
case where I think it would be very helpful -- your explanation in the code
review comment about how we need to avoid looping forever and hence don't want
to return OK was eye opening.  Incorporate that here?  Maybe just as part of the
TODO, so the information / reasoning as to why this is safest isn't lost.

http://codereview.chromium.org/10210009/diff/20006/sync/engine/process_commit...
File sync/engine/process_commit_response_command.h (right):

http://codereview.chromium.org/10210009/diff/20006/sync/engine/process_commit...
sync/engine/process_commit_response_command.h:41: 
Same as with GetCommitIdsCommand.  In Chrome code comments that are part of an
interface or abstract class definition are not copied down to all subclasses. We
typically do carry a "// Foo implementation." above methods that are
implementing a superclass Foo method, but we don't copy all the comments around.

[tim (not reviewing), 2012-05-15 22:26:16]

Oh, just saw the comment re: tests. Cool, will take a look!

[rlarocque, 2012-05-16 21:13:35]

Replying here because codereview won't show me the diffs. :(

> > 
> > The ServerConnectionManager would handle that sort of thing.  As I've
written
> > it, this code implicitly relies on the SCM for early exit.  I'd argue that
the
> > SCM (or, better yet, URLFetcher) would be a much better place to implement
> > doomsday-avoidance.  I think almost everything that should be done for every
> > POST would be better off if it were implemented as close to the POST as
> > possible.
> 
> My main point was that in general the delegate is going to have app-layer data
/
> knowledge that the http layer SCM won't, so it typically needs to be consulted
> from wherever the decision to post or not is being made.
> 
> As long as we don't diverge too far from what we have today (e.g. with exit
> requested status) we can deal with this later. In the meantime it'd be nice to
> maintain peace of mind in that we're not regressing :)
> 

OK.  I think we're on the same page, the only disagreement (if any) is what
should be implemented when.  I think that adding the EarlyExitRequested() check
in the commit loop is a more than sufficient placeholder we can use while we
build the infrastructure needed to allow the app-layer to make decisions about
whether or not to stop syncing.  Agreed?

> > 
> > > It would also be nice to have a test verifying we boot out of here in
> certain
> > > situations, depending on the delegate.
> > 
> > I thought this code had inherited some of the existing tests for that
> scenario. 
> > I'll look into it.
> 

See patch set 6.

> > This CL leaves about:sync in rough shape.  I'll try to fix it in follow-up
> CLs. 
> > Nicolas's work to expose a fresher unsynced_count will help.  I'd also like
to
> > add some strictly increasing counts of number of items committed.
> > 
> > It's important to note that we only trigger a refresh of about:sync after a
> sync
> > cycle is completed.  For cycles that commit many items, we will need to
> refresh
> > the page manually.  Previously, it would refresh itself with every commit
> batch.
> >  
> 
> OK.  Can you make sure there are bugs documenting the gaps / regressions?
> 

Done: crbug.com/128409.  That change is required because the counters we used to
rely on (per-cycle commit counts) will be much less useful after this change.

> http://codereview.chromium.org/10210009/diff/20006/sync/engine/commit.cc
> File sync/engine/commit.cc (right):
> 
>
http://codereview.chromium.org/10210009/diff/20006/sync/engine/commit.cc#newc...
> sync/engine/commit.cc:61: SyncerError BuildAndPostCommits(Syncer* syncer,
> Hmm. Now it feels like we're trying to code around the fact that this function
> should be on Syncer itself.  But if we did that here we'd just be taking a
bunch
> of code that was previously factored out and expanding it into syncer.cc,
which
> is a step backwards, because we can't let syncer.cc become a dumping ground
> again :/ So .. okay. 
> 
> One solution would be to move the loop up to syncer and have this function
> return whether you should continue invoking. The output of GetCommitIdsCommand
> tells you whether you should continue. This would produce code similar to the
> download-updates loop in syncer.cc and keep all the looping in one place, but
I
> know that's not the direction you'd like to take the code. I'm just noting my
> concern that this path seems less defensive as it lacks a single choke-point,
> and relies more heavily on keeping isolated pockets of code from diverging and
> ensuring they always do the right thing / ask all the stakeholders before
> proceeding.
> 

I'm not fully convinced that the Syncer class ought to exist.  If we remove the
IsExitRequested() bit (which I think would be a good idea for other reasons),
then this class is left with almost no member variables.  I'm starting to think
that the Syncer class is a function in disguise, like the
ModelNeutralSyncerCommands.

If that's the case, then there's not much point in moving this function into the
Syncer.  Let's wait a while and revisit it once we've figured out how the syncer
code should interact with the app-layer logic that controls it.

> I do like factoring out PrepareCommitMessage as you've done here. Although you
> now have a few things allocated for the entire scope of BuildAndProcessCommits
> (i.e. commit_set), and in an earlier patchset the argument against using a
class
> was that there was no such state. Not a big deal, just seems slightly more
> plausible that future work / factoring on this code might benefit from being
> part of a class.
> 
>
http://codereview.chromium.org/10210009/diff/20006/sync/engine/commit.cc#newc...
> sync/engine/commit.cc:74:
> SyncerProtoUtil::PostClientToServerMessage(commit_message,
> indent

Done.

> 
>
http://codereview.chromium.org/10210009/diff/20006/sync/engine/get_commit_ids...
> File sync/engine/get_commit_ids_command.h (right):
> 
>
http://codereview.chromium.org/10210009/diff/20006/sync/engine/get_commit_ids...
> sync/engine/get_commit_ids_command.h:27: 
> The instantiate / execute / cleanup pattern is characteristic of SyncerCommand
> and documented there with an example usage snippet. We shouldn't duplicate
this
> comment everywhere (or worse, only in some cases and not others). 
> 
> Feel free to augment the comment in SyncerCommand, but don't add / copy this
> around here.  Also see
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=File_C...,
> the "duplicate comments diverge" part applies in spirit.
> 

I agree, though I thought this code might merit an exception.  If I actually
intended to maintain this code, duplicating comments would be a bad idea. 
However, I intend to refactor this in a follow-up CL, so the audience for these
comments is not developers trying to maintain this code, but developers trying
to dig through commit history.  

It's not a big deal either way, so I suppose I'll just replace it with a
reference to the SyncerCommand documentation.

>
http://codereview.chromium.org/10210009/diff/20006/sync/engine/get_commit_ids...
> sync/engine/get_commit_ids_command.h:33: 
> Constructors are permitted to have parameters. The defensive perspective of
this
> comment is pretty awkward / confusing. It doesn't provide functional benefit
to
> a reader over individual descriptions for batch_size and ordered_commit_set,
and
> just talks about implementation details that don't concern the caller. I think
> it's best to remove it, or at least make it un-defensive / work-aroundy
sounding
> to reduce undue confusion. See also
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
> - it's a stretch to say it applies here, but the "people reading the code know
> what ctors are for" is partially relevant in my mind.
> 

I'll replace it with a reference to some SyncerCommand documenation.

>
http://codereview.chromium.org/10210009/diff/20006/sync/engine/process_commit...
> File sync/engine/process_commit_response_command.cc (right):
> 
>
http://codereview.chromium.org/10210009/diff/20006/sync/engine/process_commit...
> sync/engine/process_commit_response_command.cc:197: 
> Despite my earlier rant about defensive sounding comments being bad, this is a
> case where I think it would be very helpful -- your explanation in the code
> review comment about how we need to avoid looping forever and hence don't want
> to return OK was eye opening.  Incorporate that here?  Maybe just as part of
the
> TODO, so the information / reasoning as to why this is safest isn't lost.
> 

I was trying to communicate that point in the second paragraph, but I suppose
that was a little too brief.  I'll add more detail.

>
http://codereview.chromium.org/10210009/diff/20006/sync/engine/process_commit...
> File sync/engine/process_commit_response_command.h (right):
> 
>
http://codereview.chromium.org/10210009/diff/20006/sync/engine/process_commit...
> sync/engine/process_commit_response_command.h:41: 
> Same as with GetCommitIdsCommand.  In Chrome code comments that are part of an
> interface or abstract class definition are not copied down to all subclasses.
We
> typically do carry a "// Foo implementation." above methods that are
> implementing a superclass Foo method, but we don't copy all the comments
around.

Will fix.

[rlarocque, 2012-05-16 21:48:17]

Comments updated.  PTAL.

[tim (not reviewing), 2012-05-23 16:41:18]

This looks ready to go, lets focus on working the kinks out when using against
prod.

http://codereview.chromium.org/10210009/diff/23007/chrome/browser/sync/profil...
File chrome/browser/sync/profile_sync_service_harness.cc (right):

http://codereview.chromium.org/10210009/diff/23007/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service_harness.cc:796: // good chance that
we're now fully up to date.
"there's a good chance that" invites "what if" questions, might be a good idea
to outline cases.  FWIW this method's name has always bothered me since it
overpromises what it can theoretically deliver (the "fully" in "fully synced"),
but not insisting you change it here.

[rlarocque, 2012-05-25 20:03:31]

http://codereview.chromium.org/10210009/diff/23007/chrome/browser/sync/profil...
File chrome/browser/sync/profile_sync_service_harness.cc (right):

http://codereview.chromium.org/10210009/diff/23007/chrome/browser/sync/profil...
chrome/browser/sync/profile_sync_service_harness.cc:796: // good chance that
we're now fully up to date.
On 2012/05/23 16:41:18, timsteele wrote:
> "there's a good chance that" invites "what if" questions, might be a good idea
> to outline cases.  FWIW this method's name has always bothered me since it
> overpromises what it can theoretically deliver (the "fully" in "fully
synced"),
> but not insisting you change it here.

Agreed, but that's not something this CL can fix.  My change is only rewording
the old comment to match the new code (unsynced_count() == 0 vs.
last_post_commit_result == UNSET).  The "fairly reliable indicator" wording in
the old comment was not much better, IMO.

[rlarocque, 2012-05-25 20:13:57]

We've confirmed that the issues I saw when testing with this patch were due to a
server-side bug.  It should be safe to commit.

[tim (not reviewing), 2012-05-25 21:01:22]

On 2012/05/25 20:13:57, rlarocque wrote:
> We've confirmed that the issues I saw when testing with this patch were due to
a
> server-side bug.  It should be safe to commit.

LGTM

[commit-bot: I haz the power, 2012-05-25 21:27:54]

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

[commit-bot: I haz the power, 2012-05-26 00:06:50]

Change committed as 139159