Refactor following sync commit loop change

Refactor following sync commit loop change

This change includes some cleanups of the code introduced in r139519.
They have been kept separate from that CL in the hopes of making both
CLs easiser to read.

This commit moves some error-detection functionality from
ProcessCommitResponse's ModelNeutralExecuteImpl() into
BuildAndPostCommits().  This simplifies some of the error handling and
allows us to remove ModelChangingSyncerCommand's
ModelNeutralExecuteImpl().

This CL also combines both commit error indicators into a single
variable.

BUG=91696, 36594
TEST=


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

[rlarocque, 2012-06-05 17:23:24]

Here are a few refactorings.  Most of these were discussed during
http://codereview.chromium.org/10210009/.

[tim (not reviewing), 2012-06-05 19:18:25]

Nice change.

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

http://codereview.chromium.org/10523003/diff/2001/sync/engine/commit.cc#newco...
sync/engine/commit.cc:108: ClearSyncingBits(session->context()->directory(),
commit_set);
Now that you've grouped everything here, instead of having to rely on calling
ClearSyncingBits at every current and future failure exit point, you could
create a ScopedSyncingBitClearer helper(directory, commit_set); 

that would call ClearSyncingBits on destruction (or just inline that function
into the dtor), unless we made it through successfully and made one single call
to helper.CoastIsClear() (or whatever) at the end of this block. Kind of like a
special version of base::AutoReset.

http://codereview.chromium.org/10523003/diff/2001/sync/engine/commit.cc#newco...
sync/engine/commit.cc:118: size_t num_responses =
commit_response.commit().entryresponse_size();
nit - const

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

http://codereview.chromium.org/10523003/diff/2001/sync/engine/syncer.cc#newco...
sync/engine/syncer.cc:186: BuildAndPostCommits(this, session));
nit- indent

[rlarocque, 2012-06-05 20:51:14]

Fixed a few things.  If you'd like, we can talk in person about the function to
clear syncing bits.

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

http://codereview.chromium.org/10523003/diff/2001/sync/engine/commit.cc#newco...
sync/engine/commit.cc:108: ClearSyncingBits(session->context()->directory(),
commit_set);
On 2012/06/05 19:18:25, timsteele wrote:
> Now that you've grouped everything here, instead of having to rely on calling
> ClearSyncingBits at every current and future failure exit point, you could
> create a ScopedSyncingBitClearer helper(directory, commit_set); 
> 
> that would call ClearSyncingBits on destruction (or just inline that function
> into the dtor), unless we made it through successfully and made one single
call
> to helper.CoastIsClear() (or whatever) at the end of this block. Kind of like
a
> special version of base::AutoReset.

I considered something like that.  I was actually hoping to create an object
that would set the bits on construction and clear the bits on destruction.  The
problem was that there was no scope where I could put it.  The setting must be
done in the same transaction as BuildCommitCommand, which isn't very
conveniently located.  

Also, there's no need to clear the bits after a successful process commit
response, which is the normal case.  It seems wrong to open a transaction to
redundantly clear those bits.  I guess this could be avoided by putting
'release()' function on that scoped object, but then it starts to feel like it's
more complicated than its worth.  

Because of those complications, I decided that duplicating the code was the
least bad option.

http://codereview.chromium.org/10523003/diff/2001/sync/engine/commit.cc#newco...
sync/engine/commit.cc:118: size_t num_responses =
commit_response.commit().entryresponse_size();
On 2012/06/05 19:18:25, timsteele wrote:
> nit - const

Fixed here and in a few other places.

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

http://codereview.chromium.org/10523003/diff/2001/sync/engine/syncer.cc#newco...
sync/engine/syncer.cc:186: BuildAndPostCommits(this, session));
On 2012/06/05 19:18:25, timsteele wrote:
> nit- indent

Done.

[tim (not reviewing), 2012-06-05 21:16:46]

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

http://codereview.chromium.org/10523003/diff/2001/sync/engine/commit.cc#newco...
sync/engine/commit.cc:108: ClearSyncingBits(session->context()->directory(),
commit_set);
On 2012/06/05 20:51:14, rlarocque wrote:
> On 2012/06/05 19:18:25, timsteele wrote:
> > Now that you've grouped everything here, instead of having to rely on
calling
> > ClearSyncingBits at every current and future failure exit point, you could
> > create a ScopedSyncingBitClearer helper(directory, commit_set); 
> > 
> > that would call ClearSyncingBits on destruction (or just inline that
function
> > into the dtor), unless we made it through successfully and made one single
> call
> > to helper.CoastIsClear() (or whatever) at the end of this block. Kind of
like
> a
> > special version of base::AutoReset.
> 
> I considered something like that.  I was actually hoping to create an object
> that would set the bits on construction and clear the bits on destruction. 
The
> problem was that there was no scope where I could put it.  The setting must be
> done in the same transaction as BuildCommitCommand, which isn't very
> conveniently located.  
> 
> Also, there's no need to clear the bits after a successful process commit
> response, which is the normal case.  It seems wrong to open a transaction to
> redundantly clear those bits.  I guess this could be avoided by putting
> 'release()' function on that scoped object, but then it starts to feel like
it's
> more complicated than its worth.  
> 
> Because of those complications, I decided that duplicating the code was the
> least bad option.

I see. I was imagining a class with a single "CoastIsClear" method that would
get called in only one place, but alternatively, we could factor the
ClearSyncingBits call out to a function that Clear()'d if the return value was
not SYNCER_OK. 

Either seem slightly more robust than maintaining calls to Clear when adding a
new early return, for example.

[rlarocque, 2012-06-05 23:35:14]

Alternative for clearing SYNCING bits uploaded.  

Note that this new version introduces a little bit of redundancy.  We don't need
to clear the syncing bits if the ProcessCommitResponseCommand fails, but this
version will do so anyway.  

I'm not too concerned about this.  I was actually considering adding this
redundant clear myself because it's not too expensive and there's a reasonable
risk that someone could change the behaviour of ProcessCommitResponseCommand and
break the assumption.

[tim (not reviewing), 2012-06-06 00:41:46]

couple nits, then LGTM

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

http://codereview.chromium.org/10523003/diff/8003/sync/engine/commit.cc#newco...
sync/engine/commit.cc:26: bool value) {
nit - consider renaming parameter value_to_set or the function to
IterateOverSyncingBitsAndSetToValue (or something shorter :/)

Comment might be nice too.

http://codereview.chromium.org/10523003/diff/8003/sync/engine/commit.cc#newco...
sync/engine/commit.cc:44: void ClearSyncingBits(syncable::Directory* dir,
nit - We don't really need this function and could inline it at
BuildAndProcessCommits.

http://codereview.chromium.org/10523003/diff/8003/sync/engine/commit.cc#newco...
sync/engine/commit.cc:131: return processing_result;
Note that elsewhere in this file we're using no-braces-on-single-line-ifs.
Please follow suit (or make things consistent).

[rlarocque, 2012-06-06 01:01:09]

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

http://codereview.chromium.org/10523003/diff/8003/sync/engine/commit.cc#newco...
sync/engine/commit.cc:26: bool value) {
On 2012/06/06 00:41:46, timsteele wrote:
> nit - consider renaming parameter value_to_set or the function to
> IterateOverSyncingBitsAndSetToValue (or something shorter :/)
> 
> Comment might be nice too.

How about SetAllSyncingBitsToValue()?

http://codereview.chromium.org/10523003/diff/8003/sync/engine/commit.cc#newco...
sync/engine/commit.cc:44: void ClearSyncingBits(syncable::Directory* dir,
On 2012/06/06 00:41:46, timsteele wrote:
> nit - We don't really need this function and could inline it at
> BuildAndProcessCommits.

I like this function's name better than IterateOverSyncingBits().  Otherwise I
would inline it.

http://codereview.chromium.org/10523003/diff/8003/sync/engine/commit.cc#newco...
sync/engine/commit.cc:131: return processing_result;
On 2012/06/06 00:41:46, timsteele wrote:
> Note that elsewhere in this file we're using no-braces-on-single-line-ifs.
> Please follow suit (or make things consistent).

I think 'with braces' style is more common.  I'll update to using that style
everywhere.

[tim (not reviewing), 2012-06-08 22:30:51]

LGTM

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

http://codereview.chromium.org/10523003/diff/8003/sync/engine/commit.cc#newco...
sync/engine/commit.cc:44: void ClearSyncingBits(syncable::Directory* dir,
On 2012/06/06 01:01:09, rlarocque wrote:
> On 2012/06/06 00:41:46, timsteele wrote:
> > nit - We don't really need this function and could inline it at
> > BuildAndProcessCommits.
> 
> I like this function's name better than IterateOverSyncingBits().  Otherwise I
> would inline it.

Good point, although SetAllSyncingBitsToValue(false) seems more on-par naming
wise...

[commit-bot: I haz the power, 2012-06-08 22:32:14]

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

[commit-bot: I haz the power, 2012-06-08 23:25:23]

Change committed as 141321