sync: Merge {Verify,Process}UpdatesCommand

sync: Merge {Verify,Process}UpdatesCommand

The VerifyUpdatesCommand was used to ensure the validity of each update
that had been delivered from the server and to prepare for the
ProcessUpdatesCommand.  The ProcessUpdatesCommand would then take all
updates that had been successfully verified and move them into the
SERVER_ fields of the sync directory.

It turns out that the logic can be greatly simplified by performing both
tasks within the same command.  This patch does not take full advantage
of the merge.  This patch is intended simply to merge the two files,
with the goal of performing more significant refactorings later.

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

[rlarocque, 2012-10-08 23:16:40]

Here's the patch to merge Verify and Process updates commands.  This is a detour
on the way to the conflict resolution refactor.

This patch moves a lot of code between files, so the diff isn't very helpful. 
To keep confusion to a minimum, I decided to limit the scope of this change.  I
have other patches queued that will build on this one.

Please review.

http://codereview.chromium.org/11091009/diff/2001/sync/engine/process_updates...
File sync/engine/process_updates_command.cc (right):

http://codereview.chromium.org/11091009/diff/2001/sync/engine/process_updates...
sync/engine/process_updates_command.cc:138: if (verify_result != VERIFY_SUCCESS
&& verify_result != VERIFY_UNDELETE)
This function up until this point is mostly copied and pasted from
VerifyUpdatesCommand::ModelChangingExecuteImpl().

At this point we transition over to ProcessUpdatesCommand.  However, instead of
retrieving the update verification status from the UpdateProgress struct, we
simply carry on from the current update verification result.  Process and Verify
now share the same iteration logic.

http://codereview.chromium.org/11091009/diff/2001/sync/engine/process_updates...
sync/engine/process_updates_command.cc:271: // since we last verified.
This is at least somewhat redundant.  I'll investigate removing it in future
patches.

[rlarocque, 2012-10-12 17:08:55]

On 2012/10/08 23:16:40, rlarocque wrote:
> Here's the patch to merge Verify and Process updates commands.  This is a
detour
> on the way to the conflict resolution refactor.
> 
> This patch moves a lot of code between files, so the diff isn't very helpful. 
> To keep confusion to a minimum, I decided to limit the scope of this change. 
I
> have other patches queued that will build on this one.
> 
> Please review.
> 
>
http://codereview.chromium.org/11091009/diff/2001/sync/engine/process_updates...
> File sync/engine/process_updates_command.cc (right):
> 
>
http://codereview.chromium.org/11091009/diff/2001/sync/engine/process_updates...
> sync/engine/process_updates_command.cc:138: if (verify_result !=
VERIFY_SUCCESS
> && verify_result != VERIFY_UNDELETE)
> This function up until this point is mostly copied and pasted from
> VerifyUpdatesCommand::ModelChangingExecuteImpl().
> 
> At this point we transition over to ProcessUpdatesCommand.  However, instead
of
> retrieving the update verification status from the UpdateProgress struct, we
> simply carry on from the current update verification result.  Process and
Verify
> now share the same iteration logic.
> 
>
http://codereview.chromium.org/11091009/diff/2001/sync/engine/process_updates...
> sync/engine/process_updates_command.cc:271: // since we last verified.
> This is at least somewhat redundant.  I'll investigate removing it in future
> patches.

ping.

[tim (not reviewing), 2012-10-15 21:06:52]

LGTM with one comment

http://codereview.chromium.org/11091009/diff/5001/sync/engine/process_updates...
File sync/engine/process_updates_command.cc (right):

http://codereview.chromium.org/11091009/diff/5001/sync/engine/process_updates...
sync/engine/process_updates_command.cc:125: if (g !=
status->group_restriction())
Add a comment here that we have to filter by ModelSafeGroup because we're
working off the raw GetUpdatesResponse and it's illegal to touch items that
don't live on this thread. Also, add a TODO(<you or me, no biggie>) and
reference bug 121521 (there probably should have been one before in
VerifyUpdatesCommand).

[rlarocque, 2012-10-15 22:47:32]

Patch updated.  Let me know if you see any problems with the wording of the
comment.

http://codereview.chromium.org/11091009/diff/5001/sync/engine/process_updates...
File sync/engine/process_updates_command.cc (right):

http://codereview.chromium.org/11091009/diff/5001/sync/engine/process_updates...
sync/engine/process_updates_command.cc:125: if (g !=
status->group_restriction())
On 2012/10/15 21:06:52, timsteele wrote:
> Add a comment here that we have to filter by ModelSafeGroup because we're
> working off the raw GetUpdatesResponse and it's illegal to touch items that
> don't live on this thread. Also, add a TODO(<you or me, no biggie>) and
> reference bug 121521 (there probably should have been one before in
> VerifyUpdatesCommand). 

Done.

[tim (not reviewing), 2012-10-15 23:22:32]

http://codereview.chromium.org/11091009/diff/11001/sync/engine/process_update...
File sync/engine/process_updates_command.cc (right):

http://codereview.chromium.org/11091009/diff/11001/sync/engine/process_update...
sync/engine/process_updates_command.cc:134: // TODO(tim): Hide these details. 
See crbug.com/121521 .
 "Don't allow access to objects in other ModelSafeGroups, bug 121521." might
better define the goal (vs "hide these details").

[rlarocque, 2012-10-16 00:08:24]

On 2012/10/15 23:22:32, timsteele wrote:
>
http://codereview.chromium.org/11091009/diff/11001/sync/engine/process_update...
> File sync/engine/process_updates_command.cc (right):
> 
>
http://codereview.chromium.org/11091009/diff/11001/sync/engine/process_update...
> sync/engine/process_updates_command.cc:134: // TODO(tim): Hide these details. 
> See crbug.com/121521 .
>  "Don't allow access to objects in other ModelSafeGroups, bug 121521." might
> better define the goal (vs "hide these details").

Thanks for the feedback.  Fixed.

[commit-bot: I haz the power, 2012-10-16 17:12:12]

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

[commit-bot: I haz the power, 2012-10-16 18:14:46]

Change committed as 162176