Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(35)

Unified Diff: sync/syncable/parent_child_index.cc

Issue 1226213002: Sync: Support nodes with implicit permanent folders: Node Browser and out-of-order loading from DB (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master
Patch Set: Changed comments Created 5 years, 5 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « sync/syncable/parent_child_index.h ('k') | sync/syncable/parent_child_index_unittest.cc » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: sync/syncable/parent_child_index.cc
diff --git a/sync/syncable/parent_child_index.cc b/sync/syncable/parent_child_index.cc
index 0cab1d3942eb47b0bc1d1ebb8215633deef4492b..b1b493e36f8c4cc25a4897d9447a2ced1630c452 100644
--- a/sync/syncable/parent_child_index.cc
+++ b/sync/syncable/parent_child_index.cc
@@ -40,76 +40,135 @@ bool ChildComparator::operator()(const EntryKernel* a,
}
ParentChildIndex::ParentChildIndex() {
+ // Pre-allocate these two vectors to the number of model types.
+ model_type_root_ids_.resize(MODEL_TYPE_COUNT);
+ type_root_child_sets_.resize(MODEL_TYPE_COUNT);
}
ParentChildIndex::~ParentChildIndex() {
- STLDeleteContainerPairSecondPointers(
- parent_children_map_.begin(), parent_children_map_.end());
+ for (int i = 0; i < MODEL_TYPE_COUNT; i++) {
+ // Normally all OrderedChildSet instances in |type_root_child_sets_|
+ // are shared with |parent_children_map_|. Null out shared instances and
+ // ScopedVector will take care of the ones that are not shared (if any).
+ if (!model_type_root_ids_[i].IsNull()) {
+ DCHECK_EQ(type_root_child_sets_[i],
+ parent_children_map_.find(model_type_root_ids_[i])->second);
+ type_root_child_sets_[i] = nullptr;
+ }
+ }
+
+ STLDeleteContainerPairSecondPointers(parent_children_map_.begin(),
+ parent_children_map_.end());
}
bool ParentChildIndex::ShouldInclude(const EntryKernel* entry) {
// This index excludes deleted items and the root item. The root
// item is excluded so that it doesn't show up as a child of itself.
- return !entry->ref(IS_DEL) && !entry->ref(ID).IsRoot();
+ // The index also excludes items without client data e.g. the items that
+ // don't have client SPECIFICS or PARENT_ID. There is no need to include
+ // server-only entries because they shouldn't show up on the client until
+ // they're applied.
+ return !entry->ref(IS_DEL) && !entry->ref(ID).IsRoot() &&
+ (entry->GetModelType() != UNSPECIFIED ||
+ !entry->ref(PARENT_ID).IsNull());
}
bool ParentChildIndex::Insert(EntryKernel* entry) {
DCHECK(ShouldInclude(entry));
- const Id& parent_id = GetParentId(entry);
- // Store type root ID when inserting a type root entry.
- if (parent_id.IsRoot()) {
- ModelType model_type = GetModelType(entry);
- // TODO(stanisc): there are some unit tests that create entries
- // at the root and don't bother initializing specifics which
- // produces TOP_LEVEL_FOLDER type here.
- if (syncer::IsRealDataType(model_type)) {
- model_type_root_ids_[model_type] = entry->ref(ID);
+ OrderedChildSet* siblings = nullptr;
+ const Id& parent_id = entry->ref(PARENT_ID);
+ ModelType model_type = entry->GetModelType();
+
+ if (ShouldUseParentId(parent_id, model_type)) {
+ // Hierarchical type, lookup child set in the map.
+ DCHECK(!parent_id.IsNull());
+ ParentChildrenMap::iterator it = parent_children_map_.find(parent_id);
+ if (it != parent_children_map_.end()) {
+ siblings = it->second;
+ } else {
+ siblings = new OrderedChildSet();
+ parent_children_map_.insert(std::make_pair(parent_id, siblings));
}
+ } else {
+ // Non-hierarchical type, return a pre-defined collection by type.
+ siblings = GetOrCreateModelTypeChildSet(model_type);
}
- OrderedChildSet* children = NULL;
- ParentChildrenMap::iterator i = parent_children_map_.find(parent_id);
- if (i != parent_children_map_.end()) {
- children = i->second;
- } else {
- children = new OrderedChildSet();
- parent_children_map_.insert(std::make_pair(parent_id, children));
+ // If this is one of type root folder for a non-hierarchical type, associate
+ // its ID with the model type and the type's pre-defined child set with the
+ // type root ID.
+ // TODO(stanisc): crbug/438313: Just TypeSupportsHierarchy condition should
+ // theoretically be sufficient but in practice many tests don't properly
+ // initialize entries so TypeSupportsHierarchy ends up failing. Consider
+ // tweaking TypeSupportsHierarchy and fixing all related test code.
+ if (parent_id.IsRoot() && entry->ref(IS_DIR) &&
+ syncer::IsRealDataType(model_type) &&
+ !TypeSupportsHierarchy(model_type)) {
+ const Id& type_root_id = entry->ref(ID);
+ // Disassociate the type root child set with the previous ID if any.
+ const Id& prev_type_root_id = model_type_root_ids_[model_type];
+ if (!prev_type_root_id.IsNull()) {
+ ParentChildrenMap::iterator it =
+ parent_children_map_.find(prev_type_root_id);
+ if (it != parent_children_map_.end()) {
+ // If the entry exists in the map it must already have the same
+ // model type specific child set. This child set will be re-inserted
+ // in the map under the new ID below so it is safe to simply erase
+ // the entry here.
+ DCHECK_EQ(it->second, GetModelTypeChildSet(model_type));
+ parent_children_map_.erase(it);
+ }
+ }
+ // Store the new type root ID and associate the child set.
+ // Note that the child set isn't owned by the map in this case.
+ model_type_root_ids_[model_type] = type_root_id;
+ parent_children_map_.insert(
+ std::make_pair(type_root_id, GetOrCreateModelTypeChildSet(model_type)));
}
- return children->insert(entry).second;
+ // Finally, insert the entry in the child set.
+ return siblings->insert(entry).second;
}
// Like the other containers used to help support the syncable::Directory, this
// one does not own any EntryKernels. This function removes references to the
// given EntryKernel but does not delete it.
void ParentChildIndex::Remove(EntryKernel* e) {
- const Id& parent_id = GetParentId(e);
-
- ParentChildrenMap::iterator parent = parent_children_map_.find(parent_id);
- DCHECK(parent != parent_children_map_.end());
+ OrderedChildSet* siblings = nullptr;
+ ModelType model_type = e->GetModelType();
+ const Id& parent_id = e->ref(PARENT_ID);
+ bool should_erase = false;
+ ParentChildrenMap::iterator it;
+
+ if (ShouldUseParentId(parent_id, model_type)) {
+ // Hierarchical type, lookup child set in the map.
+ DCHECK(!parent_id.IsNull());
+ it = parent_children_map_.find(parent_id);
+ DCHECK(it != parent_children_map_.end());
+ siblings = it->second;
+ should_erase = true;
+ } else {
+ // Non-hierarchical type, return a pre-defined child set by type.
+ siblings = type_root_child_sets_[model_type];
+ }
- OrderedChildSet* children = parent->second;
- OrderedChildSet::iterator j = children->find(e);
- DCHECK(j != children->end());
+ OrderedChildSet::iterator j = siblings->find(e);
+ DCHECK(j != siblings->end());
- children->erase(j);
- if (children->empty()) {
- delete children;
- parent_children_map_.erase(parent);
+ // Erase the entry from the child set.
+ siblings->erase(j);
+ // If the set is now empty and isn't shareable with |type_root_child_sets_|,
+ // erase it from the map.
+ if (siblings->empty() && should_erase) {
+ delete siblings;
+ parent_children_map_.erase(it);
}
}
bool ParentChildIndex::Contains(EntryKernel *e) const {
- const Id& parent_id = GetParentId(e);
- ParentChildrenMap::const_iterator parent =
- parent_children_map_.find(parent_id);
- if (parent == parent_children_map_.end()) {
- return false;
- }
- const OrderedChildSet* children = parent->second;
- DCHECK(children && !children->empty());
- return children->count(e) > 0;
+ const OrderedChildSet* siblings = GetChildSet(e);
+ return siblings && siblings->count(e) > 0;
}
const OrderedChildSet* ParentChildIndex::GetChildren(const Id& id) const {
@@ -120,9 +179,12 @@ const OrderedChildSet* ParentChildIndex::GetChildren(const Id& id) const {
return nullptr;
}
- // A successful lookup implies at least some children exist.
- DCHECK(!parent->second->empty());
- return parent->second;
+ OrderedChildSet* children = parent->second;
+ // The expectation is that the function returns nullptr instead of an empty
+ // child set
+ if (children && children->empty())
+ children = nullptr;
+ return children;
}
const OrderedChildSet* ParentChildIndex::GetChildren(EntryKernel* e) const {
@@ -130,32 +192,52 @@ const OrderedChildSet* ParentChildIndex::GetChildren(EntryKernel* e) const {
}
const OrderedChildSet* ParentChildIndex::GetSiblings(EntryKernel* e) const {
+ // This implies the entry is in the index.
DCHECK(Contains(e));
- const OrderedChildSet* siblings = GetChildren(GetParentId(e));
+ const OrderedChildSet* siblings = GetChildSet(e);
DCHECK(siblings && !siblings->empty());
return siblings;
}
-const Id& ParentChildIndex::GetParentId(EntryKernel* e) const {
- const Id& parent_id = e->ref(PARENT_ID);
- if (!parent_id.IsNull()) {
- return parent_id;
- }
- return GetModelTypeRootId(GetModelType(e));
+/* static */
+bool ParentChildIndex::ShouldUseParentId(const Id& parent_id,
+ ModelType model_type) {
+ // For compatibility with legacy unit tests, in addition to hierarchical
+ // entries, this returns true any entries directly under root and for entries
+ // of UNSPECIFIED model type.
+ return parent_id.IsRoot() || TypeSupportsHierarchy(model_type) ||
+ !syncer::IsRealDataType(model_type);
}
-ModelType ParentChildIndex::GetModelType(EntryKernel* e) {
- // TODO(stanisc): is there a more effective way to find out model type?
+const OrderedChildSet* ParentChildIndex::GetChildSet(EntryKernel* e) const {
ModelType model_type = e->GetModelType();
- if (!syncer::IsRealDataType(model_type)) {
- model_type = e->GetServerModelType();
+
+ const Id& parent_id = e->ref(PARENT_ID);
+ if (ShouldUseParentId(parent_id, model_type)) {
+ // Hierarchical type, lookup child set in the map.
+ ParentChildrenMap::const_iterator it = parent_children_map_.find(parent_id);
+ if (it == parent_children_map_.end())
+ return nullptr;
+ return it->second;
}
- return model_type;
+
+ // Non-hierarchical type, return a collection indexed by type.
+ return GetModelTypeChildSet(model_type);
+}
+
+const OrderedChildSet* ParentChildIndex::GetModelTypeChildSet(
+ ModelType model_type) const {
+ return type_root_child_sets_[model_type];
+}
+
+OrderedChildSet* ParentChildIndex::GetOrCreateModelTypeChildSet(
+ ModelType model_type) {
+ if (!type_root_child_sets_[model_type])
+ type_root_child_sets_[model_type] = new OrderedChildSet();
+ return type_root_child_sets_[model_type];
}
const Id& ParentChildIndex::GetModelTypeRootId(ModelType model_type) const {
- // TODO(stanisc): Review whether this approach is reliable enough.
- // Should this method simply enumerate children of root node ("r") instead?
return model_type_root_ids_[model_type];
}
« no previous file with comments | « sync/syncable/parent_child_index.h ('k') | sync/syncable/parent_child_index_unittest.cc » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698