Remove dependency on server generated type root folders

sync/syncable/directory.cc

Index: sync/syncable/directory.cc
diff --git a/sync/syncable/directory.cc b/sync/syncable/directory.cc
index 68774df644bc94e5b8d25b9a341d245cfd55143f..7fa6bc68d2c6f55c8093735ea3e1ab34d4048a44 100644
--- a/sync/syncable/directory.cc
+++ b/sync/syncable/directory.cc
@@ -748,7 +748,7 @@ bool Directory::PurgeEntriesWithTypeIn(ModelTypeSet disabled_types,
       bool found_progress = false;
       for (ModelTypeSet::Iterator iter = disabled_types.First(); iter.Good();
            iter.Inc()) {
-        if (!kernel_->persisted_info.HasEmptyDownloadProgress(iter.Get()))
+        if (!HasEmptyDownloadProgress(iter.Get()))
           found_progress = true;
       }
 
@@ -925,6 +925,10 @@ void Directory::SetDownloadProgress(
   kernel_->info_status = KERNEL_SHARE_INFO_DIRTY;
 }
 
+bool Directory::HasEmptyDownloadProgress(ModelType type) const {
+  return kernel_->persisted_info.HasEmptyDownloadProgress(type);

[maniscalco, 2015/01/24 00:55:59]

Normally, when Directory methods access kernel_ they either take a ScopedLock
(like GetEntriesCount) or they assert the thread already has a lock (like
GetTransactionVersion).  Should this new method follow one of those approaches?

[stanisc, 2015/01/26 19:02:09]

Done.

[maniscalco, 2015/01/26 19:17:34]

Since Directory::HasEmptyDownloadProgress is called from PurgeEntriesWithTypeIn
while a ScopedKernelLock on "this" is held, I'm thinking
Directory::HasEmptyDownloadProgress shouldn't take a lock because SKLs are
non-recursive, right?  As is, I would think the code might DCHECK:

https://code.google.com/p/chromium/codesearch#chromium/src/base/synchronizati...

Unless, maybe I'm missing something?

Thinking more about this... WDYT about adding const ScopedKernelLock& to
Directory::HasEmptyDownloadProgress ?  We do that elsewhere in Directory to
ensure the caller is holding a lock (e.g. ClearDirtyMetahandles).  That way,
it's harder to misuse.

[stanisc, 2015/01/29 00:27:37]

Directory::HasEmptyDownloadProgress isn't called from PurgeEntriesWithTypeIn
anymore. I changed that code back to calling
Directory::PersistedKernelInfo::HasEmptyDownloadProgress directly to avoid
exactly the problem you described. I did it at the same time when I added the
lock to Directory::HasEmptyDownloadProgress. In general none of functions in
PersistedKernelInfo get ScopedKernelLock& as one of parameters. I didn't want to
change the existing pattern so I decided it would be better to inline the call
in PurgeEntriesWithTypeIn instead.

[maniscalco, 2015/01/29 00:36:34]

Ah, you're right.  I misread which HasEmptyDownloadProgress was being called
(Directory's vs Directory::PersistedKernelInfo's).

+}
+
 int64 Directory::GetTransactionVersion(ModelType type) const {
   kernel_->transaction_mutex.AssertAcquired();
   return kernel_->persisted_info.transaction_version[type];
@@ -951,6 +955,7 @@ void Directory::SetDataTypeContext(
   kernel_->info_status = KERNEL_SHARE_INFO_DIRTY;
 }
 
+// TODO: change these to not rely on the folders
 ModelTypeSet Directory::InitialSyncEndedTypes() {
   syncable::ReadTransaction trans(FROM_HERE, this);
   ModelTypeSet protocol_types = ProtocolTypes();
@@ -1175,8 +1180,7 @@ bool Directory::CheckTreeInvariants(syncable::BaseTransaction* trans,
                       "Entry should be root",
                       trans))
          return false;
-      if (!SyncAssert(!e.GetIsUnsynced(), FROM_HERE,
-                      "Entry should be sycned",
+      if (!SyncAssert(!e.GetIsUnsynced(), FROM_HERE, "Entry should be synced",
                       trans))
          return false;
       continue;
@@ -1194,7 +1198,6 @@ bool Directory::CheckTreeInvariants(syncable::BaseTransaction* trans,
 
       if (!parentid.IsNull()) {
         int safety_count = handles.size() + 1;
-        // TODO(stanisc): handle items with Null parentid
         while (!parentid.IsRoot()) {
           Entry parent(trans, GET_BY_ID, parentid);
           if (!SyncAssert(parent.good(), FROM_HERE,
@@ -1222,13 +1225,16 @@ bool Directory::CheckTreeInvariants(syncable::BaseTransaction* trans,
     int64 base_version = e.GetBaseVersion();
     int64 server_version = e.GetServerVersion();
     bool using_unique_client_tag = !e.GetUniqueClientTag().empty();
+    bool is_type_root_folder =
+        parentid.IsRoot() &&
+        e.GetUniqueServerTag() == ModelTypeToRootTag(e.GetModelType());
     if (CHANGES_VERSION == base_version || 0 == base_version) {
       if (e.GetIsUnappliedUpdate()) {
         // Must be a new item, or a de-duplicated unique client tag
         // that was created both locally and remotely.
-        if (!using_unique_client_tag) {
+        if (!(using_unique_client_tag || is_type_root_folder)) {
           if (!SyncAssert(e.GetIsDel(), FROM_HERE,
-                          "The entry should not have been deleted.",
+                          "The entry should have been deleted.",
                           trans))
             return false;
         }
@@ -1246,12 +1252,26 @@ bool Directory::CheckTreeInvariants(syncable::BaseTransaction* trans,
                           trans))
             return false;
         }
-        // Should be an uncomitted item, or a successfully deleted one.
-        if (!e.GetIsDel()) {
-          if (!SyncAssert(e.GetIsUnsynced(), FROM_HERE,
-                          "The item should be unsynced.",
-                          trans))
+        if (is_type_root_folder) {
+          // This must be a locally created type root folder
+          if (!SyncAssert(
+                  !e.GetIsUnsynced(), FROM_HERE,
+                  "Locally created type root folders should not be unsynced.",
+                  trans))
             return false;
+
+          if (!SyncAssert(
+                  !e.GetIsDel(), FROM_HERE,
+                  "Locally created type root folders should not be deleted.",
+                  trans))
+            return false;
+        } else {
+          // Should be an uncomitted item, or a successfully deleted one.
+          if (!e.GetIsDel()) {
+            if (!SyncAssert(e.GetIsUnsynced(), FROM_HERE,
+                            "The item should be unsynced.", trans))
+              return false;
+          }
         }
         // If the next check failed, it would imply that an item exists
         // on the server, isn't waiting for application locally, but either
@@ -1463,7 +1483,6 @@ void Directory::AppendChildHandles(const ScopedKernelLock& lock,
 
   for (OrderedChildSet::const_iterator i = children->begin();
        i != children->end(); ++i) {
-    DCHECK_EQ(parent_id, (*i)->ref(PARENT_ID));
     result->push_back((*i)->ref(META_HANDLE));
   }
 }