[fsp] Buffer consecutive Write() calls.

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

Index: chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer_unittest.cc
diff --git a/chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer_unittest.cc b/chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..31ede7ae5f8d2a4289f9ca2e9a0cbc8320ffdbc6
--- /dev/null
+++ b/chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer_unittest.cc
@@ -0,0 +1,401 @@
+// 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/run_loop.h"
+#include "base/thread_task_runner_handle.h"
+#include "chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_writer.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 intermediate buffer in bytes.
+const int kIntermediateBufferLength = 8;
+
+// Testing text to be written. The length must be 5 bytes, to invoke all code
+// paths for buffering.
+const char kShortTextToWrite[] = "Kitty";
+
+// Testing text to be written. The length must be longr than the intermediate
+// buffer length, in order to invoke a direct write.
+const char kLongTextToWrite[] = "Welcome to my world!";
+
+// 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 writer.
+class FakeFileStreamWriter : public storage::FileStreamWriter {
+ public:
+  FakeFileStreamWriter(std::vector<std::string>* write_log,
+                       std::vector<int>* flush_log,
+                       net::Error write_error)
+      : pending_bytes_(0),
+        write_log_(write_log),
+        flush_log_(flush_log),
+        write_error_(write_error) {}
+  virtual ~FakeFileStreamWriter() {}
+
+  // storage::FileStreamWriter overrides.
+  virtual int Write(net::IOBuffer* buf,
+                    int buf_len,
+                    const net::CompletionCallback& callback) OVERRIDE {
+    DCHECK(write_log_);
+    write_log_->push_back(std::string(buf->data(), buf_len));
+    pending_bytes_ += buf_len;
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(callback, write_error_ == net::OK ? buf_len : write_error_));
+    return net::ERR_IO_PENDING;
+  }
+
+  virtual int Cancel(const net::CompletionCallback& callback) OVERRIDE {
+    DCHECK_EQ(net::OK, write_error_);
+    callback.Run(net::OK);

[hashimoto, 2014/09/11 07:52:17]

nit: Use PostTask?

[mtomasz, 2014/09/17 04:59:41]

Done.

+    return net::ERR_IO_PENDING;
+  }
+
+  virtual int Flush(const net::CompletionCallback& callback) OVERRIDE {
+    DCHECK(flush_log_);
+    flush_log_->push_back(pending_bytes_);
+    pending_bytes_ = 0;
+    callback.Run(net::OK);

[hashimoto, 2014/09/11 07:52:16]

ditto.

[mtomasz, 2014/09/17 04:59:40]

Done.

+    return net::ERR_IO_PENDING;
+  }
+
+ private:
+  int pending_bytes_;
+  std::vector<std::string>* write_log_;  // Not owned.
+  std::vector<int>* flush_log_;          // Not owned.
+  net::Error write_error_;
+  DISALLOW_COPY_AND_ASSIGN(FakeFileStreamWriter);
+};
+
+}  // namespace
+
+class FileSystemProviderBufferingFileStreamWriterTest : public testing::Test {
+ protected:
+  FileSystemProviderBufferingFileStreamWriterTest() {}
+  virtual ~FileSystemProviderBufferingFileStreamWriterTest() {}
+
+  content::TestBrowserThreadBundle thread_bundle_;
+};
+
+TEST_F(FileSystemProviderBufferingFileStreamWriterTest, Write) {
+  std::vector<std::string> inner_write_log;
+  std::vector<int> inner_flush_log;
+  BufferingFileStreamWriter writer(
+      scoped_ptr<storage::FileStreamWriter>(new FakeFileStreamWriter(
+          &inner_write_log, &inner_flush_log, net::OK)),
+      kIntermediateBufferLength);
+
+  scoped_refptr<net::StringIOBuffer> buffer(

[hashimoto, 2014/09/11 07:52:17]

How about having this buffer and a StringBuffer made with kLongTextToWrite as
members of the test fixture?
The same buffers are constructed in many places.

[mtomasz, 2014/09/17 04:59:41]

Good idea. Done.

+      new net::StringIOBuffer(kShortTextToWrite));
+  ASSERT_LT(kIntermediateBufferLength, 2 * buffer->size());
+
+  // Writing for the first time should succeed, but buffer the write without
+  // calling the internal file stream writer.
+  {
+    std::vector<int> write_log;
+    const int result = writer.Write(
+        buffer, buffer->size(), base::Bind(&LogValue<int>, &write_log));
+    base::RunLoop().RunUntilIdle();
+
+    EXPECT_EQ(buffer->size(), result);
+    EXPECT_EQ(0u, write_log.size());
+    EXPECT_EQ(0u, inner_write_log.size());
+    EXPECT_EQ(0u, inner_flush_log.size());
+  }
+
+  // Writing for the second time should however flush the intermediate buffer,
+  // since it is full.
+  {
+    inner_write_log.clear();
+    inner_flush_log.clear();
+
+    std::vector<int> write_log;
+    const int result = writer.Write(
+        buffer, buffer->size(), base::Bind(&LogValue<int>, &write_log));
+    base::RunLoop().RunUntilIdle();
+
+    EXPECT_EQ(net::ERR_IO_PENDING, result);
+    const std::string expected_inner_write =
+        (std::string(kShortTextToWrite) + kShortTextToWrite)
+            .substr(0, kIntermediateBufferLength);
+    ASSERT_EQ(1u, inner_write_log.size());
+    EXPECT_EQ(expected_inner_write, inner_write_log[0]);
+    ASSERT_EQ(1u, write_log.size());
+    EXPECT_EQ(buffer->size(), write_log[0]);
+    EXPECT_EQ(0u, inner_flush_log.size());
+  }
+
+  // There should be a remainder in the intermediate buffer. Calling flush
+  // should invoke a write on the internal file stream writer.
+  {
+    inner_write_log.clear();
+    inner_flush_log.clear();
+
+    std::vector<int> flush_log;
+    const int result = writer.Flush(base::Bind(&LogValue<int>, &flush_log));
+    base::RunLoop().RunUntilIdle();
+
+    EXPECT_EQ(net::ERR_IO_PENDING, result);
+    ASSERT_EQ(1u, flush_log.size());
+    EXPECT_EQ(net::OK, flush_log[0]);
+
+    const std::string expected_inner_write =
+        (std::string(kShortTextToWrite) + kShortTextToWrite)
+            .substr(kIntermediateBufferLength, std::string::npos);
+    ASSERT_EQ(1u, inner_write_log.size());
+    EXPECT_EQ(expected_inner_write, inner_write_log[0]);
+
+    const int expected_inner_flush = 2 * buffer->size();
+    ASSERT_EQ(1u, inner_flush_log.size());
+    EXPECT_EQ(expected_inner_flush, inner_flush_log[0]);
+  }
+}
+
+TEST_F(FileSystemProviderBufferingFileStreamWriterTest, Write_WithError) {
+  std::vector<std::string> inner_write_log;
+  std::vector<int> inner_flush_log;
+  BufferingFileStreamWriter writer(
+      scoped_ptr<storage::FileStreamWriter>(new FakeFileStreamWriter(
+          &inner_write_log, &inner_flush_log, net::ERR_FAILED)),
+      kIntermediateBufferLength);
+
+  scoped_refptr<net::StringIOBuffer> buffer(
+      new net::StringIOBuffer(kShortTextToWrite));
+  ASSERT_LT(kIntermediateBufferLength, 2 * buffer->size());
+
+  // Writing for the first time should succeed, but buffer the write without
+  // calling the internal file stream writer. Because of that, the error will
+  // not be generated unless the intermediate buffer is flushed.
+  {
+    std::vector<int> write_log;
+    const int result = writer.Write(
+        buffer, buffer->size(), base::Bind(&LogValue<int>, &write_log));
+    base::RunLoop().RunUntilIdle();
+
+    EXPECT_EQ(buffer->size(), result);
+    EXPECT_EQ(0u, write_log.size());
+    EXPECT_EQ(0u, inner_write_log.size());
+    EXPECT_EQ(0u, inner_flush_log.size());
+  }
+
+  // Writing for the second time should however flush the intermediate buffer,
+  // since it is full. That should generate an error.
+  {
+    inner_write_log.clear();
+    inner_flush_log.clear();
+
+    std::vector<int> write_log;
+    const int result = writer.Write(
+        buffer, buffer->size(), base::Bind(&LogValue<int>, &write_log));
+    base::RunLoop().RunUntilIdle();
+
+    EXPECT_EQ(net::ERR_IO_PENDING, result);
+    const std::string expected_inner_write =
+        (std::string(kShortTextToWrite) + kShortTextToWrite)
+            .substr(0, kIntermediateBufferLength);
+    ASSERT_EQ(1u, inner_write_log.size());
+    EXPECT_EQ(expected_inner_write, inner_write_log[0]);
+    ASSERT_EQ(1u, write_log.size());
+    EXPECT_EQ(net::ERR_FAILED, write_log[0]);
+    EXPECT_EQ(0u, inner_flush_log.size());
+  }
+}
+
+TEST_F(FileSystemProviderBufferingFileStreamWriterTest, Write_Directly) {
+  std::vector<std::string> inner_write_log;
+  std::vector<int> inner_flush_log;
+  BufferingFileStreamWriter writer(
+      scoped_ptr<storage::FileStreamWriter>(new FakeFileStreamWriter(
+          &inner_write_log, &inner_flush_log, net::OK)),
+      kIntermediateBufferLength);
+
+  scoped_refptr<net::StringIOBuffer> short_buffer(
+      new net::StringIOBuffer(kShortTextToWrite));
+  ASSERT_GT(kIntermediateBufferLength, short_buffer->size());
+
+  scoped_refptr<net::StringIOBuffer> long_buffer(
+      new net::StringIOBuffer(kLongTextToWrite));
+  ASSERT_LT(kIntermediateBufferLength, long_buffer->size());
+
+  // Write few bytes first, so the intermediate buffer is not empty.
+  {
+    std::vector<int> write_log;
+    const int result = writer.Write(short_buffer,
+                                    short_buffer->size(),
+                                    base::Bind(&LogValue<int>, &write_log));
+    base::RunLoop().RunUntilIdle();
+
+    EXPECT_EQ(short_buffer->size(), result);
+    EXPECT_EQ(0u, write_log.size());
+    EXPECT_EQ(0u, inner_flush_log.size());
+  }
+
+  // Request writing a long chunk of data, which is longr than size of the
+  // intermediate buffer.
+  {
+    inner_write_log.clear();
+    inner_flush_log.clear();
+
+    std::vector<int> write_log;
+    const int result = writer.Write(long_buffer,
+                                    long_buffer->size(),
+                                    base::Bind(&LogValue<int>, &write_log));
+    base::RunLoop().RunUntilIdle();
+
+    EXPECT_EQ(net::ERR_IO_PENDING, result);
+    ASSERT_EQ(2u, inner_write_log.size());
+    EXPECT_EQ(kShortTextToWrite, inner_write_log[0]);
+    EXPECT_EQ(kLongTextToWrite, inner_write_log[1]);
+    ASSERT_EQ(1u, write_log.size());
+    EXPECT_EQ(long_buffer->size(), write_log[0]);
+    EXPECT_EQ(0u, inner_flush_log.size());
+  }
+
+  // Write a long chunk again, to test the case with an empty intermediate
+  // buffer.
+  {
+    inner_write_log.clear();
+    inner_flush_log.clear();
+
+    std::vector<int> write_log;
+    const int result = writer.Write(long_buffer,
+                                    long_buffer->size(),
+                                    base::Bind(&LogValue<int>, &write_log));
+    base::RunLoop().RunUntilIdle();
+
+    EXPECT_EQ(net::ERR_IO_PENDING, result);
+    ASSERT_EQ(1u, inner_write_log.size());
+    EXPECT_EQ(kLongTextToWrite, inner_write_log[0]);
+    ASSERT_EQ(1u, write_log.size());
+    EXPECT_EQ(long_buffer->size(), write_log[0]);
+    EXPECT_EQ(0u, inner_flush_log.size());
+  }
+}
+
+TEST_F(FileSystemProviderBufferingFileStreamWriterTest, Cancel) {
+  std::vector<std::string> inner_write_log;
+  std::vector<int> inner_flush_log;
+  BufferingFileStreamWriter writer(
+      scoped_ptr<storage::FileStreamWriter>(new FakeFileStreamWriter(
+          &inner_write_log, &inner_flush_log, net::OK)),
+      kIntermediateBufferLength);
+
+  scoped_refptr<net::StringIOBuffer> buffer(
+      new net::StringIOBuffer(kLongTextToWrite));
+
+  // Write directly, so there is something to actually cancel. Note, that
+  // buffered writes which do not invoke flushing the intermediate buffer finish
+  // immediately, so they are not cancellable.
+  std::vector<int> write_log;
+  const int write_result = writer.Write(
+      buffer, buffer->size(), base::Bind(&LogValue<int>, &write_log));
+  EXPECT_EQ(net::ERR_IO_PENDING, write_result);
+
+  std::vector<int> cancel_log;
+  const int cancel_result =
+      writer.Cancel(base::Bind(&LogValue<int>, &cancel_log));
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(net::ERR_IO_PENDING, cancel_result);
+  ASSERT_EQ(1u, cancel_log.size());
+  EXPECT_EQ(net::OK, cancel_log[0]);

[hashimoto, 2014/09/11 07:52:17]

How about counting how many times the fake's Cancel() method was called?

[mtomasz, 2014/09/17 04:59:41]

Done.

+}
+
+TEST_F(FileSystemProviderBufferingFileStreamWriterTest, Flush) {
+  std::vector<std::string> inner_write_log;
+  std::vector<int> inner_flush_log;
+  BufferingFileStreamWriter writer(
+      scoped_ptr<storage::FileStreamWriter>(new FakeFileStreamWriter(
+          &inner_write_log, &inner_flush_log, net::OK)),
+      kIntermediateBufferLength);
+
+  scoped_refptr<net::StringIOBuffer> buffer(
+      new net::StringIOBuffer(kShortTextToWrite));
+
+  // Write less bytes than size of the intermediate buffer.
+  std::vector<int> write_log;
+  const int write_result = writer.Write(
+      buffer, buffer->size(), base::Bind(&LogValue<int>, &write_log));
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(buffer->size(), write_result);
+  EXPECT_EQ(0u, write_log.size());
+  EXPECT_EQ(0u, inner_write_log.size());
+  EXPECT_EQ(0u, inner_flush_log.size());
+
+  // Calling Flush() will force flushing the intermediate buffer before it's
+  // filled out.
+  std::vector<int> flush_log;
+  const int flush_result = writer.Flush(base::Bind(&LogValue<int>, &flush_log));
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(net::ERR_IO_PENDING, flush_result);
+  ASSERT_EQ(1u, flush_log.size());
+  EXPECT_EQ(net::OK, flush_log[0]);
+
+  ASSERT_EQ(1u, inner_write_log.size());
+  EXPECT_EQ(kShortTextToWrite, inner_write_log[0]);
+
+  ASSERT_EQ(1u, inner_flush_log.size());
+  EXPECT_EQ(buffer->size(), inner_flush_log[0]);
+}
+
+TEST_F(FileSystemProviderBufferingFileStreamWriterTest, Flush_AfterWriteError) {
+  std::vector<std::string> inner_write_log;
+  std::vector<int> inner_flush_log;
+  BufferingFileStreamWriter writer(
+      scoped_ptr<storage::FileStreamWriter>(new FakeFileStreamWriter(
+          &inner_write_log, &inner_flush_log, net::ERR_FAILED)),
+      kIntermediateBufferLength);
+
+  scoped_refptr<net::StringIOBuffer> buffer(
+      new net::StringIOBuffer(kShortTextToWrite));
+
+  // Write less bytes than size of the intermediate buffer. This should succeed
+  // since the inner file stream writer is not invoked.
+  std::vector<int> write_log;
+  const int write_result = writer.Write(
+      buffer, buffer->size(), base::Bind(&LogValue<int>, &write_log));
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(buffer->size(), write_result);
+  EXPECT_EQ(0u, write_log.size());
+  EXPECT_EQ(0u, inner_write_log.size());
+  EXPECT_EQ(0u, inner_flush_log.size());
+
+  // Calling Flush() will force flushing the intermediate buffer before it's
+  // filled out. That should cause failing.
+  std::vector<int> flush_log;
+  const int flush_result = writer.Flush(base::Bind(&LogValue<int>, &flush_log));
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(net::ERR_IO_PENDING, flush_result);
+  ASSERT_EQ(1u, flush_log.size());
+  EXPECT_EQ(net::ERR_FAILED, flush_log[0]);
+
+  ASSERT_EQ(1u, inner_write_log.size());
+  EXPECT_EQ(kShortTextToWrite, inner_write_log[0]);
+
+  // Flush of the internal file stream writer is not invoked, since a Write
+  // method invocation fails before it.
+  EXPECT_EQ(0u, inner_flush_log.size());
+}
+
+}  // namespace file_system_provider
+}  // namespace chromeos