Simplify implementation of FindEntryByPathSync

chrome/browser/chromeos/gdata/gdata_files.cc

Index: chrome/browser/chromeos/gdata/gdata_files.cc
===================================================================
--- chrome/browser/chromeos/gdata/gdata_files.cc	(revision 150246)
+++ chrome/browser/chromeos/gdata/gdata_files.cc	(working copy)
@@ -350,14 +350,11 @@
   return true;
 }
 
-bool GDataDirectory::RemoveEntry(GDataEntry* entry) {
+void GDataDirectory::RemoveEntry(GDataEntry* entry) {
   DCHECK(entry);
 
-  if (!RemoveChild(entry))
-    return false;
-
+  RemoveChild(entry);

[satorux1, 2012/08/07 22:38:09]

The new behavior may be rather scary. Previously if |entry| is not found in this
directory, this function did nothing but returned false. With the new behavior,
|entry| is deleted regardless. 

I guess we want to keep the original behavior just in case.

[achuithb, 2012/08/07 22:48:35]

We're using this function in 2 places in GDataFileSystem and 1 place in
GDataDirectoryService. In all cases, we do a parent test, so we are effectively
doing something like
entry->parent()->RemoveEntry(entry);

I believe the return false case is a dead code path, and I think we should get
rid of it.

This code is not going to get merged into the branch, so even though this is
potentially risky, I think it's better due to its simplicity.

   delete entry;
-  return true;
 }
 
 GDataEntry* GDataDirectory::FindChild(
@@ -386,16 +383,10 @@
     child_directories_.insert(std::make_pair(entry->base_name(), directory));
 }
 
-bool GDataDirectory::RemoveChild(GDataEntry* entry) {
+void GDataDirectory::RemoveChild(GDataEntry* entry) {
   DCHECK(entry);
 
-  const std::string file_name(entry->base_name());
-  GDataEntry* found_entry = FindChild(file_name);
-  if (!found_entry)
-    return false;
-
-  DCHECK_EQ(entry, found_entry);
-
+  const std::string& file_name(entry->base_name());

[satorux1, 2012/08/07 22:27:05]

file_name -> base_name ?

[achuithb, 2012/08/07 22:29:43]

Done.

   // Remove entry from resource map first.
   if (directory_service_)
     directory_service_->RemoveEntryFromResourceMap(entry);
@@ -403,8 +394,6 @@
   // Then delete it from tree.
   child_files_.erase(file_name);
   child_directories_.erase(file_name);
-
-  return true;
 }
 
 void GDataDirectory::RemoveChildren() {
@@ -619,39 +608,22 @@
 
 GDataEntry* GDataDirectoryService::FindEntryByPathSync(
     const FilePath& file_path) {
+  if (file_path == root_->GetFilePath())
+    return root_.get();
+
   std::vector<FilePath::StringType> components;
   file_path.GetComponents(&components);
-
   GDataDirectory* current_dir = root_.get();
-  FilePath directory_path;
 
-  for (size_t i = 0; i < components.size() && current_dir; i++) {
-    directory_path = directory_path.Append(current_dir->base_name());
-
-    // Last element must match, if not last then it must be a directory.
-    if (i == components.size() - 1) {
-      if (current_dir->base_name() == components[i])
-        return current_dir;
-      else
-        return NULL;
-    }
-
-    // Not the last part of the path, search for the next segment.
-    GDataEntry* entry = current_dir->FindChild(components[i + 1]);
-    if (!entry) {
+  for (size_t i = 1; i < components.size() && current_dir; ++i) {
+    GDataEntry* entry = current_dir->FindChild(components[i]);
+    if (!entry)
       return NULL;
-    }
 
-    // Found file, must be the last segment.
-    if (entry->file_info().is_directory) {
-      // Found directory, continue traversal.
+    if (i == components.size() - 1)  // Last component.
+      return entry;
+    else
       current_dir = entry->AsGDataDirectory();
-    } else {
-      if ((i + 1) == (components.size() - 1))
-        return entry;
-      else
-        return NULL;
-    }
   }

[satorux1, 2012/08/07 22:27:05]

Much simpler! Nice.

   return NULL;
 }