sync: Implement per-type GetCommitIds

sync: Implement per-type GetCommitIds

Convert the SyncerCommand GetCommitIdsCommand into a function named
GetCommitIds that serves the same purpose.  There was no benefit in
keeping this functionality in a SyncerCommand, and doing so would have
made the implementation of a per-type GetCommitIds more difficult.

Expose a function to get the IDs to commit for a single data type.  This
function is used in the implementation of GetCommitIdsCommand, but
nowhere else.  It will become more useful when we try to implement the
model type abstraction layer.

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

[rlarocque, 2013-09-05 18:33:48]

Please review.

https://codereview.chromium.org/23809005/diff/8001/sync/engine/get_commit_ids.cc
File sync/engine/get_commit_ids.cc (right):

https://codereview.chromium.org/23809005/diff/8001/sync/engine/get_commit_ids...
sync/engine/get_commit_ids.cc:213: std::set<int64> added_handles_;
This member is not really necessary, but I felt bad about implementing
HaveItem() with an O(n) search where n ~= 90.  The old code's equivalent used an
OrderedCommitSet here, so its search was O(lg(n)).

I haven't profiled the change, so this may be premature optimization, but I
figured it would be safest to try to mimic the old code's algorithms.

[Nicolas Zea, 2013-09-05 23:55:36]

https://codereview.chromium.org/23809005/diff/8001/sync/engine/get_commit_ids.cc
File sync/engine/get_commit_ids.cc (right):

https://codereview.chromium.org/23809005/diff/8001/sync/engine/get_commit_ids...
sync/engine/get_commit_ids.cc:174: bool
AddUncommittedParentsAndTheirPredecessors(
make these methods private?

https://codereview.chromium.org/23809005/diff/8001/sync/engine/get_commit_ids...
sync/engine/get_commit_ids.cc:207: void
AppendManyToTraversal(std::vector<int64>);
variable name

https://codereview.chromium.org/23809005/diff/8001/sync/engine/get_commit_ids...
sync/engine/get_commit_ids.cc:212: std::vector<int64>* out_;
perhaps use Metahandles, which I believe is just the same type? (here and
elsewhere you have vectors of handles)

https://codereview.chromium.org/23809005/diff/8001/sync/engine/get_commit_ids...
sync/engine/get_commit_ids.cc:215: syncable::BaseTransaction* trans_;
DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/23809005/diff/8001/sync/engine/get_commit_ids...
sync/engine/get_commit_ids.cc:258: result->insert(result->end(),
dependencies.begin(), dependencies.end());
this is no longer reversing right?

https://codereview.chromium.org/23809005/diff/8001/sync/engine/get_commit_ids...
sync/engine/get_commit_ids.cc:489: size_t space_remaining = commit_batch_size -
ordered_commit_set->Size();
size_t is unsigned right? The <= 0 condition doesn't really make sense in that
case

https://codereview.chromium.org/23809005/diff/8001/sync/engine/get_commit_ids.h
File sync/engine/get_commit_ids.h (right):

https://codereview.chromium.org/23809005/diff/8001/sync/engine/get_commit_ids...
sync/engine/get_commit_ids.h:42: const size_t max_entries,
no need for const on size_t (here and below)

https://codereview.chromium.org/23809005/diff/8001/sync/engine/get_commit_ids...
sync/engine/get_commit_ids.h:67: SYNC_EXPORT_PRIVATE void GetCommitIds(
Are any of the other methods beyond this one used outside of the cc file? Do we
need to declare them in the header?

[rlarocque, 2013-09-06 18:58:26]

Patch updated.  PTAL.

https://codereview.chromium.org/23809005/diff/8001/sync/engine/get_commit_ids.cc
File sync/engine/get_commit_ids.cc (right):

https://codereview.chromium.org/23809005/diff/8001/sync/engine/get_commit_ids...
sync/engine/get_commit_ids.cc:174: bool
AddUncommittedParentsAndTheirPredecessors(
On 2013/09/05 23:55:37, Nicolas Zea wrote:
> make these methods private?

Done.

https://codereview.chromium.org/23809005/diff/8001/sync/engine/get_commit_ids...
sync/engine/get_commit_ids.cc:207: void
AppendManyToTraversal(std::vector<int64>);
On 2013/09/05 23:55:37, Nicolas Zea wrote:
> variable name

Done.

https://codereview.chromium.org/23809005/diff/8001/sync/engine/get_commit_ids...
sync/engine/get_commit_ids.cc:212: std::vector<int64>* out_;
On 2013/09/05 23:55:37, Nicolas Zea wrote:
> perhaps use Metahandles, which I believe is just the same type? (here and
> elsewhere you have vectors of handles)

Done.

https://codereview.chromium.org/23809005/diff/8001/sync/engine/get_commit_ids...
sync/engine/get_commit_ids.cc:215: syncable::BaseTransaction* trans_;
On 2013/09/05 23:55:37, Nicolas Zea wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

https://codereview.chromium.org/23809005/diff/8001/sync/engine/get_commit_ids...
sync/engine/get_commit_ids.cc:258: result->insert(result->end(),
dependencies.begin(), dependencies.end());
On 2013/09/05 23:55:37, Nicolas Zea wrote:
> this is no longer reversing right?

Good catch!  Somehow the unit tests missed this.

I've fixed the bug and added a test that will catch similar fixes in the future.

https://codereview.chromium.org/23809005/diff/8001/sync/engine/get_commit_ids...
sync/engine/get_commit_ids.cc:489: size_t space_remaining = commit_batch_size -
ordered_commit_set->Size();
On 2013/09/05 23:55:37, Nicolas Zea wrote:
> size_t is unsigned right? The <= 0 condition doesn't really make sense in that
> case

Good point.

This should still work as written, since all we really need to do here is break
when space_remaining == 0.  space_remaining < 0 should never happen.  

You're right that this doesn't make much sense, though.  I've rewritten this to
make it more explicit and avoid < 0 comparisons on unsigned ints.

https://codereview.chromium.org/23809005/diff/8001/sync/engine/get_commit_ids.h
File sync/engine/get_commit_ids.h (right):

https://codereview.chromium.org/23809005/diff/8001/sync/engine/get_commit_ids...
sync/engine/get_commit_ids.h:42: const size_t max_entries,
On 2013/09/05 23:55:37, Nicolas Zea wrote:
> no need for const on size_t (here and below)

Done.

https://codereview.chromium.org/23809005/diff/8001/sync/engine/get_commit_ids...
sync/engine/get_commit_ids.h:67: SYNC_EXPORT_PRIVATE void GetCommitIds(
On 2013/09/05 23:55:37, Nicolas Zea wrote:
> Are any of the other methods beyond this one used outside of the cc file? Do
we
> need to declare them in the header?

No, but I would like to keep their implementations ordered in the .cc file in a
way that preservers history and makes for readable diffs, and that requires
forward declaring these functions somewhere.

As per offline discussion, I moved these forward declarations to the .cc file.

I left the declaration for GetCommitIdsForType() in the header because I intend
to make use of it soon.

[tim (not reviewing), 2013-09-08 19:48:20]

https://codereview.chromium.org/23809005/diff/28001/sync/engine/get_commit_id...
File sync/engine/get_commit_ids.cc (right):

https://codereview.chromium.org/23809005/diff/28001/sync/engine/get_commit_id...
sync/engine/get_commit_ids.cc:82: DVLOG(1) << "Debug commit batch result:" <<
(*out)[i];
nit - It's a generally followed convention / good idea to have {} around macros.

https://codereview.chromium.org/23809005/diff/28001/sync/engine/get_commit_id...
sync/engine/get_commit_ids.cc:488: // placed last.
Wouldn't hurt to put this in the header file where you mention "valid commit
ordering", or refer to comments in the impl file for details.

https://codereview.chromium.org/23809005/diff/28001/sync/engine/get_commit_id...
sync/engine/get_commit_ids.cc:490: // Since the implementation of
UniquePositions, it is probably no longer
nit - It's better to succinctly state why things work the way they work now than
refer to previous implementations. that won't do anything to help a newcomer
(who's not familiar with the interpolating-position scheme we had before) to
this code understand how things work.

https://codereview.chromium.org/23809005/diff/28001/sync/engine/get_commit_id...
sync/engine/get_commit_ids.cc:492: // removed in the future.
nit - bug or todo for things that "may be removed in the future".

https://codereview.chromium.org/23809005/diff/28001/sync/engine/get_commit_ids.h
File sync/engine/get_commit_ids.h (right):

https://codereview.chromium.org/23809005/diff/28001/sync/engine/get_commit_id...
sync/engine/get_commit_ids.h:28: // in a valid commit ordering.
nit - Do we explain what 'valid commit ordering' is anywhere?

[rlarocque, 2013-09-09 17:41:29]

Addressed comments.  PTAL.

https://codereview.chromium.org/23809005/diff/28001/sync/engine/get_commit_id...
File sync/engine/get_commit_ids.cc (right):

https://codereview.chromium.org/23809005/diff/28001/sync/engine/get_commit_id...
sync/engine/get_commit_ids.cc:82: DVLOG(1) << "Debug commit batch result:" <<
(*out)[i];
On 2013/09/08 19:48:20, timsteele wrote:
> nit - It's a generally followed convention / good idea to have {} around
macros.

Done.

https://codereview.chromium.org/23809005/diff/28001/sync/engine/get_commit_id...
sync/engine/get_commit_ids.cc:488: // placed last.
On 2013/09/08 19:48:20, timsteele wrote:
> Wouldn't hurt to put this in the header file where you mention "valid commit
> ordering", or refer to comments in the impl file for details.

Done.

https://codereview.chromium.org/23809005/diff/28001/sync/engine/get_commit_id...
sync/engine/get_commit_ids.cc:490: // Since the implementation of
UniquePositions, it is probably no longer
On 2013/09/08 19:48:20, timsteele wrote:
> nit - It's better to succinctly state why things work the way they work now
than
> refer to previous implementations. that won't do anything to help a newcomer
> (who's not familiar with the interpolating-position scheme we had before) to
> this code understand how things work.

I don't think it can be avoided in this case.  There is no reason for this code
to do work to maintain ordering, except that that's the way it's always been.

I'll try to rephrase it so it can be better understood by newcomers.

https://codereview.chromium.org/23809005/diff/28001/sync/engine/get_commit_id...
sync/engine/get_commit_ids.cc:492: // removed in the future.
On 2013/09/08 19:48:20, timsteele wrote:
> nit - bug or todo for things that "may be removed in the future".

Done.

[Nicolas Zea, 2013-09-09 17:58:15]

LGTM with nits

https://codereview.chromium.org/23809005/diff/36001/sync/engine/get_commit_id...
File sync/engine/get_commit_ids.cc (right):

https://codereview.chromium.org/23809005/diff/36001/sync/engine/get_commit_id...
sync/engine/get_commit_ids.cc:30: void FilterUnreadyEntries(
I think it's useful to keep the function descriptions around at the declaration
site.

https://codereview.chromium.org/23809005/diff/36001/sync/engine/get_commit_id...
sync/engine/get_commit_ids.cc:81: for (size_t i = 0; i < out->size(); i++) {
put this entire thing in an if (DVLOG_IS_ON(1))?

https://codereview.chromium.org/23809005/diff/36001/sync/engine/get_commit_id...
sync/engine/get_commit_ids.cc:398: if (out_->size() > max_entries_) {
nit: remove curly braces

https://codereview.chromium.org/23809005/diff/36001/sync/engine/get_commit_ids.h
File sync/engine/get_commit_ids.h (right):

https://codereview.chromium.org/23809005/diff/36001/sync/engine/get_commit_id...
sync/engine/get_commit_ids.h:39: SYNC_EXPORT_PRIVATE void GetCommitIdsForType(
nit: this function isn't actually returning anything. Perhaps, like in the
comment below, say "Fills the specific |out| vector withe up to ..."

[rlarocque, 2013-09-09 18:36:25]

https://codereview.chromium.org/23809005/diff/36001/sync/engine/get_commit_id...
File sync/engine/get_commit_ids.cc (right):

https://codereview.chromium.org/23809005/diff/36001/sync/engine/get_commit_id...
sync/engine/get_commit_ids.cc:30: void FilterUnreadyEntries(
On 2013/09/09 17:58:15, Nicolas Zea wrote:
> I think it's useful to keep the function descriptions around at the
declaration
> site.

Done.

https://codereview.chromium.org/23809005/diff/36001/sync/engine/get_commit_id...
sync/engine/get_commit_ids.cc:81: for (size_t i = 0; i < out->size(); i++) {
On 2013/09/09 17:58:15, Nicolas Zea wrote:
> put this entire thing in an if (DVLOG_IS_ON(1))?

I trust the compiler will recognize this is a no-op and optimize appropriately.

https://codereview.chromium.org/23809005/diff/36001/sync/engine/get_commit_id...
sync/engine/get_commit_ids.cc:398: if (out_->size() > max_entries_) {
On 2013/09/09 17:58:15, Nicolas Zea wrote:
> nit: remove curly braces

Done.

[tim (not reviewing), 2013-09-09 22:38:06]

lgtm

[commit-bot: I haz the power, 2013-09-09 22:41:44]

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

[commit-bot: I haz the power, 2013-09-10 09:10:57]

Change committed as 222234