Sync: fix for the code that checks whether the initial download has completed

Sync: fix for the code that checks whether the initial download has completed

The code that detects whether the initial download has completed for
a type had been inadvertently broken by https://codereview.chromium.org/1142423006/

This fix changes Directory::InitialSyncEndedForType back to the
original implementation and adds special handling for implicitly created
root folders (i.e. root folders auto-created locally by the update
handling code). Since the locally created root folders don't get applied
with the rest of downloaded nodes, their base version has to be
explicitly changed from CHANGES_VERSION to some other value at
the end of update application process. That would to indicate that at
least one update has been applied for the type and be consistent with
how the server generated root folders are handled.

BUG=540814
Committed: https://crrev.com/b691b7a0f29e7ebafad5fadb8a6d59cce8cff279
Cr-Commit-Position: refs/heads/master@{#353913}

[stanisc, 2015-10-07 18:52:20]

stanisc@chromium.org changed reviewers:
+ zea@chromium.org

[stanisc, 2015-10-07 18:55:19]

PTAL!

[Nicolas Zea, 2015-10-07 23:42:56]

Do we have any tests that check the case where the application step has not been
reached (i.e. was interrupted)?

Also, could we go ahead and update the fake server and/or the python server to
enable implicit permanent folders?

[stanisc, 2015-10-09 05:18:31]

Patchset #2 (id:20001) has been deleted

[stanisc, 2015-10-09 15:37:32]

Fixed control datatypes, added some new unit tests, PTAL!

I am not aware of any end-to-end tests that abort before updates are applied.
Where should I look? I'll follow up today on enabling implicit permanent folders
in the fake server.

Meanwhile I've added a few lower level tests that would have caught the problem
when the semantics of Directory::InitialSyncEndedForType had changed. 
I've also done manual testing with the alpha server.

[Nicolas Zea, 2015-10-09 22:23:34]

I do think we need e2e test coverage of this using the fake server.

https://codereview.chromium.org/1393633003/diff/40001/sync/engine/apply_contr...
File sync/engine/apply_control_data_updates.cc (right):

https://codereview.chromium.org/1393633003/diff/40001/sync/engine/apply_contr...
sync/engine/apply_control_data_updates.cc:40: if (!entry.GetIsUnappliedUpdate())
{
Is this the right condition? The first time we download a control type root
(e.g. the nigori node), it will be unapplied. Don't we want to mark it as
initial sync ended though?

https://codereview.chromium.org/1393633003/diff/40001/sync/engine/apply_contr...
File sync/engine/apply_control_data_updates_unittest.cc (right):

https://codereview.chromium.org/1393633003/diff/40001/sync/engine/apply_contr...
sync/engine/apply_control_data_updates_unittest.cc:928:
EXPECT_FALSE(directory()->InitialSyncEndedForType(EXPERIMENTS));
This test doesn't cover the case where there are no child items (like with the
nigori node), which I think might be broken.

[stanisc, 2015-10-10 00:44:18]

https://codereview.chromium.org/1393633003/diff/40001/sync/engine/apply_contr...
File sync/engine/apply_control_data_updates.cc (right):

https://codereview.chromium.org/1393633003/diff/40001/sync/engine/apply_contr...
sync/engine/apply_control_data_updates.cc:40: if (!entry.GetIsUnappliedUpdate())
{
On 2015/10/09 22:23:33, Nicolas Zea wrote:
> Is this the right condition? The first time we download a control type root
> (e.g. the nigori node), it will be unapplied. Don't we want to mark it as
> initial sync ended though?

This case is to handle only locally created root node which isn't downloaded and
isn't unapplied. Never applies to Nigori which is handled below.

https://codereview.chromium.org/1393633003/diff/40001/sync/engine/apply_contr...
File sync/engine/apply_control_data_updates_unittest.cc (right):

https://codereview.chromium.org/1393633003/diff/40001/sync/engine/apply_contr...
sync/engine/apply_control_data_updates_unittest.cc:928:
EXPECT_FALSE(directory()->InitialSyncEndedForType(EXPERIMENTS));
On 2015/10/09 22:23:34, Nicolas Zea wrote:
> This test doesn't cover the case where there are no child items (like with the
> nigori node), which I think might be broken.

This case handles control types with implicit permanent folder. 
Nigori isn't broken. Handling of Nigori is unchanged - it still comes from the
server unapplied with base version = CHANGES_VERSION and the normal application
process still changes its base version. Basically for server created root nodes
there is no need to call the new MarkInitialSyncEndedForType method.
But you are right that there is no test case for Nigore. I'll see if I can add
one or modify one of the Nigori test cases above.

[stanisc, 2015-10-13 16:33:02]

PTAL!

Added/modified 3 more unit tests to cover the following cases:
- Initial update of Nigori node
- Initial update of a datatype with no entities, with progress marker only
- Aborted multi-package update where applying updates step doesn't run.

[Nicolas Zea, 2015-10-13 17:51:37]

LGTM with some nits.

https://codereview.chromium.org/1393633003/diff/60001/sync/engine/apply_contr...
File sync/engine/apply_control_data_updates_unittest.cc (right):

https://codereview.chromium.org/1393633003/diff/60001/sync/engine/apply_contr...
sync/engine/apply_control_data_updates_unittest.cc:942:
EXPECT_FALSE(directory()->InitialSyncEndedForType(NIGORI));
Did you mean to leave this in? (maybe comment why?)

https://codereview.chromium.org/1393633003/diff/60001/sync/engine/syncer_unit...
File sync/engine/syncer_unittest.cc (right):

https://codereview.chromium.org/1393633003/diff/60001/sync/engine/syncer_unit...
sync/engine/syncer_unittest.cc:4711: TEST_F(SyncerTest,
ConfigureFailsDontApplyUpdates) {
Should this be updated? AFAICT you're now actually applying updates, and
successfully configuring.

Maybe pull your logic into a separate test?

https://codereview.chromium.org/1393633003/diff/60001/sync/engine/syncer_unit...
sync/engine/syncer_unittest.cc:4787: TEST_F(SyncerTest,
ProgressMarkerOnlyUpdate) {
This is to test the the implicit permanent folder is created right? Maybe add a
comment about that?

[stanisc, 2015-10-13 21:23:12]

https://codereview.chromium.org/1393633003/diff/60001/sync/engine/apply_contr...
File sync/engine/apply_control_data_updates_unittest.cc (right):

https://codereview.chromium.org/1393633003/diff/60001/sync/engine/apply_contr...
sync/engine/apply_control_data_updates_unittest.cc:942:
EXPECT_FALSE(directory()->InitialSyncEndedForType(NIGORI));
On 2015/10/13 17:51:37, Nicolas Zea wrote:
> Did you mean to leave this in? (maybe comment why?)

This was to verify that there is no side effect on other control types.
Added a comment.

https://codereview.chromium.org/1393633003/diff/60001/sync/engine/syncer_unit...
File sync/engine/syncer_unittest.cc (right):

https://codereview.chromium.org/1393633003/diff/60001/sync/engine/syncer_unit...
sync/engine/syncer_unittest.cc:4711: TEST_F(SyncerTest,
ConfigureFailsDontApplyUpdates) {
On 2015/10/13 17:51:37, Nicolas Zea wrote:
> Should this be updated? AFAICT you're now actually applying updates, and
> successfully configuring.
> 
> Maybe pull your logic into a separate test?

Good point. I removed the extra sync cycle from this test and instead updated
the test above - the happy case.

https://codereview.chromium.org/1393633003/diff/60001/sync/engine/syncer_unit...
sync/engine/syncer_unittest.cc:4787: TEST_F(SyncerTest,
ProgressMarkerOnlyUpdate) {
On 2015/10/13 17:51:37, Nicolas Zea wrote:
> This is to test the the implicit permanent folder is created right? Maybe add
a
> comment about that?

Right. Added a comment. Also added a check that verifies that the root folder
was created.

[stanisc, 2015-10-13 23:09:31]

The CQ bit was checked by stanisc@chromium.org

[stanisc, 2015-10-13 23:09:32]

The patchset sent to the CQ was uploaded after l-g-t-m from zea@chromium.org
Link to the patchset: https://codereview.chromium.org/1393633003/#ps80001
(title: "Addressed CR feedback")

[commit-bot: I haz the power, 2015-10-13 23:10:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1393633003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1393633003/80001

[commit-bot: I haz the power, 2015-10-13 23:57:27]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2015-10-13 23:58:32]

Patchset 4 (id:??) landed as
https://crrev.com/b691b7a0f29e7ebafad5fadb8a6d59cce8cff279
Cr-Commit-Position: refs/heads/master@{#353913}