Revert of mojo filesystem: Simplify full file reading/writing. (patchset #5...

Revert of mojo filesystem: Simplify full file reading/writing. (patchset #5 id:80001 of https://codereview.chromium.org/1634293002/ )

Reason for revert:
Broke the Windows build.

https://build.chromium.org/p/chromium.win/builders/Win%20x64%20GN/builds/10430/steps/compile/logs/stdio

FAILED: ninja -t msvc -e environment.x64 -- E:\b\build\goma/gomacc.exe "E:\b\depot_tools\win_toolchain\vs2013_files\VC\bin\amd64/cl.exe" /nologo /showIncludes /FC @obj/components/filesystem/lib/directory_impl.obj.rsp /c ../../components/filesystem/directory_impl.cc /Foobj/components/filesystem/lib/directory_impl.obj /Fdobj/components/filesystem/lib_cc.pdb
e:\b\build\slave\win_x64_gn\build\src\components\filesystem\directory_impl.cc(274) : error C2220: warning treated as error - no 'object' file generated
e:\b\build\slave\win_x64_gn\build\src\components\filesystem\directory_impl.cc(274) : warning C4267: 'argument' : conversion from 'size_t' to 'int', possible loss of data
ninja: build stopped: subcommand failed.

Original issue's description:
> mojo filesystem: Simplify full file reading/writing.
>
> One common pattern that's coming up multiple times is that we want to
> read or write the full contents of a file. The first attempt at an
> interface to that had was put on the File object.
>
> However, file seeking behaviour differs between platforms. Performing a
> seek on an empty file is safe on posix and errors on Windows. And when
> dealing with arbitrary File objects, we want to seek to the beginning
> just in case there was any previous usage on the File object.
>
> It was also cumbersome. The user was still responsible for opening the
> file and closing it once they were done with it. Putting these
> operations on Directory not only removes a bug, but also simplifies the
> interface.
>
> BUG=557405
>
> Committed: https://crrev.com/fd918ae590f61f145d75c800abb8eb050ca32b06
> Cr-Commit-Position: refs/heads/master@{#371845}

TBR=sky@chromium.org,erg@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=557405

Review URL: https://codereview.chromium.org/1643733002

Cr-Commit-Position: refs/heads/master@{#371863}

components/filesystem/directory_impl.cc

@@ -15,7 +15,6 @@
 #include "build/build_config.h"
 #include "components/filesystem/file_impl.h"
 #include "components/filesystem/util.h"
-#include "mojo/common/common_type_converters.h"
 
 namespace filesystem {
 
@@ -65,7 +64,7 @@ void DirectoryImpl::OpenFile(const mojo::String& raw_path,
 
 #if defined(OS_WIN)
   // On Windows, FILE_FLAG_BACKUP_SEMANTICS is needed to open a directory.
-  if (base::DirectoryExists(path))
+  if (DirectoryExists(path))
     open_flags |= base::File::FLAG_BACKUP_SEMANTICS;
 #endif  // OS_WIN
 
@@ -216,68 +215,4 @@ void DirectoryImpl::Flush(const FlushCallback& callback) {
   callback.Run(FileError::OK);
 }
 
-void DirectoryImpl::ReadEntireFile(const mojo::String& raw_path,
-                                   const ReadEntireFileCallback& callback) {
-  base::FilePath path;
-  FileError error = ValidatePath(raw_path, directory_path_, &path);
-  if (error != FileError::OK) {
-    callback.Run(error, mojo::Array<uint8_t>(0));
-    return;
-  }
-
-  if (base::DirectoryExists(path)) {
-    callback.Run(FileError::NOT_A_FILE, mojo::Array<uint8_t>(0));
-    return;
-  }
-
-  base::File base_file(path, base::File::FLAG_OPEN | base::File::FLAG_READ);
-  if (!base_file.IsValid()) {
-    callback.Run(GetError(base_file), mojo::Array<uint8_t>(0));
-    return;
-  }
-
-  std::string contents;
-  const int kBufferSize = 1 << 16;
-  scoped_ptr<char[]> buf(new char[kBufferSize]);
-  int len;
-  while ((len = base_file.ReadAtCurrentPos(buf.get(), kBufferSize)) > 0)
-    contents.append(buf.get(), len);
-
-  callback.Run(FileError::OK, mojo::Array<uint8_t>::From(contents));
-}
-
-void DirectoryImpl::WriteFile(const mojo::String& raw_path,
-                              mojo::Array<uint8_t> data,
-                              const WriteFileCallback& callback) {
-  base::FilePath path;
-  FileError error = ValidatePath(raw_path, directory_path_, &path);
-  if (error != FileError::OK) {
-    callback.Run(error);
-    return;
-  }
-
-  if (base::DirectoryExists(path)) {
-    callback.Run(FileError::NOT_A_FILE);
-    return;
-  }
-
-  base::File base_file(path,
-                       base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE);
-  if (!base_file.IsValid()) {
-    callback.Run(GetError(base_file));
-    return;
-  }
-
-  // If we're given empty data, we don't write and just truncate the file.
-  if (data.size()) {
-    if (base_file.Write(0, reinterpret_cast<char*>(&data.front()),
-                        data.size()) == -1) {
-      callback.Run(GetError(base_file));
-      return;
-    }
-  }
-
-  callback.Run(FileError::OK);
-}
-
 }  // namespace filesystem

components/filesystem/directory_impl.h

@@ -55,11 +55,6 @@ class DirectoryImpl : public Directory {
   void IsWritable(const mojo::String& path,
                   const IsWritableCallback& callback) override;
   void Flush(const FlushCallback& callback) override;
-  void ReadEntireFile(const mojo::String& path,
-                      const ReadEntireFileCallback& callback) override;
-  void WriteFile(const mojo::String& path,
-                 mojo::Array<uint8_t> data,
-                 const WriteFileCallback& callback) override;
 
  private:
   mojo::StrongBinding<Directory> binding_;

components/filesystem/directory_impl_unittest.cc

@@ -10,7 +10,6 @@
 
 #include "base/macros.h"
 #include "components/filesystem/files_test_base.h"
-#include "mojo/common/common_type_converters.h"
 #include "mojo/util/capture_util.h"
 
 using mojo::Capture;
@@ -154,93 +153,6 @@ TEST_F(DirectoryImplTest, CantOpenDirectoriesAsFiles) {
   }
 }
 
-TEST_F(DirectoryImplTest, WriteFileReadFile) {
-  DirectoryPtr directory;
-  GetTemporaryRoot(&directory);
-  FileError error;
-
-  std::string data("one two three");
-  {
-    directory->WriteFile("data", mojo::Array<uint8_t>::From(data),
-                         Capture(&error));
-    ASSERT_TRUE(directory.WaitForIncomingResponse());
-    EXPECT_EQ(FileError::OK, error);
-  }
-
-  {
-    mojo::Array<uint8_t> file_contents;
-    directory->ReadEntireFile("data", Capture(&error, &file_contents));
-    ASSERT_TRUE(directory.WaitForIncomingResponse());
-    EXPECT_EQ(FileError::OK, error);
-
-    EXPECT_EQ(data, file_contents.To<std::string>());
-  }
-}
-
-TEST_F(DirectoryImplTest, ReadEmptyFileIsNotFoundError) {
-  DirectoryPtr directory;
-  GetTemporaryRoot(&directory);
-  FileError error;
-
-  {
-    mojo::Array<uint8_t> file_contents;
-    directory->ReadEntireFile("doesnt_exist", Capture(&error, &file_contents));
-    ASSERT_TRUE(directory.WaitForIncomingResponse());
-    EXPECT_EQ(FileError::NOT_FOUND, error);
-  }
-}
-
-TEST_F(DirectoryImplTest, CantReadEntireFileOnADirectory) {
-  DirectoryPtr directory;
-  GetTemporaryRoot(&directory);
-  FileError error;
-
-  // Create a directory
-  {
-    DirectoryPtr my_file_directory;
-    error = FileError::FAILED;
-    directory->OpenDirectory(
-        "my_dir", GetProxy(&my_file_directory),
-        kFlagRead | kFlagWrite | kFlagCreate,
-        Capture(&error));
-    ASSERT_TRUE(directory.WaitForIncomingResponse());
-    EXPECT_EQ(FileError::OK, error);
-  }
-
-  // Try to read it as a file
-  {
-    mojo::Array<uint8_t> file_contents;
-    directory->ReadEntireFile("my_dir", Capture(&error, &file_contents));
-    ASSERT_TRUE(directory.WaitForIncomingResponse());
-    EXPECT_EQ(FileError::NOT_A_FILE, error);
-  }
-}
-
-TEST_F(DirectoryImplTest, CantWriteFileOnADirectory) {
-  DirectoryPtr directory;
-  GetTemporaryRoot(&directory);
-  FileError error;
-
-  // Create a directory
-  {
-    DirectoryPtr my_file_directory;
-    error = FileError::FAILED;
-    directory->OpenDirectory(
-        "my_dir", GetProxy(&my_file_directory),
-        kFlagRead | kFlagWrite | kFlagCreate,
-        Capture(&error));
-    ASSERT_TRUE(directory.WaitForIncomingResponse());
-    EXPECT_EQ(FileError::OK, error);
-  }
-
-  {
-    std::string data("one two three");
-    directory->WriteFile("my_dir", mojo::Array<uint8_t>::From(data),
-                         Capture(&error));
-    ASSERT_TRUE(directory.WaitForIncomingResponse());
-    EXPECT_EQ(FileError::NOT_A_FILE, error);
-  }
-}
 
 // TODO(vtl): Test delete flags.
 

components/filesystem/file_impl.cc

@@ -94,6 +94,28 @@ void FileImpl::Read(uint32_t num_bytes_to_read,
   callback.Run(FileError::OK, std::move(bytes_read));
 }
 
+void FileImpl::ReadEntireFile(const ReadEntireFileCallback& callback) {
+  if (!file_.IsValid()) {
+    callback.Run(GetError(file_), mojo::Array<uint8_t>());
+    return;
+  }
+
+  // Seek to the front of the file.
+  if (file_.Seek(base::File::FROM_BEGIN, 0) == -1) {
+    callback.Run(FileError::FAILED, mojo::Array<uint8_t>());
+    return;
+  }
+
+  std::string contents;
+  const int kBufferSize = 1 << 16;
+  scoped_ptr<char[]> buf(new char[kBufferSize]);
+  size_t len;
+  while ((len = file_.ReadAtCurrentPos(buf.get(), kBufferSize)) > 0)
+    contents.append(buf.get(), len);
+
+  callback.Run(FileError::OK, mojo::Array<uint8_t>::From(contents));
+}
+
 // TODO(vtl): Move the implementation to a thread pool.
 void FileImpl::Write(mojo::Array<uint8_t> bytes_to_write,
                      int64_t offset,

components/filesystem/file_impl.h

@@ -34,6 +34,7 @@ class FileImpl : public File {
             int64_t offset,
             Whence whence,
             const ReadCallback& callback) override;
+  void ReadEntireFile(const ReadEntireFileCallback& callback) override;
   void Write(mojo::Array<uint8_t> bytes_to_write,
              int64_t offset,
              Whence whence,

components/filesystem/public/cpp/prefs/filesystem_json_pref_store.cc

@@ -348,6 +348,17 @@ void FilesystemJsonPrefStore::OnOpenFilesystem(base::Closure callback,
 }
 
 void FilesystemJsonPrefStore::OnTempFileWriteStart() {
+  // Open up a temporary file and truncate it.
+  directory_->OpenFile(
+      "tmp", GetProxy(&temporary_file_), kFlagWrite | kFlagCreate,
+      Bind(&FilesystemJsonPrefStore::OnTempFileOpened, AsWeakPtr()));
+}
+
+void FilesystemJsonPrefStore::OnTempFileOpened(FileError err) {
+  // TODO(erg): Error handling. The JsonPrefStore code assumes that writing the
+  // file can never fail.
+  CHECK_EQ(FileError::OK, err);
+
   // Calculate what we want to write, and then write to the temporary file.
   pending_lossy_write_ = false;
 
@@ -359,29 +370,50 @@ void FilesystemJsonPrefStore::OnTempFileWriteStart() {
   serializer.set_pretty_print(false);
   serializer.Serialize(*prefs_);
 
-  directory_->WriteFile(
-      "tmp",
-      mojo::Array<uint8_t>::From(output),
+  temporary_file_->Write(
+      mojo::Array<uint8_t>::From(output), 0, Whence::FROM_CURRENT,
       Bind(&FilesystemJsonPrefStore::OnTempFileWrite, AsWeakPtr()));
 }
 
-void FilesystemJsonPrefStore::OnTempFileWrite(FileError err) {
+void FilesystemJsonPrefStore::OnTempFileWrite(FileError err,
+                                              uint32_t num_bytes_written) {
+  // TODO(erg): Error handling. The JsonPrefStore code assumes that writing the
+  // file can never fail.
+  CHECK_EQ(FileError::OK, err);
+
+  // Now that we've written the file, close it.
+  temporary_file_->Close(
+      Bind(&FilesystemJsonPrefStore::OnTempFileClosed, AsWeakPtr()));
+}
+
+void FilesystemJsonPrefStore::OnTempFileClosed(FileError err) {
   // TODO(erg): Error handling. The JsonPrefStore code assumes that writing the
   // file can never fail.
   CHECK_EQ(FileError::OK, err);
 
+  temporary_file_.reset();
   directory_->Rename(
       "tmp", path_,
       Bind(&FilesystemJsonPrefStore::OnTempFileRenamed, AsWeakPtr()));
 }
 
-void FilesystemJsonPrefStore::OnTempFileRenamed(FileError err) {
-}
+void FilesystemJsonPrefStore::OnTempFileRenamed(FileError err) {}
 
 void FilesystemJsonPrefStore::OnPreferencesReadStart() {
-  directory_->ReadEntireFile(
-      path_,
-      Bind(&FilesystemJsonPrefStore::OnPreferencesFileRead, AsWeakPtr()));
+  // TODO(erg): implement me.
+  directory_->OpenFile(
+      path_, GetProxy(&preferences_file_), kFlagRead | kFlagOpen,
+      Bind(&FilesystemJsonPrefStore::OnPreferencesFileOpened, AsWeakPtr()));
+}
+
+void FilesystemJsonPrefStore::OnPreferencesFileOpened(FileError err) {
+  // TODO(erg): Error handling.
+  if (err == FileError::OK) {
+    preferences_file_->ReadEntireFile(
+        Bind(&FilesystemJsonPrefStore::OnPreferencesFileRead, AsWeakPtr()));
+  } else {
+    OnPreferencesFileRead(err, mojo::Array<uint8_t>());
+  }
 }
 
 void FilesystemJsonPrefStore::OnPreferencesFileRead(
@@ -391,13 +423,12 @@ void FilesystemJsonPrefStore::OnPreferencesFileRead(
       new FilesystemJsonPrefStore::ReadResult);
   // TODO(erg): Needs even better error handling.
   switch (err) {
-    case FileError::FAILED:
     case FileError::IN_USE:
-    case FileError::ACCESS_DENIED:
-    case FileError::NOT_A_FILE: {
+    case FileError::ACCESS_DENIED: {
       read_only_ = true;
       break;
     }
+    case FileError::FAILED:
     case FileError::NOT_FOUND: {
       // If the file just doesn't exist, maybe this is the first run. Just
       // don't pass a value.
@@ -414,6 +445,8 @@ void FilesystemJsonPrefStore::OnPreferencesFileRead(
     }
   }
 
+  preferences_file_.reset();
+
   OnFileRead(std::move(read_result));
 }
 

components/filesystem/public/cpp/prefs/filesystem_json_pref_store.h

@@ -140,11 +140,14 @@ class FilesystemJsonPrefStore
 
   // Asynchronous implementation details of PerformWrite().
   void OnTempFileWriteStart();
-  void OnTempFileWrite(FileError err);
+  void OnTempFileOpened(FileError err);
+  void OnTempFileWrite(FileError err, uint32_t num_bytes_written);
+  void OnTempFileClosed(FileError err);
   void OnTempFileRenamed(FileError err);
 
   // Asynchronous implementation details of ReadPrefsAsync().
   void OnPreferencesReadStart();
+  void OnPreferencesFileOpened(FileError err);
   void OnPreferencesFileRead(FileError err, mojo::Array<uint8_t> contents);
 
   const std::string path_;
@@ -155,6 +158,9 @@ class FilesystemJsonPrefStore
   // |filesystem. See OpenFilesystem().
   DirectoryPtr directory_;
 
+  FilePtr preferences_file_;
+  FilePtr temporary_file_;
+
   scoped_ptr<base::DictionaryValue> prefs_;
 
   bool read_only_;

components/filesystem/public/interfaces/directory.mojom

@@ -52,12 +52,6 @@ interface Directory {
   // fsync()/FlushFileBuffers().
   Flush() => (FileError error);
 
-  // Reads the contents of an entire file.
-  ReadEntireFile(string path) => (FileError error, array<uint8> data);
-
-  // Writes |data| to |path|, overwriting the file if it already exists.
-  WriteFile(string path, array<uint8> data) => (FileError error);
-
   // TODO(vtl): directory "streaming"?
   // TODO(vtl): "make root" (i.e., prevent cd-ing, etc., to parent); note that
   // this would require a much more complicated implementation (e.g., it needs

components/filesystem/public/interfaces/file.mojom

@@ -27,6 +27,10 @@ interface File {
   Read(uint32 num_bytes_to_read, int64 offset, Whence whence)
       => (FileError error, array<uint8>? bytes_read);
 
+  // Returns the entire contents of the file as an array of bytes. The mojo
+  // equivalent of base::ReadFileToString().
+  ReadEntireFile() => (FileError error, array<uint8>? bytes_read);
+
   // Writes |bytes_to_write| to the location specified by |offset|/|whence|.
   // TODO(vtl): Clarify behavior when |num_bytes_written| is less than the size
   // of |bytes_to_write|.