[fsp] Introduce BufferingFileStreamReader to read files in bigger chunks.

chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc

Index: chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc
diff --git a/chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc b/chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..e757e9e8d2de159d522d0754ae9dab4fe10ae3bf
--- /dev/null
+++ b/chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc
@@ -0,0 +1,293 @@
+// Copyright 2014 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 <string>
+#include <vector>
+
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"

[hashimoto, 2014/07/08 11:12:55]

nit: No need to include this.

[mtomasz, 2014/07/09 02:05:22]

Done.

+#include "base/message_loop/message_loop.h"

[hashimoto, 2014/07/08 11:12:55]

nit: This should be message_loop_proxy

[mtomasz, 2014/07/09 02:05:22]

Done.

+#include "base/run_loop.h"
+#include "chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "net/base/io_buffer.h"
+#include "net/base/net_errors.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace chromeos {
+namespace file_system_provider {
+namespace {
+
+// Size of the fake file in bytes.
+const int kFileSize = 1024;
+
+// Size of the preloading buffer in bytes.
+const int kBufferSize = 8;
+
+// Number of bytes requested per BufferingFileStreamReader::Read().
+const int kChunkSize = 3;
+
+// Pushes a value to the passed log vector.
+template <typename T>
+void LogValue(std::vector<T>* log, T value) {
+  log->push_back(value);
+}
+
+// Fake internal file stream reader.
+class FakeFileStreamReader : public webkit_blob::FileStreamReader {
+ public:
+  FakeFileStreamReader(std::vector<int>* log, bool return_error)

[hashimoto, 2014/07/08 11:12:55]

How about replacing |return_error| with net::Error (or int)?
This way there is no need to have /* return_error */ comments everywhere.

[mtomasz, 2014/07/09 02:05:22]

Done.

+      : log_(log), return_error_(return_error) {}
+  virtual ~FakeFileStreamReader() {}
+
+  // webkit_blob::FileStreamReader overrides.
+  virtual int Read(net::IOBuffer* buf,
+                   int buf_len,
+                   const net::CompletionCallback& callback) OVERRIDE {
+    DCHECK(log_);
+    log_->push_back(buf_len);
+
+    if (return_error_) {
+      base::MessageLoopProxy::current()->PostTask(
+          FROM_HERE, base::Bind(callback, net::ERR_ACCESS_DENIED));
+      return net::ERR_IO_PENDING;
+    }
+
+    const std::string fake_data('X', buf_len);
+    memcpy(buf->data(), fake_data.c_str(), buf_len);
+
+    base::MessageLoopProxy::current()->PostTask(FROM_HERE,
+                                                base::Bind(callback, buf_len));
+    return net::ERR_IO_PENDING;
+  }
+
+  virtual int64 GetLength(
+      const net::Int64CompletionCallback& callback) OVERRIDE {
+    base::MessageLoopProxy::current()->PostTask(
+        FROM_HERE, base::Bind(callback, kFileSize));
+    return net::ERR_IO_PENDING;
+  }
+
+ private:
+  std::vector<int>* log_;  // Not owned.
+  bool return_error_;
+  DISALLOW_COPY_AND_ASSIGN(FakeFileStreamReader);
+};
+
+}  // namespace
+
+class FileSystemProviderBufferingFileStreamReaderTest : public testing::Test {
+ protected:
+  FileSystemProviderBufferingFileStreamReaderTest() {}
+  virtual ~FileSystemProviderBufferingFileStreamReaderTest() {}
+
+  content::TestBrowserThreadBundle thread_bundle_;
+};
+
+TEST_F(FileSystemProviderBufferingFileStreamReaderTest, Read) {
+  std::vector<int> inner_read_log;
+  BufferingFileStreamReader reader(
+      scoped_ptr<webkit_blob::FileStreamReader>(
+          new FakeFileStreamReader(&inner_read_log, false /* return_error */)),
+      kBufferSize);
+
+  // For the first read, the internal file stream reader is fired, as there is
+  // no data in the preloading buffer.
+  {
+    scoped_refptr<net::IOBuffer> buffer(new net::IOBuffer(kChunkSize));
+    std::vector<int> read_log;
+    const int result =
+        reader.Read(buffer, kChunkSize, base::Bind(&LogValue<int>, &read_log));
+    base::RunLoop().RunUntilIdle();
+
+    EXPECT_EQ(net::ERR_IO_PENDING, result);
+    ASSERT_EQ(1u, inner_read_log.size());
+    EXPECT_EQ(kBufferSize, inner_read_log[0]);
+    ASSERT_EQ(1u, read_log.size());
+    EXPECT_EQ(kChunkSize, read_log[0]);
+  }
+
+  // Second read should return data from the preloading buffer, without calling
+  // the internal file stream reader.
+  {
+    scoped_refptr<net::IOBuffer> buffer(new net::IOBuffer(kChunkSize));
+    std::vector<int> read_log;
+    const int result =
+        reader.Read(buffer, kChunkSize, base::Bind(&LogValue<int>, &read_log));
+    base::RunLoop().RunUntilIdle();
+
+    EXPECT_EQ(kChunkSize, result);
+    EXPECT_EQ(1u, inner_read_log.size());
+    // Results returned synchronously, so no new read result events.
+    EXPECT_EQ(0u, read_log.size());
+  }
+
+  // Third read should return partial result from the preloading buffer. It is
+  // valid to return less bytes than requested.
+  {
+    scoped_refptr<net::IOBuffer> buffer(new net::IOBuffer(kChunkSize));
+    std::vector<int> read_log;
+    const int result =
+        reader.Read(buffer, kChunkSize, base::Bind(&LogValue<int>, &read_log));
+    base::RunLoop().RunUntilIdle();
+
+    EXPECT_EQ(kBufferSize - 2 * kChunkSize, result);
+    EXPECT_EQ(1u, inner_read_log.size());
+    // Results returned synchronously, so no new read result events.
+    EXPECT_EQ(0u, read_log.size());
+  }
+
+  // The preloading buffer is now empty, so reading should invoke the internal
+  // file stream reader.
+  {
+    scoped_refptr<net::IOBuffer> buffer(new net::IOBuffer(kChunkSize));
+    std::vector<int> read_log;
+    const int result =
+        reader.Read(buffer, kChunkSize, base::Bind(&LogValue<int>, &read_log));
+    base::RunLoop().RunUntilIdle();
+
+    EXPECT_EQ(net::ERR_IO_PENDING, result);
+    ASSERT_EQ(2u, inner_read_log.size());
+    EXPECT_EQ(kBufferSize, inner_read_log[0]);

[hashimoto, 2014/07/08 11:12:55]

Shouldn't this be inner_read_log[1]?

BTW, how about calling inner_read_log.clear() before calling Read()?
It should eliminate this kind of mistakes.

[mtomasz, 2014/07/09 02:05:22]

Good idea. Done.

+    ASSERT_EQ(1u, read_log.size());
+    EXPECT_EQ(kChunkSize, read_log[0]);
+  }
+}
+
+TEST_F(FileSystemProviderBufferingFileStreamReaderTest, Read_Directly) {
+  std::vector<int> inner_read_log;
+  BufferingFileStreamReader reader(
+      scoped_ptr<webkit_blob::FileStreamReader>(
+          new FakeFileStreamReader(&inner_read_log, false /* return_error */)),
+      kBufferSize);
+
+  // First read couple of bytes, so the internal buffer is filled out.
+  {
+    scoped_refptr<net::IOBuffer> buffer(new net::IOBuffer(kChunkSize));
+    std::vector<int> read_log;
+    const int result =
+        reader.Read(buffer, kChunkSize, base::Bind(&LogValue<int>, &read_log));
+    base::RunLoop().RunUntilIdle();
+
+    EXPECT_EQ(net::ERR_IO_PENDING, result);
+    ASSERT_EQ(1u, inner_read_log.size());
+    EXPECT_EQ(kBufferSize, inner_read_log[0]);
+    ASSERT_EQ(1u, read_log.size());
+    EXPECT_EQ(kChunkSize, read_log[0]);
+  }
+
+  const int read_bytes = kBufferSize * 2;
+  ASSERT_GT(kFileSize, read_bytes);
+
+  // Reading more than the internal buffer size would cause fetching only
+  // as much as available in the internal buffer.
+  {
+    scoped_refptr<net::IOBuffer> buffer(new net::IOBuffer(kChunkSize));
+    std::vector<int> read_log;
+    const int result =
+        reader.Read(buffer, read_bytes, base::Bind(&LogValue<int>, &read_log));
+    base::RunLoop().RunUntilIdle();
+
+    EXPECT_EQ(kBufferSize - kChunkSize, result);
+    ASSERT_EQ(1u, inner_read_log.size());
+    EXPECT_EQ(kBufferSize, inner_read_log[0]);
+    EXPECT_EQ(0u, read_log.size());
+  }
+
+  // The internal buffer is clean. Fetching more than the internal buffer size
+  // would cause fetching data directly from the inner reader, with skipping
+  // the internal buffer.
+  {
+    scoped_refptr<net::IOBuffer> buffer(new net::IOBuffer(kChunkSize));
+    std::vector<int> read_log;
+    const int result =
+        reader.Read(buffer, read_bytes, base::Bind(&LogValue<int>, &read_log));
+    base::RunLoop().RunUntilIdle();
+
+    EXPECT_EQ(net::ERR_IO_PENDING, result);
+    ASSERT_EQ(2u, inner_read_log.size());
+    EXPECT_EQ(read_bytes, inner_read_log[1]);
+    ASSERT_EQ(1u, read_log.size());
+    EXPECT_EQ(read_bytes, read_log[0]);
+  }
+}
+
+TEST_F(FileSystemProviderBufferingFileStreamReaderTest,
+       Read_MoreThanBufferSize) {
+  std::vector<int> inner_read_log;
+  BufferingFileStreamReader reader(
+      scoped_ptr<webkit_blob::FileStreamReader>(
+          new FakeFileStreamReader(&inner_read_log, false /* return_error */)),
+      kBufferSize);
+  // First read couple of bytes, so the internal buffer is filled out.
+  {
+    scoped_refptr<net::IOBuffer> buffer(new net::IOBuffer(kChunkSize));
+    std::vector<int> read_log;
+    const int result =
+        reader.Read(buffer, kChunkSize, base::Bind(&LogValue<int>, &read_log));
+    base::RunLoop().RunUntilIdle();
+
+    EXPECT_EQ(net::ERR_IO_PENDING, result);
+    ASSERT_EQ(1u, inner_read_log.size());
+    EXPECT_EQ(kBufferSize, inner_read_log[0]);
+    ASSERT_EQ(1u, read_log.size());
+    EXPECT_EQ(kChunkSize, read_log[0]);
+  }
+
+  // Returning less than requested number of bytes is valid, and should not
+  // fail.
+  {
+    const int chunk_size = 20;
+    ASSERT_LT(kBufferSize, chunk_size);
+    scoped_refptr<net::IOBuffer> buffer(new net::IOBuffer(chunk_size));
+    std::vector<int> read_log;
+    const int result =
+        reader.Read(buffer, chunk_size, base::Bind(&LogValue<int>, &read_log));
+    base::RunLoop().RunUntilIdle();
+
+    EXPECT_EQ(5, result);
+    EXPECT_EQ(1u, inner_read_log.size());
+    EXPECT_EQ(0u, read_log.size());
+  }
+}
+
+TEST_F(FileSystemProviderBufferingFileStreamReaderTest, Read_WithError) {
+  std::vector<int> inner_read_log;
+  BufferingFileStreamReader reader(
+      scoped_ptr<webkit_blob::FileStreamReader>(
+          new FakeFileStreamReader(&inner_read_log, true /* return_error */)),
+      kBufferSize);
+
+  scoped_refptr<net::IOBuffer> buffer(new net::IOBuffer(kChunkSize));
+  std::vector<int> read_log;
+  const int result =
+      reader.Read(buffer, kChunkSize, base::Bind(&LogValue<int>, &read_log));
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(net::ERR_IO_PENDING, result);
+  ASSERT_EQ(1u, inner_read_log.size());
+  EXPECT_EQ(kBufferSize, inner_read_log[0]);
+  ASSERT_EQ(1u, read_log.size());
+  EXPECT_EQ(net::ERR_ACCESS_DENIED, read_log[0]);
+}
+
+TEST_F(FileSystemProviderBufferingFileStreamReaderTest, GetLength) {
+  BufferingFileStreamReader reader(
+      scoped_ptr<webkit_blob::FileStreamReader>(
+          new FakeFileStreamReader(NULL, false /* return_error */)),
+      kBufferSize);
+
+  std::vector<int64> get_length_log;
+  const int64 result =
+      reader.GetLength(base::Bind(&LogValue<int64>, &get_length_log));
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(net::ERR_IO_PENDING, result);
+  ASSERT_EQ(1u, get_length_log.size());
+  EXPECT_EQ(kFileSize, get_length_log[0]);
+}
+
+}  // namespace file_system_provider
+}  // namespace chromeos