sync: Initialize entries with a valid model type

sync: Initialize entries with a valid model type

This change modifies two important sync functions.

The first is MutableEntry's CREATE constructor.  It now requires that
the ModelType of the entry to be specified when the entry is create.
This doesn't quite eliminate the existence of nodes without a valid
model type (the CREATE_NEW_UPDATE_ITEM can still make them), but it
helps.

This paves the way for the upcoming UniquePosition change.  Part of that
change is that bookmarks must be granted a unique tag on creation.  To
support this, we must know at creation time the type of a new entry, so
we can take the appropriate bookmark-specific steps if necessary.

The second function to get a new signature is WriteNode's
InitByCreation(), which has been renamed to
WriteNode::InitBookmarkByCreation().  The function can only be used to
create bookmarks, so the new name describes its function more precisely.

Updates to the call-sites of MutableEntry's constructor make up the
majority of this change.  This change also includes some minor updates
to test functions that create entries to make them compatible with the
stricter assertions or to make them more closely reflect real world
behaviour.

BUG=145412, 126505
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=176340

[rlarocque, 2013-01-09 01:20:35]

Please review.

This patch is big and tedious, but at least it's not very dangerous.  

My first attempt at this involved making a new MutableEntry constructor
specifically for creating bookmarks.  I had it working, but eventually decided
that change wasn't worthwhile and reverted it.

There are some left-overs from that approach that haven't been entirely
reverted.  The WriteNode::Init{,Bookmark}ByCreation change is one of them. 
These were left in because I thought they were an improvement on the existing
code.  I could revert them if you are strongly opposed.

[tim (not reviewing), 2013-01-10 20:16:43]

LGTM

https://codereview.chromium.org/11817010/diff/1/sync/engine/process_commit_re...
File sync/engine/process_commit_response_command_unittest.cc (right):

https://codereview.chromium.org/11817010/diff/1/sync/engine/process_commit_re...
sync/engine/process_commit_response_command_unittest.cc:83: int
CreateUnprocessedCommitResult(
Comment that the return value is the created metahandle.

https://codereview.chromium.org/11817010/diff/1/sync/engine/process_commit_re...
sync/engine/process_commit_response_command_unittest.cc:201: Entry
b_folder(&trans, syncable::GET_BY_HANDLE, bookmark_folder_handle);
Is there a practical advantage here to returning / using the handle instead of
the ID?  The code already passes the IDs in to CreateUnprocessedCommitResult,
seems like it'd be slightly easier to stick to IDs and not bring handles into
it?

[rlarocque, 2013-01-10 21:32:09]

Patch updated.  I'll send this to the CQ soon.

https://codereview.chromium.org/11817010/diff/1/sync/engine/process_commit_re...
File sync/engine/process_commit_response_command_unittest.cc (right):

https://codereview.chromium.org/11817010/diff/1/sync/engine/process_commit_re...
sync/engine/process_commit_response_command_unittest.cc:201: Entry
b_folder(&trans, syncable::GET_BY_HANDLE, bookmark_folder_handle);
On 2013/01/10 20:16:43, timsteele wrote:
> Is there a practical advantage here to returning / using the handle instead of
> the ID?  The code already passes the IDs in to CreateUnprocessedCommitResult,
> seems like it'd be slightly easier to stick to IDs and not bring handles into
> it?

The ID is renamed when the commit response is processed.

[commit-bot: I haz the power, 2013-01-10 23:20:23]

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

[commit-bot: I haz the power, 2013-01-11 13:34:38]

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

[commit-bot: I haz the power, 2013-01-11 13:43:13]

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

[commit-bot: I haz the power, 2013-01-11 13:52:36]

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

[commit-bot: I haz the power, 2013-01-11 15:11:23]

Change committed as 176340

[Joao da Silva, 2013-01-11 15:53:59]

This broke several bots:

http://build.chromium.org/p/chromium.win/buildstatus?builder=Vista%20Tests%20...

http://build.chromium.org/p/chromium.mac/buildstatus?builder=Mac10.6%20Tests%...

http://build.chromium.org/p/chromium.mac/buildstatus?builder=Mac10.7%20Tests%...

http://build.chromium.org/p/chromium.linux/buildstatus?builder=Linux%20Aura&n...

http://build.chromium.org/p/chromium.chromiumos/buildstatus?builder=Linux%20C...



Reverted in https://codereview.chromium.org/11863011/

[Joao da Silva, 2013-01-11 15:54:54]

Great, codereview broke the URLs. Here they are again:

http://build.chromium.org/p/chromium.win/buildstatus?builder=Vista%20Tests%20...

http://build.chromium.org/p/chromium.mac/buildstatus?builder=Mac10.6%20Tests%...

http://build.chromium.org/p/chromium.mac/buildstatus?builder=Mac10.7%20Tests%...

http://build.chromium.org/p/chromium.linux/buildstatus?builder=Linux%20Aura&n...

http://build.chromium.org/p/chromium.chromiumos/buildstatus?builder=Linux%20C...



On Fri, Jan 11, 2013 at 4:54 PM, <joaodasilva@chromium.org> wrote:

> This broke several bots:
>
> http://build.chromium.org/p/**chromium.win/buildstatus?**
>
builder=Vista%20Tests%20%283%**29&number=26229<http://build.chromium.org/p/chromium.win/buildstatus?builder=Vista%20Tests%20%283%29&number=26229>
>
> http://build.chromium.org/p/**chromium.mac/buildstatus?**
>
builder=Mac10.6%20Tests%20%**282%29&number=31185<http://build.chromium.org/p/chromium.mac/buildstatus?builder=Mac10.6%20Tests%20%282%29&number=31185>
>
> http://build.chromium.org/p/**chromium.mac/buildstatus?**
>
builder=Mac10.7%20Tests%20%**282%29&number=7505<http://build.chromium.org/p/chromium.mac/buildstatus?builder=Mac10.7%20Tests%20%282%29&number=7505>
>
> http://build.chromium.org/p/**chromium.linux/buildstatus?**
>
builder=Linux%20Aura&number=**970<http://build.chromium.org/p/chromium.linux/buildstatus?builder=Linux%20Aura&number=970>
>
> http://build.chromium.org/p/**chromium.chromiumos/**
>
buildstatus?builder=Linux%**20ChromiumOS%20Tests%20%281%**29&number=18694<http://build.chromium.org/p/chromium.chromiumos/buildstatus?builder=Linux%20ChromiumOS%20Tests%20%281%29&number=18694>
>
>
>
> Reverted in
https://codereview.chromium.**org/11863011/<https://codereview.chromium.org/1...
>
>
https://chromiumcodereview.**appspot.com/11817010/<https://chromiumcodereview...
>

[rlarocque, 2013-01-11 18:41:14]

https://codereview.chromium.org/11817010/diff/4001/sync/internal_api/test/tes...
File sync/internal_api/test/test_entry_factory.cc (right):

https://codereview.chromium.org/11817010/diff/4001/sync/internal_api/test/tes...
sync/internal_api/test/test_entry_factory.cc:123:
DCHECK(entry.PutPredecessor(predecessor_id));
I bet this is the cause of the buildbot failures.

I had fixed this in other patch stacks, but apparently that never got applied to
this one.

https://codereview.chromium.org/11817010/diff/4001/sync/syncable/mutable_entr...
File sync/syncable/mutable_entry.cc (right):

https://codereview.chromium.org/11817010/diff/4001/sync/syncable/mutable_entr...
sync/syncable/mutable_entry.cc:66: DCHECK(trans->directory()->InsertEntry(trans,
kernel_));
Actually, this is the root cause of the failures.  This one would break a lot
more tests.  

This, too, was fixed in a different patch stack.