Clean up ARC file system unit tests.

components/arc/test/fake_file_system_instance.h

 // Copyright 2016 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.
 
 #ifndef COMPONENTS_ARC_TEST_FAKE_FILE_SYSTEM_INSTANCE_H_
 #define COMPONENTS_ARC_TEST_FAKE_FILE_SYSTEM_INSTANCE_H_
 
+#include <stdint.h>
+
+#include <map>
 #include <string>
+#include <utility>
 #include <vector>
 
+#include "base/files/scoped_temp_dir.h"
 #include "base/macros.h"
 #include "components/arc/common/file_system.mojom.h"
 
 namespace arc {
 
-// A fake implementation which returns errors for all requests or just ignores
-// them.
+// Fake implementation to operate on documents in memory.
+//
+// ArcFileSystemOperationRunner provides two types of methods: content URL
+// based and documents provider based. According to backend type, you need
+// to setup the fake with different functions.
+//
+// Content URL based functions are:
+// - GetFileSize()
+// - OpenFileToRead()
+// Fake files for those functions can be set up by AddFile().
+//
+// Documents provider based functions are:
+// - GetDocument()
+// - GetChildDocuments()
+// Fake documents for those functions can be set up by AddDocument().
 class FakeFileSystemInstance : public mojom::FileSystemInstance {
  public:
+  // Specification of a fake file available to content URL based methods.
+  struct FileSpec {
+    // Content URL of a file.
+    const char* url;
+
+    // The content of a file.
+    const char* content;

[hidehiko, 2017/01/26 07:24:20]

Could you comment this only accept c-style string?

[Shuhei Takahashi, 2017/01/27 09:55:10]

Good point, done.

+
+    // If this is false, OpenFileToRead() for this file returns a seekable
+    // file descriptor. Otherwise it returns a non-seekable pipe.
+    bool stream;

[hidehiko, 2017/01/26 07:24:20]

Optional: "seekable" may be consistent with your comment...?

[Shuhei Takahashi, 2017/01/27 09:55:10]

Done.

+
+    constexpr FileSpec() : FileSpec(nullptr, nullptr, false) {}

[hidehiko, 2017/01/26 07:24:20]

This is POD, so I do not think you need ctors?

[hashimoto, 2017/01/26 08:05:54]

Or, how about replacing const char* with std::string to allow new tests to
construct test data dynamically?

[Shuhei Takahashi, 2017/01/27 09:55:10]

std::string makes this class non-constexpr, and then we can't define FileSpec
constants in global scope, reducing convenience in unit tests a bit. Or if you
have any suggestion to write such kind of unit test constants without depending
on constexpr please let me know.

[hashimoto, 2017/01/27 10:46:50]

I'm concerned about possible use-after-free of strings, as it's easy to fill
this struct with std::string::c_str() and mistakenly mutate or destroy the
string object during this class's lifetime.
Could you at least stop storing const char* in files_ and documents_?

+    constexpr FileSpec(const char* url, const char* content, bool stream)
+        : url(url), content(content), stream(stream) {}
+  };
+
+  // Specification of a fake document available to documents provider based
+  // methods.
+  struct DocumentSpec {
+    // Authority.
+    const char* authority;
+
+    // ID of this document.
+    const char* document_id;
+
+    // ID of the parent document. Can be nullptr string if this is a root.
+    const char* parent_document_id;
+
+    // File name displayed to users.
+    const char* display_name;
+
+    // MIME type.
+    const char* mime_type;
+
+    // File size in bytes. Set to -1 if size is not available.
+    int64_t size;
+
+    // Last modified time in milliseconds from the UNIX epoch.
+    uint64_t last_modified;

[hashimoto, 2017/01/26 08:05:54]

base::Time() is Chromium's standard type for time values.

[Shuhei Takahashi, 2017/01/27 09:55:10]

I know, but in this case it makes more sense to use the most similar type as one
used in file_system.mojom because the purpose of this class is to fake
FileSystemInstance. It was also suggested to use a time type in
file_system.mojom but it was technically impossible due to an existing bug. I
added a TODO comment.

+
+    constexpr DocumentSpec()

[hidehiko, 2017/01/26 07:24:20]

Same here.

[Shuhei Takahashi, 2017/01/27 09:55:10]

Done.

+        : DocumentSpec(nullptr, nullptr, nullptr, nullptr, nullptr, -1, 0) {}
+    constexpr DocumentSpec(const char* authority,
+                           const char* document_id,
+                           const char* parent_document_id,
+                           const char* display_name,
+                           const char* mime_type,
+                           int64_t size,
+                           uint64_t last_modified)
+        : authority(authority),
+          document_id(document_id),
+          parent_document_id(parent_document_id),
+          display_name(display_name),
+          mime_type(mime_type),
+          size(size),
+          last_modified(last_modified) {}
+  };
+
   FakeFileSystemInstance();
+  ~FakeFileSystemInstance() override;
+
+  // Adds a file accessible by content URL based methods.
+  void AddFile(const FileSpec& spec);
+
+  // Adds a document accessible by document provider based methods.
+  void AddDocument(const DocumentSpec& spec);
 
   // mojom::FileSystemInstance:
-  ~FakeFileSystemInstance() override;
-
   void GetChildDocuments(const std::string& authority,
                          const std::string& document_id,
                          const GetChildDocumentsCallback& callback) override;
-
   void GetDocument(const std::string& authority,
                    const std::string& document_id,
                    const GetDocumentCallback& callback) override;
-
   void GetFileSize(const std::string& url,
                    const GetFileSizeCallback& callback) override;
-
   void OpenFileToRead(const std::string& url,
                       const OpenFileToReadCallback& callback) override;
-
   void RequestMediaScan(const std::vector<std::string>& paths) override;
 
  private:
+  // A pair of an authority and a document ID which identifies the location
+  // of a document in documents providers.
+  using DocumentKey = std::pair<std::string, std::string>;
+
+  base::ScopedTempDir temp_dir_;
+
+  std::map<std::string, FileSpec> files_;

[hashimoto, 2017/01/26 08:05:54]

Please make it clear that this map is from URL to file.
(e.g. renaming this to urls_to_files_, or adding a comment).

[Shuhei Takahashi, 2017/01/27 09:55:10]

Added comments.

+
+  std::map<DocumentKey, DocumentSpec> documents_;
+  std::map<DocumentKey, std::vector<DocumentKey>> child_documents_;
+
   DISALLOW_COPY_AND_ASSIGN(FakeFileSystemInstance);
 };
 
 }  // namespace arc
 
 #endif  // COMPONENTS_ARC_TEST_FAKE_FILE_SYSTEM_INSTANCE_H_