[Sync] Add HasChildren() function and use it instead of GetFirstChildId()

[Sync] Add HasChildren() function and use it instead of GetFirstChildId()

This avoids some crashes when we try to traverse the list to find the
first child and encounter a dangling Id.

Remove some spurious virtual keywords.

BUG=100907
TEST=


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

[akalin, 2011-10-25 23:27:13]

+tim for review

[akalin, 2011-10-26 18:39:47]

-tim (who's out today), +zez

[akalin, 2011-10-26 18:39:54]

On 2011/10/26 18:39:47, akalin wrote:
> -tim (who's out today), +zez

er, +zea

[Nicolas Zea, 2011-10-26 22:21:35]

+Richard as FYI, since it seems like we now have three people working on the
same issue.

LGTM with nit

http://codereview.chromium.org/8396022/diff/1/chrome/browser/sync/syncable/sy...
File chrome/browser/sync/syncable/syncable_unittest.cc (right):

http://codereview.chromium.org/8396022/diff/1/chrome/browser/sync/syncable/sy...
chrome/browser/sync/syncable/syncable_unittest.cc:254: }
may as well add another case with id 97 has PREV_ID == NEXT_ID == ID, and ensure
we don't bork on the self loop.

[rlarocque, 2011-10-26 22:43:18]

LGTM.  

I want to be clear about what the goal is, though.  Is this intended to fix
crashes or pave the way for future fixes?  It looks like (and the comments
indicate) that this code is still vulnerable to crbug.com/100907.  What crash
case (if any) is this commit intended to fix?

http://codereview.chromium.org/8396022/diff/1/chrome/browser/sync/syncable/sy...
File chrome/browser/sync/syncable/syncable.cc (right):

http://codereview.chromium.org/8396022/diff/1/chrome/browser/sync/syncable/sy...
chrome/browser/sync/syncable/syncable.cc:1982: EntryKernel*
Directory::GetPossibleFirstChild(
nit:
Could we name this "GetFirstChildInServerOrdering" instead?  I think that would
more precisely describe what it does.

http://codereview.chromium.org/8396022/diff/1/chrome/browser/sync/syncable/sy...
File chrome/browser/sync/syncable/syncable.h (right):

http://codereview.chromium.org/8396022/diff/1/chrome/browser/sync/syncable/sy...
chrome/browser/sync/syncable/syncable.h:923: // Returns true iff |id| has
children.
Should we mention that self-looped children are not counted as children?  

That's an honest question.  I have no idea what it means for a child to be
self-looped, or what a caller of this function might expect if there are only
self-looped children.

[akalin, 2011-10-26 22:49:17]

On 2011/10/26 22:43:18, rlarocque wrote:
> LGTM.  
> 
> I want to be clear about what the goal is, though.  Is this intended to fix
> crashes or pave the way for future fixes?  It looks like (and the comments
> indicate) that this code is still vulnerable to crbug.com/100907.  What crash
> case (if any) is this commit intended to fix?

This is a partial fix for 100907.  I have a full fix in the works.  Basically,
calls to HasChildren can't crash, whereas calls to FindFirstChildId can.  So
callers that have been changed to call the former won't crash.

[akalin, 2011-10-27 00:19:20]

Landing as soon as trybots pass

http://codereview.chromium.org/8396022/diff/1/chrome/browser/sync/syncable/sy...
File chrome/browser/sync/syncable/syncable.cc (right):

http://codereview.chromium.org/8396022/diff/1/chrome/browser/sync/syncable/sy...
chrome/browser/sync/syncable/syncable.cc:1982: EntryKernel*
Directory::GetPossibleFirstChild(
On 2011/10/26 22:43:18, rlarocque wrote:
> nit:
> Could we name this "GetFirstChildInServerOrdering" instead?  I think that
would
> more precisely describe what it does.

Meh, I kinda disagree.  The callers of this class need know only that they
receive some node which is likely to be the first one.  How the node is found is
an implementation detail. (Even though this is a private function, it helps to
still think of it as an interface to the public methods of the class.)

http://codereview.chromium.org/8396022/diff/1/chrome/browser/sync/syncable/sy...
File chrome/browser/sync/syncable/syncable.h (right):

http://codereview.chromium.org/8396022/diff/1/chrome/browser/sync/syncable/sy...
chrome/browser/sync/syncable/syncable.h:923: // Returns true iff |id| has
children.
On 2011/10/26 22:43:18, rlarocque wrote:
> Should we mention that self-looped children are not counted as children?  
> 
> That's an honest question.  I have no idea what it means for a child to be
> self-looped, or what a caller of this function might expect if there are only
> self-looped children.

I don't think so, since deleted == self-looped.

However, the two functions above, which do return self-looped nodes, should be
documented as such.  Done.

http://codereview.chromium.org/8396022/diff/1/chrome/browser/sync/syncable/sy...
File chrome/browser/sync/syncable/syncable_unittest.cc (right):

http://codereview.chromium.org/8396022/diff/1/chrome/browser/sync/syncable/sy...
chrome/browser/sync/syncable/syncable_unittest.cc:254: }
On 2011/10/26 22:21:35, nzea wrote:
> may as well add another case with id 97 has PREV_ID == NEXT_ID == ID, and
ensure
> we don't bork on the self loop.

Done.

[commit-bot: I haz the power, 2011-10-27 03:58:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/8396022/4004

[commit-bot: I haz the power, 2011-10-27 05:08:30]

Change committed as 107533