Move DirectoryReader::ReadEntries to FileRef::ReadDirectoryEntries

ppapi/tests/test_file_ref.cc

Index: ppapi/tests/test_file_ref.cc
diff --git a/ppapi/tests/test_file_ref.cc b/ppapi/tests/test_file_ref.cc
index 4aab3d67b21c054205481725961a481947d5a8cd..9c94b8eda9a995ece18976e0ec859a9149ba70f5 100644
--- a/ppapi/tests/test_file_ref.cc
+++ b/ppapi/tests/test_file_ref.cc
@@ -10,8 +10,7 @@
 #include "ppapi/c/pp_errors.h"
 #include "ppapi/c/ppb_file_io.h"
 #include "ppapi/c/dev/ppb_testing_dev.h"
-#include "ppapi/cpp/dev/directory_entry_dev.h"
-#include "ppapi/cpp/dev/directory_reader_dev.h"
+#include "ppapi/cpp/directory_entry.h"
 #include "ppapi/cpp/file_io.h"
 #include "ppapi/cpp/file_ref.h"
 #include "ppapi/cpp/file_system.h"
@@ -34,6 +33,8 @@ const char* kPersFilePath = "/foo/bar/persistent";
 const char* kTempFilePath = "/foo/bar/temporary";
 const char* kTerribleName = "!@#$%^&*()-_=+{}[] ;:'\"|`~\t\n\r\b?";
 
+typedef std::vector<pp::DirectoryEntry> DirEntries;
+
 std::string ReportMismatch(const std::string& method_name,
                            const std::string& returned_result,
                            const std::string& expected_result) {
@@ -41,6 +42,12 @@ std::string ReportMismatch(const std::string& method_name,
       expected_result + "' expected.";
 }
 
+std::string IntegerToString(int value) {
+  char result[12];
+  sprintf(result, "%d", value);

[dmichael (off chromium), 2013/05/02 19:43:54]

nit: please use snprintf (or just delete this function; see below)

[hamaji, 2013/05/02 20:14:49]

Removed

+  return result;
+}
+
 }  // namespace
 
 bool TestFileRef::Init() {
@@ -68,6 +75,39 @@ std::string TestFileRef::MakeExternalFileRef(pp::FileRef* file_ref_ext) {
   PASS();
 }
 
+int32_t TestFileRef::DeleteDirectoryRecursively(pp::FileRef* dir) {
+  if (!dir)
+    return PP_ERROR_BADARGUMENT;
+
+  TestCompletionCallback callback(instance_->pp_instance(), callback_type());
+  TestCompletionCallbackWithOutput<DirEntries> output_callback(
+      instance_->pp_instance(), callback_type());
+
+  output_callback.WaitForResult(
+      dir->ReadEntries(output_callback.GetCallback()));
+  int32_t rv = output_callback.result();
+  if (rv != PP_OK && rv != PP_ERROR_FILENOTFOUND)
+    return rv;
+
+  DirEntries entries = output_callback.output();
+  for (DirEntries::const_iterator it = entries.begin();
+       it != entries.end(); ++it) {

[dmichael (off chromium), 2013/05/02 19:43:54]

nit: I prefer each statement in the loop have its own line if they don't fit on
1

[hamaji, 2013/05/02 20:14:49]

Yeah, I actually have the same preference :) (as you might know, I just copied
most code from test_directory_reader.cc)

+    pp::FileRef file_ref = it->file_ref();
+    if (it->file_type() == PP_FILETYPE_DIRECTORY) {
+      rv = DeleteDirectoryRecursively(&file_ref);
+      if (rv != PP_OK && rv != PP_ERROR_FILENOTFOUND)
+        return rv;
+    } else {
+      callback.WaitForResult(file_ref.Delete(callback.GetCallback()));
+      rv = callback.result();
+      if (rv != PP_OK && rv != PP_ERROR_FILENOTFOUND)
+        return rv;
+    }
+  }
+  callback.WaitForResult(dir->Delete(callback.GetCallback()));
+  return callback.result();
+}
+
 void TestFileRef::RunTests(const std::string& filter) {
   RUN_CALLBACK_TEST(TestFileRef, Create, filter);
   RUN_CALLBACK_TEST(TestFileRef, GetFileSystemType, filter);
@@ -80,6 +120,9 @@ void TestFileRef::RunTests(const std::string& filter) {
   RUN_CALLBACK_TEST(TestFileRef, RenameFileAndDirectory, filter);
   RUN_CALLBACK_TEST(TestFileRef, Query, filter);
   RUN_CALLBACK_TEST(TestFileRef, FileNameEscaping, filter);
+  // FileRef::ReadEntries is in-process only.
+  if (testing_interface_->IsOutOfProcess())
+    RUN_CALLBACK_TEST(TestFileRef, ReadEntries, filter);
 }
 
 std::string TestFileRef::TestCreate() {
@@ -588,21 +631,124 @@ std::string TestFileRef::TestFileNameEscaping() {
   CHECK_CALLBACK_BEHAVIOR(callback);
   ASSERT_EQ(PP_OK, callback.result());
 
-  // DirectoryReader only works out-of-process.
+  // FileRef::ReadEntries only works out-of-process.
   if (testing_interface_->IsOutOfProcess()) {
-    TestCompletionCallbackWithOutput< std::vector<pp::DirectoryEntry_Dev> >
+    TestCompletionCallbackWithOutput<DirEntries>
         output_callback(instance_->pp_instance(), callback_type());
-    pp::DirectoryReader_Dev directory_reader(test_dir_ref);
 
     output_callback.WaitForResult(
-        directory_reader.ReadEntries(output_callback.GetCallback()));
+        test_dir_ref.ReadEntries(output_callback.GetCallback()));
     CHECK_CALLBACK_BEHAVIOR(output_callback);
     ASSERT_EQ(PP_OK, output_callback.result());
 
-    std::vector<pp::DirectoryEntry_Dev> entries = output_callback.output();
+    DirEntries entries = output_callback.output();
     ASSERT_EQ(1, entries.size());
     ASSERT_EQ(kTerribleName, entries.front().file_ref().GetName().AsString());
   }
 
   PASS();
 }
+
+std::string TestFileRef::TestReadEntries() {
+  TestCompletionCallback callback(instance_->pp_instance(), callback_type());
+  pp::FileSystem file_system(
+      instance_, PP_FILESYSTEMTYPE_LOCALTEMPORARY);
+  callback.WaitForResult(file_system.Open(1024, callback.GetCallback()));
+  CHECK_CALLBACK_BEHAVIOR(callback);
+  ASSERT_EQ(PP_OK, callback.result());
+
+  // Setup testing directories and files.
+  const char* test_dir_name = "/test_get_next_file";
+  const char* file_prefix = "file_";
+  const char* dir_prefix = "dir_";
+
+  pp::FileRef test_dir(file_system, test_dir_name);
+  int32_t rv = DeleteDirectoryRecursively(&test_dir);
+  if (rv != PP_OK && rv != PP_ERROR_FILENOTFOUND)
+    return ReportError("DeleteDirectoryRecursively", rv);

[dmichael (off chromium), 2013/05/02 19:43:54]

nit: I'd prefer just:
ASSERT_TRUE(rv == PP_OK || rv == PP_ERROR_FILENOTFOUND);

[hamaji, 2013/05/02 20:14:49]

Done.

+
+  callback.WaitForResult(test_dir.MakeDirectory(callback.GetCallback()));
+  CHECK_CALLBACK_BEHAVIOR(callback);
+  ASSERT_EQ(PP_OK, callback.result());
+
+  std::set<std::string> expected_file_names;
+  for (int i = 1; i < 4; ++i) {
+    char buffer[40];
+    sprintf(buffer, "%s/%s%d", test_dir_name, file_prefix, i);

[dmichael (off chromium), 2013/05/02 19:43:54]

snprintf

[hamaji, 2013/05/02 20:14:49]

Done.

+    pp::FileRef file_ref(file_system, buffer);
+
+    pp::FileIO file_io(instance_);
+    callback.WaitForResult(
+        file_io.Open(file_ref, PP_FILEOPENFLAG_CREATE, callback.GetCallback()));
+    CHECK_CALLBACK_BEHAVIOR(callback);
+    ASSERT_EQ(PP_OK, callback.result());
+
+    expected_file_names.insert(buffer);
+  }
+
+  std::set<std::string> expected_dir_names;
+  for (int i = 1; i < 4; ++i) {

[dmichael (off chromium), 2013/05/02 19:43:54]

suggestion: May make sense to use a constant like:
const int kNumDirectories = 3;
And loop while "i <= kNumDirectories" for readability and to keep the lopos in
sync

[hamaji, 2013/05/02 20:14:49]

It seems this loop creates directories but the loop above creates files. That
said, I agree adding kNum* would improve readability. Added kNumFiles and
kNumDirectories.

+    char buffer[40];
+    sprintf(buffer, "%s/%s%d", test_dir_name, dir_prefix, i);

[dmichael (off chromium), 2013/05/02 19:43:54]

snprintf

[hamaji, 2013/05/02 20:14:49]

Done.

+    pp::FileRef file_ref(file_system, buffer);
+
+    callback.WaitForResult(file_ref.MakeDirectory(callback.GetCallback()));
+    CHECK_CALLBACK_BEHAVIOR(callback);
+    ASSERT_EQ(PP_OK, callback.result());
+
+    expected_dir_names.insert(buffer);
+  }
+
+  // Test that |ReadEntries()| is able to fetch all directories and files that
+  // we created.
+  {
+    TestCompletionCallbackWithOutput<DirEntries> output_callback(
+        instance_->pp_instance(), callback_type());
+
+    output_callback.WaitForResult(
+        test_dir.ReadEntries(output_callback.GetCallback()));
+    CHECK_CALLBACK_BEHAVIOR(output_callback);
+    ASSERT_EQ(PP_OK, output_callback.result());
+
+    DirEntries entries = output_callback.output();
+    size_t sum = expected_file_names.size() + expected_dir_names.size();
+    if (entries.size() != sum)
+      return "Expected " + IntegerToString(sum) + " entries, got " +
+          IntegerToString(entries.size());

[dmichael (off chromium), 2013/05/02 19:43:54]

How about just ASSERT_EQ(entries.size(), sum);
Then you don't even need IntegerToString.

[hamaji, 2013/05/02 20:14:49]

Done.

+
+    for (DirEntries::const_iterator it = entries.begin();
+         it != entries.end(); ++it) {
+      pp::FileRef file_ref = it->file_ref();
+      std::string file_path = file_ref.GetPath().AsString();
+      std::set<std::string>::iterator found =
+          expected_file_names.find(file_path);
+      if (found != expected_file_names.end()) {
+        if (it->file_type() != PP_FILETYPE_REGULAR)
+          return file_path + " should have been a regular file.";
+        expected_file_names.erase(found);
+      } else {
+        found = expected_dir_names.find(file_path);
+        if (found == expected_dir_names.end())
+          return "Unexpected file path: " + file_path;
+        if (it->file_type() != PP_FILETYPE_DIRECTORY)
+          return file_path + " should have been a directory.";
+        expected_dir_names.erase(found);
+      }
+    }
+    if (!expected_file_names.empty() || !expected_dir_names.empty())
+      return "Expected more file paths.";

[dmichael (off chromium), 2013/05/02 19:43:54]

ASSERT_TRUE(expected_file_names.empty());
ASSERT_TRUE(expected_dir_names.empty());

[hamaji, 2013/05/02 20:14:49]

Done.

+  }
+
+  // Test cancellation of asynchronous |ReadEntries()|.
+  TestCompletionCallbackWithOutput<DirEntries> output_callback(
+      instance_->pp_instance(), callback_type());
+  {
+    rv = pp::FileRef(file_system, test_dir_name)
+        .ReadEntries(output_callback.GetCallback());
+  }
+  output_callback.WaitForAbortResult(rv);
+  CHECK_CALLBACK_BEHAVIOR(output_callback);
+
+
+  PASS();
+}