sync: Process client commands on commit response

sync: Process client commands on commit response

The server sends down client commands on both commit and get update
responses.  Prior to this change, the client would only react to the
commands sent in the get update response.  Now it will respond to
commands sent in either message.

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

[rlarocque, 2012-09-11 18:07:55]

Please review.

[tim (not reviewing), 2012-09-11 20:08:55]

http://codereview.chromium.org/10909160/diff/2002/sync/sessions/sync_session.cc
File sync/sessions/sync_session.cc (right):

http://codereview.chromium.org/10909160/diff/2002/sync/sessions/sync_session....
sync/sessions/sync_session.cc:155: void SyncSession::ProcessClientCommand(const
sync_pb::ClientCommand& command) {
We have a few "do on every request/response" actions now, and we're handling
most (all?) of them in PostClientToServerMessage. For example,
PersistBagOfChips, delegate()->OnSyncProtocolError(), SetProtocolVersion... etc.
 Could you move this handling to that method?  Whether or not it is the final
resting place for these things, it'd be nice to keep them together at a single
choke point versus maintaining them scattered in each command.

[rlarocque, 2012-09-11 20:28:54]

http://codereview.chromium.org/10909160/diff/2002/sync/sessions/sync_session.cc
File sync/sessions/sync_session.cc (right):

http://codereview.chromium.org/10909160/diff/2002/sync/sessions/sync_session....
sync/sessions/sync_session.cc:155: void SyncSession::ProcessClientCommand(const
sync_pb::ClientCommand& command) {
On 2012/09/11 20:08:55, timsteele wrote:
> We have a few "do on every request/response" actions now, and we're handling
> most (all?) of them in PostClientToServerMessage. For example,
> PersistBagOfChips, delegate()->OnSyncProtocolError(), SetProtocolVersion...
etc.
>  Could you move this handling to that method?  Whether or not it is the final
> resting place for these things, it'd be nice to keep them together at a single
> choke point versus maintaining them scattered in each command. 

Done, sort of.

I moved the call to this function into PostClientToServerMessage.  I left the
definition here.  That was how I interpreted your comment on first reading.  

Did you also want to inline this code into that method?

[tim (not reviewing), 2012-09-11 20:46:03]

On 2012/09/11 20:28:54, rlarocque wrote:
>
http://codereview.chromium.org/10909160/diff/2002/sync/sessions/sync_session.cc
> File sync/sessions/sync_session.cc (right):
> 
>
http://codereview.chromium.org/10909160/diff/2002/sync/sessions/sync_session....
> sync/sessions/sync_session.cc:155: void
SyncSession::ProcessClientCommand(const
> sync_pb::ClientCommand& command) {
> On 2012/09/11 20:08:55, timsteele wrote:
> > We have a few "do on every request/response" actions now, and we're handling
> > most (all?) of them in PostClientToServerMessage. For example,
> > PersistBagOfChips, delegate()->OnSyncProtocolError(), SetProtocolVersion...
> etc.
> >  Could you move this handling to that method?  Whether or not it is the
final
> > resting place for these things, it'd be nice to keep them together at a
single
> > choke point versus maintaining them scattered in each command. 
> 
> Done, sort of.
> 
> I moved the call to this function into PostClientToServerMessage.  I left the
> definition here.  That was how I interpreted your comment on first reading.  
> 
> Did you also want to inline this code into that method?

I think inlining it would make sense.  It's a little weird to have the session
doing actual sync protocol / engine work.  Typically 'engine' components are the
ones actually doing the work and syncable + sessions are just state / models /
abstractions.

[tim (not reviewing), 2012-09-11 22:15:33]

lgtm

[commit-bot: I haz the power, 2012-09-11 22:16:43]

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

[commit-bot: I haz the power, 2012-09-12 02:08:21]

Try job failure for 10909160-3013 (retry) on win_rel for steps
"interactive_ui_tests, browser_tests" (clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-09-12 19:17:58]

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

[commit-bot: I haz the power, 2012-09-12 20:07:10]

Try job failure for 10909160-3013 (retry) on android for steps "compile, build"
(clobber build).
It's a second try, previously, steps "compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[commit-bot: I haz the power, 2012-09-12 21:32:20]

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

[commit-bot: I haz the power, 2012-09-12 23:49:31]

Change committed as 156424