[Sync] Start implementation of migration for USS.

components/sync/engine_impl/uss_migrator.cc

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/sync/engine_impl/uss_migrator.h"
+
+#include <memory>
+#include <string>
+#include <utility>
+#include <vector>
+
+#include "base/memory/ptr_util.h"
+#include "components/sync/base/time.h"
+#include "components/sync/engine_impl/model_type_worker.h"
+#include "components/sync/protocol/sync.pb.h"
+#include "components/sync/syncable/directory.h"
+#include "components/sync/syncable/entry.h"
+#include "components/sync/syncable/read_node.h"
+#include "components/sync/syncable/read_transaction.h"
+#include "components/sync/syncable/user_share.h"
+
+namespace syncer {
+
+namespace {
+
+bool ExtractSyncEntity(ReadTransaction* trans,

[skym, 2016/10/21 15:34:20]

Should this be a const ref?

[maxbogue, 2016/10/24 19:02:18]

ReadNode takes a non-const transaction pointer, so no.

+                       int64_t id,
+                       sync_pb::SyncEntity* entity) {
+  ReadNode read_node(trans);
+  if (read_node.InitByIdLookup(id) != BaseNode::INIT_OK)

[skym, 2016/10/21 15:34:20]

So why could this fail? How does this handle tombstones? How does this handle
encrypted data? Is it possible the directory could be hanging onto some
unapplied data that has not reached the model yet?

[maxbogue, 2016/10/24 19:02:18]

This line is just a failure to look up data. The rest of your comments probably
apply below. Tombstones should just have IsDel set to true. Encrypted data...
I'm not sure whether it's still encrypted or not in the specifics I'm getting,
but it also shouldn't matter right? The worker can decrypt. If data was
unapplied we still want to pass it along here, so the USS merge can apply it.

[skym, 2016/10/25 15:48:12]

I guess what I was suggesting, poorly, was lets look at
https://cs.chromium.org/chromium/src/components/sync/syncable/write_node.cc?q...
and think about the reasons we can get into != BaseNode::INIT_OK

+    return false;
+
+  const syncable::Entry& entry = *read_node.GetEntry();
+
+  entity->set_id_string(entry.GetId().GetServerId());
+  entity->set_version(entry.GetServerVersion());
+  entity->set_mtime(TimeToProtoTime(entry.GetServerMtime()));
+  entity->set_ctime(TimeToProtoTime(entry.GetServerCtime()));
+  entity->set_name(entry.GetServerNonUniqueName());
+  entity->set_deleted(entry.GetServerIsDel());
+  entity->set_client_defined_unique_tag(entry.GetUniqueClientTag());

[skym, 2016/10/21 15:34:20]

There's a lot of fields in Entry and/or SyncEntity that you're not copying (I'm
assuming since the worker doesn't read them). Can you put a comment in here
explaining this?

[maxbogue, 2016/10/24 19:02:18]

Done.

+
+  // It looks like there are fancy other ways to get e.g. passwords specifics
+  // out of Entry. Do we need to special-case them when we ship those types?

[skym, 2016/10/21 15:34:20]

DCHECK not passwords?

[maxbogue, 2016/10/24 19:02:19]

Done in the main function.

+  entity->mutable_specifics()->CopyFrom(entry.GetSpecifics());

[skym, 2016/10/21 15:34:20]

Can you add a comment for why not ServerSpecifics or BaseServerSpecifics?

Also, when we've enabled passphrase encryption, we still store them unencrypted
on disk (directory), right?

[maxbogue, 2016/10/24 19:02:18]

Mmm... I think I mean to do server specifics here, whoops.

+  return true;
+}
+
+}  // namespace
+
+bool MigrateDirectoryData(ModelType type,
+                          UserShare* user_share,
+                          ModelTypeWorker* worker,
+                          int batch_size) {
+  std::string type_name = ModelTypeToString(type);
+  ReadTransaction trans(FROM_HERE, user_share);

[skym, 2016/10/21 15:34:20]

So what's the difference between syncer::syncable::ReadTransaction and
syncer::ReadTransaction? I don't get it.

[maxbogue, 2016/10/24 19:02:18]

¯\_(ツ)_/¯ I also do not quite get it. It might just be a layering thing that my
reorganization has completely outdated.

+
+  ReadNode root(&trans);
+  if (root.InitTypeRoot(type) != BaseNode::INIT_OK) {
+    LOG(ERROR) << "Missing root node for " << type_name << ".";

[skym, 2016/10/21 15:34:20]

I would drop the trailing period, don't need correct sentences in logging
messages. Also, having a type_name variable kind of increases mental load
reading this method. Might want to consider in-lining it in both places it is
used.

[maxbogue, 2016/10/24 19:02:18]

Done.

+    return false;
+  }
+
+  // Get the progress marker and context from the directory.
+  sync_pb::DataTypeProgressMarker progress;
+  sync_pb::DataTypeContext context;
+  user_share->directory->GetDownloadProgress(type, &progress);

[skym, 2016/10/21 15:34:20]

These can't fail? How does ScopdKernelLock interact with transactions? And why
does GetDataTypeContext take a trans, but GetDownloadedProgress doesn't?

[maxbogue, 2016/10/24 19:02:18]

They don't seem to fail, no. It seems to interact fine since it works; I don't
know exactly though. And I don't know; it looks like GetDataTypeContext
literally doesn't use the transaction:
https://cs.chromium.org/chromium/src/components/sync/syncable/directory.cc?l=912

+  user_share->directory->GetDataTypeContext(trans.GetWrappedTrans(), type,
+                                            &context);
+
+  std::vector<int64_t> child_ids;
+  root.GetChildIds(&child_ids);

[skym, 2016/10/21 15:34:20]

DCHECK not bookmarks? Does look like this is handling more than one layer of
children.

[maxbogue, 2016/10/24 19:02:18]

Done.

+
+  size_t i = 0;
+  while (i < child_ids.size()) {
+    // Vector to own the temporary entities.
+    std::vector<std::unique_ptr<sync_pb::SyncEntity>> entities;
+    // Vector of raw pointers for passing to ProcessGetUpdatesResponse().
+    SyncEntityList entity_ptrs;
+
+    // Process |batch_size| entities at a time to reduce memory usage.

[skym, 2016/10/21 15:34:20]

I think this is the wrong place for this comment.

[maxbogue, 2016/10/24 19:02:18]

Moved above the loop... hopefully that's better?

+    const size_t batch_limit = i + batch_size;
+    while (i < batch_limit && i < child_ids.size()) {

[skym, 2016/10/21 15:34:20]

Can you use 'for' here? I think it'd make it more clear what's going on. With
two while loops that iterate over essentially the same thing, things are
confusing who is actually iterating until you get to the i++ part. I think
having that be higher/more obvious would be good. Also maybe min?

for (; i < std::min(batch_limit, child_ids.size()); i++) {

[maxbogue, 2016/10/24 19:02:18]

Done. Also I just did the min to set batch_limit :P

+      auto entity = base::MakeUnique<sync_pb::SyncEntity>();
+      if (!ExtractSyncEntity(&trans, child_ids[i], entity.get())) {
+        LOG(ERROR) << "Failed to fetch child node for " << type_name << ".";
+        return false;

[skym, 2016/10/21 15:34:20]

Can you comment about why you don't need to strip any data out of the worker. It
is potentiality filled with lots of partial directory data. And then we're going
to perform an initial download.

[maxbogue, 2016/10/24 19:02:18]

I've added an AbortMigration method to the worker that's called in the failure
cases here.

+      }
+      entity_ptrs.push_back(entity.get());
+      entities.push_back(std::move(entity));
+      i++;
+    }
+
+    worker->ProcessGetUpdatesResponse(progress, context, entity_ptrs, nullptr);

[skym, 2016/10/21 15:34:20]

It is kind of odd that we update progress/context every time.

[maxbogue, 2016/10/24 19:02:18]

Sure is... I don't really feel comfortable changing the interface to fix it
though ¯\_(ツ)_/¯

[skym, 2016/10/25 15:48:12]

That's reasonable.

[maxbogue, 2016/10/26 23:26:15]

Acknowledged.

+  }
+
+  worker->PassiveApplyUpdates(nullptr);

[skym, 2016/10/21 15:34:20]

I'm worried about all the different places responsible for nudging/applying
initial updates to the worker.

[maxbogue, 2016/10/24 19:02:18]

All the places? There's just this place and the normal place, isn't there?

[skym, 2016/10/25 15:48:12]

And on migration failure in the worker.

[maxbogue, 2016/10/26 23:26:15]

Ah right, we nudge there. That's a different problem because the nudge doesn't
do anything, so I think for now it's fine to leave it be.

+  return true;
+}
+
+}  // namespace syncer