sync: Remove ModelNeutralExecuteImpl()

sync: Remove ModelNeutralExecuteImpl()

ProcessUpdatesCommand was the only child of ModelChangingSyncerCommand
to override this function.  Since it is special, let's just treat it
specially in syncer.cc rather than modifying ModelChangingSyncerCommand
to suit its needs.

BUG=36594
TEST=


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

[rlarocque, 2011-11-16 20:38:23]

The return errors from SyncerCommand patch looks a bit nicer if I fix this
cleanup bug first.  Let me know what you think of this approach.

[tim (not reviewing), 2011-11-17 18:46:38]

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

http://codereview.chromium.org/8510079/diff/1/chrome/browser/sync/engine/sync...
chrome/browser/sync/engine/syncer.cc:146: if
(session->status_controller()->ResponseContainsUpdates()) {
I like this but the pattern here is typically to figure out next_step before
transitioning, as you'd draw it in a state diagram.  I can see arguments for
doing it this way (happens regardless of path taken to get here, consistent with
the previous implementation).

I'm curious where you're going though... could you provide any info on how this
helps you with the next piece?

[rlarocque, 2011-11-17 19:00:53]

On 2011/11/17 18:46:38, timsteele wrote:
>
http://codereview.chromium.org/8510079/diff/1/chrome/browser/sync/engine/sync...
> File chrome/browser/sync/engine/syncer.cc (right):
> 
>
http://codereview.chromium.org/8510079/diff/1/chrome/browser/sync/engine/sync...
> chrome/browser/sync/engine/syncer.cc:146: if
> (session->status_controller()->ResponseContainsUpdates()) {
> I like this but the pattern here is typically to figure out next_step before
> transitioning, as you'd draw it in a state diagram.  I can see arguments for
> doing it this way (happens regardless of path taken to get here, consistent
with
> the previous implementation).
> 
> I'm curious where you're going though... could you provide any info on how
this
> helps you with the next piece?

I'm intending to add a return value to SyncerCommand::Execute() and to all its
helper functions that do the actual work.  That includes ExecuteImpl(),
ModelChangingExecuteImpl(), and (possibly) ModelNeutralExecuteImpl().  I believe
this is what was agreed on in the meeting.  

The problem is that ModelNeutralExecuteImpl() doesn't fit.  It already has a
return value, which may or may not cause us to skip the non-ModelNeutral bits of
the command.  That means it can't return errors if anything goes wrong.  

I considered various alternatives for dealing with this, then decided to go with
this one.  I justified the if statement in syncer.cc by the fact that the
BUILD_COMMIT_REQUEST command was doing something similar.  Besides, if we're
going to have an early-out if statement, I'd prefer to see that if statement in
the syncer, rather than hidden in the base class of ProcessUpdatesCommand.  

Let me know if you want to discuss other alternatives.

[tim (not reviewing), 2011-11-17 19:36:52]

On 2011/11/17 19:00:53, rlarocque wrote:
> On 2011/11/17 18:46:38, timsteele wrote:
> >
>
http://codereview.chromium.org/8510079/diff/1/chrome/browser/sync/engine/sync...
> > File chrome/browser/sync/engine/syncer.cc (right):
> > 
> >
>
http://codereview.chromium.org/8510079/diff/1/chrome/browser/sync/engine/sync...
> > chrome/browser/sync/engine/syncer.cc:146: if
> > (session->status_controller()->ResponseContainsUpdates()) {
> > I like this but the pattern here is typically to figure out next_step before
> > transitioning, as you'd draw it in a state diagram.  I can see arguments for
> > doing it this way (happens regardless of path taken to get here, consistent
> with
> > the previous implementation).
> > 
> > I'm curious where you're going though... could you provide any info on how
> this
> > helps you with the next piece?
> 
> I'm intending to add a return value to SyncerCommand::Execute() and to all its
> helper functions that do the actual work.  That includes ExecuteImpl(),
> ModelChangingExecuteImpl(), and (possibly) ModelNeutralExecuteImpl().  I
believe
> this is what was agreed on in the meeting.  
> 
> The problem is that ModelNeutralExecuteImpl() doesn't fit.  It already has a
> return value, which may or may not cause us to skip the non-ModelNeutral bits
of
> the command.  That means it can't return errors if anything goes wrong.  
> 
> I considered various alternatives for dealing with this, then decided to go
with
> this one.  I justified the if statement in syncer.cc by the fact that the
> BUILD_COMMIT_REQUEST command was doing something similar.  Besides, if we're
> going to have an early-out if statement, I'd prefer to see that if statement
in
> the syncer, rather than hidden in the base class of ProcessUpdatesCommand.  
> 
> Let me know if you want to discuss other alternatives.

Yeah, I agree with your justification.  I mainly felt that the if belonged at
the point of deciding where to transition to (i.e. what next_step should be),
PROCESS_UPDATES or STORE_TIMESTAMPS depending on RequestContainsUpdates.  I do
think this fits the pattern better, but both have their advantages, so I'm not
especially passionate about it

LGTM!

[commit-bot: I haz the power, 2011-11-17 19:40:53]

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

[commit-bot: I haz the power, 2011-11-17 21:18:37]

Change committed as 110555