Simplify implementation of FindEntryByPathSync

chrome/browser/chromeos/gdata/gdata_files.cc

 // Copyright (c) 2012 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 "chrome/browser/chromeos/gdata/gdata_files.h"
 
 #include <leveldb/db.h>
 #include <vector>
 
 #include "base/message_loop_proxy.h"
@@ skipping 332 matching lines @@
 
   for (GDataDirectoryCollection::iterator iter =
       dir->child_directories_.begin();
        iter != dir->child_directories_.end(); ++iter) {
     AddEntry(iter->second);
   }
   dir->child_directories_.clear();
   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(
     const FilePath::StringType& file_name) const {
   GDataFileCollection::const_iterator it = child_files_.find(file_name);
   if (it != child_files_.end())
     return it->second;
 
   GDataDirectoryCollection::const_iterator itd =
       child_directories_.find(file_name);
   if (itd != child_directories_.end())
     return itd->second;
 
   return NULL;
 }
 
 void GDataDirectory::AddChild(GDataEntry* entry) {
   DCHECK(entry);
 
   GDataFile* file = entry->AsGDataFile();
   if (file)
     child_files_.insert(std::make_pair(entry->base_name(), file));
 
   GDataDirectory* directory = entry->AsGDataDirectory();
   if (directory)
     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);
 
   // Then delete it from tree.
   child_files_.erase(file_name);
   child_directories_.erase(file_name);
-
-  return true;
 }
 
 void GDataDirectory::RemoveChildren() {
   RemoveChildFiles();
   RemoveChildDirectories();
 }
 
 void GDataDirectory::RemoveChildFiles() {
   for (GDataFileCollection::const_iterator iter = child_files_.begin();
        iter != child_files_.end(); ++iter) {
@@ skipping 194 matching lines @@
   resource_map_.insert(std::make_pair(entry->resource_id(), entry));
 }
 
 void GDataDirectoryService::RemoveEntryFromResourceMap(GDataEntry* entry) {
   // GDataFileSystem has already locked.
   resource_map_.erase(entry->resource_id());
 }
 
 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();
 
-  GDataDirectory* current_dir = root_.get();
-  FilePath directory_path;
+  for (size_t i = 1; i < components.size() && current_dir; ++i) {
+    GDataEntry* entry = current_dir->FindChild(components[i]);
+    if (!entry)
+      return NULL;
 
-  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) {
-      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;
 }
 
 void GDataDirectoryService::FindEntryByPathAndRunSync(
     const FilePath& search_file_path,
     const FindEntryCallback& callback) {
   GDataEntry* entry = FindEntryByPathSync(search_file_path);
   callback.Run(entry ? GDATA_FILE_OK : GDATA_FILE_ERROR_NOT_FOUND, entry);
 }
 
@@ skipping 431 matching lines @@
     if (file->FromProto(entry_proto)) {
       entry.reset(file.release());
     } else {
       NOTREACHED() << "FromProto (file) failed";
     }
   }
   return entry.Pass();
 }
 
 }  // namespace gdata