sync: Remove support for hierarchy deletions

sync: Remove support for hierarchy deletions

Removes client-side support for implicitly deleting a folder and all of
its contents by issuing a single folder delete to the server.

Increments the protocol version to advertise to servers that this client
handles folder deletions differently.

BUG=373869
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285201

[rlarocque, 2014-06-19 22:22:08]

Here's the patch.

It's a lot uglier than I would have liked, but it will have to do for now.  This
file is due for a rewrite.

Please review.

[maniscalco, 2014-06-19 23:01:34]

LGTM after you address the few comments I have below.

https://codereview.chromium.org/331863010/diff/40001/sync/engine/get_commit_i...
File sync/engine/get_commit_ids.cc (right):

https://codereview.chromium.org/331863010/diff/40001/sync/engine/get_commit_i...
sync/engine/get_commit_ids.cc:53: void GetCommitIdsForType(
Can you double check that the comment in the header file describing this
function is up-to-date with this CL?

https://codereview.chromium.org/331863010/diff/40001/sync/engine/get_commit_i...
sync/engine/get_commit_ids.cc:253: bool AddDeletedParents(const std::set<int64>&
ready_unsynced_set,
Looks like we could probably use "const MetahandleSet&" here and throughout the
file (from sync/syncable/metahandle_set.h).  Consistency is what's important so
I'd probably stick with what you have here or fix the whole file.

https://codereview.chromium.org/331863010/diff/40001/sync/engine/get_commit_i...
sync/engine/get_commit_ids.cc:255: const syncable::Directory::Metahandles
traversed,
Pass by reference?

https://codereview.chromium.org/331863010/diff/40001/sync/engine/get_commit_i...
sync/engine/get_commit_ids.cc:389: // The result list is reversed before it is
returned, so the resulting
It's worth mentioning that we're appending to result instead of "setting" it
(i.e. we don't clear it).

https://codereview.chromium.org/331863010/diff/40001/sync/engine/get_commit_i...
sync/engine/get_commit_ids.cc:397: syncable::Id parent_id = item.GetParentId();
Could item ever be IsRoot()?  Is that something to DCHECK?

https://codereview.chromium.org/331863010/diff/40001/sync/engine/get_commit_i...
sync/engine/get_commit_ids.cc:523: }
Should we be doing anything special when AddDeletedParents fails?  

Anything useful to log at this point (I see we're already logging at the "return
false" point)?  Any kind of counters we'd want incremented?

[rlarocque, 2014-06-19 23:37:33]

https://codereview.chromium.org/331863010/diff/40001/sync/engine/get_commit_i...
File sync/engine/get_commit_ids.cc (right):

https://codereview.chromium.org/331863010/diff/40001/sync/engine/get_commit_i...
sync/engine/get_commit_ids.cc:53: void GetCommitIdsForType(
On 2014/06/19 23:01:34, maniscalco wrote:
> Can you double check that the comment in the header file describing this
> function is up-to-date with this CL?

Good point.  Fixed.

https://codereview.chromium.org/331863010/diff/40001/sync/engine/get_commit_i...
sync/engine/get_commit_ids.cc:253: bool AddDeletedParents(const std::set<int64>&
ready_unsynced_set,
On 2014/06/19 23:01:34, maniscalco wrote:
> Looks like we could probably use "const MetahandleSet&" here and throughout
the
> file (from sync/syncable/metahandle_set.h).  Consistency is what's important
so
> I'd probably stick with what you have here or fix the whole file.

Let's leave it as is for now to keep the diff small.

https://codereview.chromium.org/331863010/diff/40001/sync/engine/get_commit_i...
sync/engine/get_commit_ids.cc:255: const syncable::Directory::Metahandles
traversed,
On 2014/06/19 23:01:34, maniscalco wrote:
> Pass by reference?

Done.

https://codereview.chromium.org/331863010/diff/40001/sync/engine/get_commit_i...
sync/engine/get_commit_ids.cc:389: // The result list is reversed before it is
returned, so the resulting
On 2014/06/19 23:01:34, maniscalco wrote:
> It's worth mentioning that we're appending to result instead of "setting" it
> (i.e. we don't clear it).

Done.

https://codereview.chromium.org/331863010/diff/40001/sync/engine/get_commit_i...
sync/engine/get_commit_ids.cc:397: syncable::Id parent_id = item.GetParentId();
On 2014/06/19 23:01:34, maniscalco wrote:
> Could item ever be IsRoot()?  Is that something to DCHECK?

That would imply that the root node is unsynced.  We've already DCHECKed against
that scenario in IsEntryReadyForCommit().

https://codereview.chromium.org/331863010/diff/40001/sync/engine/get_commit_i...
sync/engine/get_commit_ids.cc:523: }
On 2014/06/19 23:01:34, maniscalco wrote:
> Should we be doing anything special when AddDeletedParents fails?  
> 
> Anything useful to log at this point (I see we're already logging at the
"return
> false" point)?  Any kind of counters we'd want incremented?

Probably not.

AddCreatesAndMoves() has a similar case that doesn't bother to collect counters.

If we do have persistent conflicts, we should have collected that status when
trying to apply updates, so we do have some counters that can help us infer this
is a problem.

[rlarocque, 2014-07-17 00:24:41]

+Timon for server expertise.

The server-side support for this change has landed.  I think it's time to revive
this CL.

I'll give you another chance to review before I send this to the CQ.  PTAL.

[timonvo, 2014-07-17 16:14:37]

On 2014/07/17 00:24:41, rlarocque wrote:
> +Timon for server expertise.
> 
> The server-side support for this change has landed.  I think it's time to
revive
> this CL.
> 
> I'll give you another chance to review before I send this to the CQ.  PTAL.

Looks good on my end.

[maniscalco, 2014-07-23 23:10:29]

I just re-reviewed the latest patch set (#5), still LGTM.

[rlarocque, 2014-07-23 23:12:57]

The CQ bit was checked by rlarocque@chromium.org

[commit-bot: I haz the power, 2014-07-23 23:16:16]

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

[commit-bot: I haz the power, 2014-07-24 11:54:51]

Change committed as 285201