[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..d8bbb9a297e8549c2181f58c069fa30123388097
--- /dev/null
+++ b/chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc
@@ -0,0 +1,292 @@
+// 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"
+#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;
+
+// Behaviour tested by a test case.
+enum TestMode { SYNCHRONOUS, ASYNCHRONOUS };
+
+// Logs invocations on the fake file stream reader.
+class Logger {
+ public:
+  Logger() : weak_ptr_factory_(this) {}
+  virtual ~Logger() {}
+
+  // Registers calls to the internal file stream reader, for the Read() method.
+  void OnInnerReadRequested(int buf_len) {
+    inner_read_requested_events_.push_back(buf_len);
+  }
+
+  // Registers results returned asynchronously by the buffering stream reader,
+  // for the Read() method.
+  void OnReadResult(int result) { read_result_events_.push_back(result); }
+
+  // Registers eresults returned asynchronously by the buffering strem reader,

[hashimoto, 2014/06/05 03:18:36]

nit: s/eresults/results/?

[mtomasz, 2014/06/05 09:22:27]

Done.

+  // for the GetLength() method.
+  void OnGetLengthResult(int64 result) {
+    get_length_result_events_.push_back(result);
+  }
+
+  // Gets a weak pointer for asynchronous callback invocations on the logger.
+  base::WeakPtr<Logger> GetWeakPtr() { return weak_ptr_factory_.GetWeakPtr(); }

[hashimoto, 2014/06/05 03:18:36]

No need to have this method?
I don't see any danger in using base::Unretained as Logger always lives longer
than the reader.

[mtomasz, 2014/06/05 09:22:27]

We are passing it to the BufferingFileStreamReader, which can call it back with
a PostTask. Using Unretained wouldn't crash, only because we (1) call
RunLoop::RunUntilIdle and (2) the current TestBrowserThreadBundle creates fake
threads backed up with the same message loop. If we had real threads, then this
could crash (IIUC). I think having a weak ptr is just a safe choice, since its
very simple and will always work fine. It doesn't add much overhead either.
WDYT?

[hashimoto, 2014/06/05 10:07:10]

I think it's OK to take advantage of the single threaded test and using WeakPtr
looks like a kind of overkill.
Also, we can even stop Logger for recording results like below.
(the only exception is recording the method calls for the internal reader)

void PushResultToVector(std::vector<int>* out, int result) {
  out->push_back(result);
}

TEST(...) {
  std::vector<int> results;
  reader.Read(buffer, kChunkSize, &base::Bind(&PushResultToVector, &results));
}

Anyways, it's up to you.

[mtomasz, 2014/06/06 01:31:03]

Even if all threads are backed by the same message loop, I think it is not safe
to use base::Unretained. It may happen that tested classes (1) store the
callback and run it via PostTask in their destructor, *or* (2) we forget to run
RunLoopUntilIdle(). As a result, a task will be added to the message loop, and
executed *after* the test case is finished. When TestBrowserThreadBundle is
destroyed, all message loops are drained, and then we would get a crash:
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes...

This is not likely to happen in this case, but in general I think we can't
safely assume that we can use base::Unretained in unit tests in a single
threaded environment. Saying that, always using a weak pointer gives us a
guarantee that it will always work, and never crash, whatever is the
implementation of tested classes, and whether we forget to call RunLoop() or
not. And it costs us only 1 extra line of code.

[hashimoto, 2014/06/06 03:36:52]

(1) Here we know that doesn't happen with the tested class.
(2) To report what happened, tests should behave nicely even when the tested
class behaves badly, but forgetting to call Run() is a bug of the test itself. 
There is no point to add another complexity to be robust against badly
implemented tests.
Not only one line, but also the vectors and base::Bind.
Basically, you are reinventing TestCopmletionCallback.

[mtomasz, 2014/06/06 04:22:56]

Your suggestion also uses vectors and base::Bind, but without the class wrapper.
I can't use TestCompletionCallback, since it doesn't register multiple calls.
Saying that, I don't think having a logger class is such a big overhead. It is
self documenting and wrapping all of the logging callbacks and vectors in one
place.

If you don't have a strong feeling about this, I'd like to go with the current
solution, as all of the tests in chrome/browser/file_system_provider use the
same pattern.

[hashimoto, 2014/06/09 11:12:04]

I still don't understand why you need this class:
 1. When an unexpected call is made against the callback, it's better to get
notified about it with a crash instead of the call being silently ignored by
WeakPtr. (A crash in a test is worse than a graceful failure because it aborts
the test process, but better than silently dismissing hidden failures.)
 2. Too many things are repeated. 3 std::vectors, 3 same-looking methods to push
the result, 3 getters, lengthy base::Bind everywhere.  This class is basically a
reinvention of TestCompletionCallback in a less sophisticated way.
 3. TestCompletionCallback is used much more widely in the entire code base.

Anyways, if you believe this class is necessary, I'm OK with this code at least
it works and I'm not the engineer who is responsible to maintain this code until
the end of its lifetime.

[mtomasz, 2014/06/09 13:51:15]

I think we are talking about two different things. IIUC you want to use
TestCompletionCallback here. But we can't do that, as I said before. In case of
line #211, we are checking the second call.

If we can't use TestCompletionCallback, then we would have to use suggested by
you PushResultToVector, with local vectors. Do you suggest to use
TestCompletionCallback for callbacks which we assume are called once, and
PushResultToVector for callbacks which do not have such assumption? We would
save some lines of code, but isn't having a simple Logger class just neater and
cleaner? WDYT?

As for the weak pointer, I'll remove it. Catching the crash over ignoring sounds
good to me.

[hashimoto, 2014/06/10 10:43:24]

Sorry, I forgot to note that inner_read_requested_events_ is the exception.
This vector is serving for a totally different purpose while other vectors can
be easily replaced with TestCompletionCallback.
The problem of this class's design is that there is no merit to have
read_result_events_ and get_length_result_events_ in this class.
These two vectors and related methods can be replaced with
TestCompletionCallback or local variables and free functions without any
problems.
Is there any reason with which you need to put the vector for recording the
arguments of function calls made against the inner reader and the vectors for
recording the result of function calls made against the external reader?

[mtomasz, 2014/06/10 12:23:55]

1. inner_read_requested_events_ can't be replaced. We want to check if it is
called only once. TestCompletionCallback doesn't allow that.
2. read_result_events can't be replaced - called more than once. In #211.
3. get_length_result_events could be replaced - called up to once, but I think
it is better to check if it is not called more times than expected.

So, IIUC, at most one of these three vectors can be replaced by
TestCompletionCallback.
Note, that I'm going to implement adaptive chunk size very soon, as written in
the TODO.
After doing that, we will have more complex test cases, which would require
multiple callback calls.

I can split the class to separate utility methods like you suggested before, and
declare vectors locally. That would however end up on declaring the same vectors
in several test cases. I'm not convinced it is more readable. But let's give it
a try. 
I'm not sure if I understand. We should test if the external reader calls the
internal reader properly. Eg. we want to know if it was called with correct
arguments (eg. here with larger buffer length).

Also, I'm not recording calls made against the external reader. I'm recording
the external Read and GetLength callback calls to know if they return correct
value.

Please clarify.

[hashimoto, 2014/06/11 06:01:03]

You can do it because you are passing a new callback made by Bind for each
method call.
You can use TestCompletionCallback multiple times by passing a callback returned
by TestCompletionCallback::callback() for each method call.

If the test becomes more complex with this "logger", you'll end up having
something like:

...
ASSERT_EQ(5u, logger.xxx().size());
EXPECT_EQ(kAAA, logger.xxx()[4]); 
[Do something]
ASSERT_EQ(6u, logger.xxx().size());
EXPECT_EQ(kBBB, logger.xxx()[5]);
[Do something] 
ASSERT_EQ(7u, logger.xxx().size());
EXPECT_EQ(kCCC, logger.xxx()[6]);
...

The problem is that when you want to remove just one of these steps, you also
have to rewrite all the following steps to fix indices.
I'm still not sure why you chose this design among the all possible solutions.
You can use TestCompletionCallback, or implement a subclass of
TestCompletionCallback which overrides SetResult() to check the callback doesn't
get called twice (if you really care about it), or do anything else.
Anyways, I'm OK with this "logger" as long as you will be responsible to
maintain this code as I said.

[mtomasz, 2014/06/12 04:10:09]

I made the change as you suggested for comparison.
https://codereview.chromium.org/334593003/diff2/1:20001/chrome/browser/chrome...

Please note how tricky the new code is. We can't use GetResult() in some places
since it could cause a freeze (if the callback is not called but was expected).
We need to check for have_result() or called_times() before comparing the
result, since it would be undefined (sic!).

In comparison, the original code is very simple and very easy to understand. No
tricks at all. And after all, shorter.

+
+  const std::vector<int>& inner_read_requested_events() const {
+    return inner_read_requested_events_;
+  }
+
+  const std::vector<int>& read_result_events() const {
+    return read_result_events_;
+  }
+
+  const std::vector<int64>& get_length_result_events() const {
+    return get_length_result_events_;
+  }
+
+ private:
+  std::vector<int> inner_read_requested_events_;
+  std::vector<int> read_result_events_;
+  std::vector<int64> get_length_result_events_;
+
+  base::WeakPtrFactory<Logger> weak_ptr_factory_;
+  DISALLOW_COPY_AND_ASSIGN(Logger);
+};
+
+// Fake internal file stream reader.
+class FakeFileStreamReader : public webkit_blob::FileStreamReader {
+ public:
+  FakeFileStreamReader(Logger* logger, TestMode mode, bool return_error)
+      : logger_(logger), mode_(mode), return_error_(return_error) {}
+  virtual ~FakeFileStreamReader() {}
+
+  // webkit_blob::FileStreamReader overrides.
+  virtual int Read(net::IOBuffer* buf,
+                   int buf_len,
+                   const net::CompletionCallback& callback) OVERRIDE {
+    logger_->OnInnerReadRequested(buf_len);
+
+    if (return_error_) {
+      if (mode_ == SYNCHRONOUS)
+        return net::ERR_ACCESS_DENIED;
+
+      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);
+
+    if (mode_ == SYNCHRONOUS)
+      return 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 {
+    if (mode_ == SYNCHRONOUS)
+      return kFileSize;
+
+    base::MessageLoopProxy::current()->PostTask(
+        FROM_HERE, base::Bind(callback, kFileSize));
+    return net::ERR_IO_PENDING;
+  }
+
+ private:
+  Logger* logger_;  // Not owned.
+  TestMode mode_;
+  bool return_error_;
+  DISALLOW_COPY_AND_ASSIGN(FakeFileStreamReader);
+};
+
+}  // namespace
+
+class FileSystemProviderBufferingFileStreamReaderTest
+    : public testing::Test,
+      public ::testing::WithParamInterface<TestMode> {
+ protected:
+  FileSystemProviderBufferingFileStreamReaderTest() {}
+  virtual ~FileSystemProviderBufferingFileStreamReaderTest() {}
+
+  content::TestBrowserThreadBundle thread_bundle_;
+};
+
+TEST_P(FileSystemProviderBufferingFileStreamReaderTest, Read) {
+  Logger logger;
+  BufferingFileStreamReader reader(
+      scoped_ptr<webkit_blob::FileStreamReader>(new FakeFileStreamReader(
+          &logger, GetParam(), 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));
+    const int result =
+        reader.Read(buffer,
+                    kChunkSize,
+                    base::Bind(&Logger::OnReadResult, logger.GetWeakPtr()));

[hashimoto, 2014/06/05 03:18:36]

How about using TestCompletionCallback?
The same goes for other places passing Logger methods as callbacks.

[mtomasz, 2014/06/05 09:22:27]

With TestCompletionCallback we can't control how many times the callback has
been called.

[hashimoto, 2014/06/06 03:36:52]

What do you mean?
TestCompletionCallback::WaitForResult() quits the message loop once the callback
gets called.

[mtomasz, 2014/06/06 04:22:56]

If the tested class by accident calls the callback multiple times instead of
one, then we won't catch this error with TestCompletionCallback.

Such error would happen, if we had an error in:
https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/...

GOOD:
79   if (result != net::ERR_IO_PENDING) {

BAD:
79   if (result == net::ERR_IO_PENDING) {

Using TestCompletionCallback would not let us catch this serious (and easy to
make) mistake.

[hashimoto, 2014/06/09 11:12:04]

At least, we can catch it because it crashes as you described.
This can be caught by the synchronous version tests.

BTW, why do you need to have a code path and parameterized tests to handle
results returned synchronously?
Doesn't providing extensions always return the result asynchronously?

[mtomasz, 2014/06/09 13:51:15]

It would crash. But I don't think crashing is better that detecting it with an
assert.
Also, we are checking the second result in some places. With
TestCompletionCallback we can't do that.
The BufferingFileStreamReader works with any webkit_blob::FileStreamReader, and
it has zero dependency on the underlying file stream reader. It makes it simple.

We can of course remove the synchronous code paths, and add dependency and
assumptions on the implementation of the provided file system reader.
Surprisingly, we would get 2 extra DCHECKS and remove only 3 lines of code. Is
it really worth?

[hashimoto, 2014/06/10 10:43:24]

You cannot prepare for every possible failure which can be caused by the tested
class.
What if, say, the tested class posts a task to base::WorkerPool and run the
callback on a different thread than the main thread?
Anyways, as I said, I'm OK with this "logger" as long as you take the
responsibility to maintain this code.
Didn't you say that only one single "if" statement can be a prone of bugs?
Also, please don't forget about the complicated test code and TEST_P used here
which often degrades the readability of test failure log quite much.

[mtomasz, 2014/06/10 12:23:55]

I cannot test everything, but I'm not sure if it is an argument to reduce test
coverage.
BufferingFileStreamReader is a thin wrapper. Supporting sync features doesn't
add much complexity. Removing sync support would add DCHECKs and possible
undefined behaviour in the future, if the inner reader returns something
synchronously.

I always prefer to avoid writing code which is not used, but here in the thin
wrapper, it seems just less bug prone to support all features than cut out some
of them. In return we are getting zero dependency, which is a good thing.

I don't know about log degradation when using TEST_P. Test case names are very
neat. Synchronous/FileSystemProviderBufferingFileStreamTest.Read, etc.

[hashimoto, 2014/06/11 06:01:03]

This doesn't affect the test coverage (i.e. how much code of the tested class is
actually covered by the test cases).
Just the simplicity of the test code itself.
The existence of this buffer reader itself is depending on a huge assumption.
It can be justified to have this buffering mechanism only if the actual read
operation is quite slow so that the cost for the redundant data copy becomes
negligible.
Anyways, if you think having this unused code is acceptable, I'm OK with it as
long as you will be responsible to maintain it.

[mtomasz, 2014/06/16 04:12:17]

It affects how detailed the tests are. IMHO tests should be precise if possible.
If we can *easily* test that the callback is not called more times than
expected, then we should do that.

Argument for losing precision just in order to be able to use
TestCompletionCallback doesn't hold water for me. If we lose precision, then it
means that TestCompletionCallback is not a correct tool for these tests.
According to the benchmarks I've shared with you in the caching plans document,
an extra copy here doesn't make any significant difference here.

[hashimoto, 2014/06/19 07:27:45]

TestCompletionCallback is widely used and it makes the test body short and easy
to understand for someone not familiar to this code.
How easy it is to understand existing tests, to add new tests, and to fix broken
tests is quite important part of the quality of the test.
Also, if you really care about this problem, you can create a subclass of
TestCompletionCallback to have that check as I said in
https://codereview.chromium.org/334593003/.
What I meant was that when you cannot make some assumption on the code owned by
you, how can you make assumption on the code now owned by you.  Extension/IPC
system eventually may get faster in future.

[mtomasz, 2014/06/20 04:20:23]

TestCompletionCallback makes the code nice and short but has side effects. As I
said before, it causes (a) loss of precision and (b) timing out instead of
asserts/expects. If we want to avoid (a) and (b), we can still use
TestCompletionCallback, but the code gets complicated, and difficult to
maintain.


So, we have three options:

1. Keep Logger, hence detailed and never timeouting tests. The code is very
simple and clean, hence easy to maintain.

2. Use TestCompletionCallback and lose precision (a) + have timing out unit
tests instead of asserts/expects if a callback is not called (b). The code is
simple and clean but with low precision, and we can't use it for the inner
reader. For the inner reader we still need to have some kind of a logger. So we
have also inconsistency.

3. Use TestCompletionCallback with overriding it + using tricks to avoid (a) and
(b). Complex and difficult code to maintain. We can't use it for the inner
reader, so we need some logger as in (2) additionally. In total, the code may be
longer than (1).

Since we already have the logger, I don't understand why are you pushing on
using either (2) or (3), where both seem to be more problematic than (1).
Could you clarify? Why can't I make assumptions on the code owned by me?

+    base::RunLoop().RunUntilIdle();
+
+    EXPECT_EQ(net::ERR_IO_PENDING, result);
+    ASSERT_EQ(1u, logger.inner_read_requested_events().size());
+    EXPECT_EQ(kBufferSize, logger.inner_read_requested_events()[0]);
+    ASSERT_EQ(1u, logger.read_result_events().size());
+    EXPECT_EQ(kChunkSize, logger.read_result_events()[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));
+    const int result =
+        reader.Read(buffer,
+                    kChunkSize,
+                    base::Bind(&Logger::OnReadResult, logger.GetWeakPtr()));
+    base::RunLoop().RunUntilIdle();
+
+    EXPECT_EQ(kChunkSize, result);
+    EXPECT_EQ(1u, logger.inner_read_requested_events().size());
+    // Results returned synchronously, so no new read result events.
+    EXPECT_EQ(1u, logger.read_result_events().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));
+    const int result =
+        reader.Read(buffer,
+                    kChunkSize,
+                    base::Bind(&Logger::OnReadResult, logger.GetWeakPtr()));
+    base::RunLoop().RunUntilIdle();
+
+    EXPECT_EQ(kBufferSize - 2 * kChunkSize, result);
+    EXPECT_EQ(1u, logger.inner_read_requested_events().size());
+    // Results returned synchronously, so no new read result events.
+    EXPECT_EQ(1u, logger.read_result_events().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));
+    const int result =
+        reader.Read(buffer,
+                    kChunkSize,
+                    base::Bind(&Logger::OnReadResult, logger.GetWeakPtr()));
+    base::RunLoop().RunUntilIdle();
+
+    EXPECT_EQ(net::ERR_IO_PENDING, result);
+    ASSERT_EQ(2u, logger.inner_read_requested_events().size());
+    EXPECT_EQ(kBufferSize, logger.inner_read_requested_events()[0]);
+    ASSERT_EQ(2u, logger.read_result_events().size());
+    EXPECT_EQ(kChunkSize, logger.read_result_events()[1]);
+  }
+}
+
+TEST_P(FileSystemProviderBufferingFileStreamReaderTest,
+       Read_MoreThanBufferSize) {
+  Logger logger;
+  BufferingFileStreamReader reader(
+      scoped_ptr<webkit_blob::FileStreamReader>(new FakeFileStreamReader(
+          &logger, GetParam(), false /* return_error */)),
+      kBufferSize);
+
+  // 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));
+  const int result =
+      reader.Read(buffer,
+                  chunk_size,
+                  base::Bind(&Logger::OnReadResult, logger.GetWeakPtr()));
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(net::ERR_IO_PENDING, result);
+  ASSERT_EQ(1u, logger.inner_read_requested_events().size());
+  EXPECT_EQ(kBufferSize, logger.inner_read_requested_events()[0]);
+  ASSERT_EQ(1u, logger.read_result_events().size());
+  EXPECT_EQ(kBufferSize, logger.read_result_events()[0]);
+}
+
+TEST_P(FileSystemProviderBufferingFileStreamReaderTest, Read_WithError) {
+  Logger logger;
+  BufferingFileStreamReader reader(
+      scoped_ptr<webkit_blob::FileStreamReader>(new FakeFileStreamReader(
+          &logger, GetParam(), true /* return_error */)),
+      kBufferSize);
+
+  scoped_refptr<net::IOBuffer> buffer(new net::IOBuffer(kChunkSize));
+  const int result =
+      reader.Read(buffer,
+                  kChunkSize,
+                  base::Bind(&Logger::OnReadResult, logger.GetWeakPtr()));
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(net::ERR_IO_PENDING, result);
+  ASSERT_EQ(1u, logger.inner_read_requested_events().size());
+  EXPECT_EQ(kBufferSize, logger.inner_read_requested_events()[0]);
+  ASSERT_EQ(1u, logger.read_result_events().size());
+  EXPECT_EQ(net::ERR_ACCESS_DENIED, logger.read_result_events()[0]);
+}
+
+TEST_P(FileSystemProviderBufferingFileStreamReaderTest, GetLength) {
+  Logger logger;
+  BufferingFileStreamReader reader(
+      scoped_ptr<webkit_blob::FileStreamReader>(new FakeFileStreamReader(
+          &logger, GetParam(), false /* return_error */)),
+      kBufferSize);
+
+  const int64 result = reader.GetLength(
+      base::Bind(&Logger::OnGetLengthResult, logger.GetWeakPtr()));
+  base::RunLoop().RunUntilIdle();
+
+  if (GetParam() == SYNCHRONOUS) {
+    EXPECT_EQ(kFileSize, result);
+    EXPECT_EQ(0u, logger.get_length_result_events().size());
+  } else {
+    EXPECT_EQ(net::ERR_IO_PENDING, result);
+    ASSERT_EQ(1u, logger.get_length_result_events().size());
+    EXPECT_EQ(kFileSize, logger.get_length_result_events()[0]);
+  }
+}
+
+INSTANTIATE_TEST_CASE_P(Synchronous,
+                        FileSystemProviderBufferingFileStreamReaderTest,
+                        testing::Values(SYNCHRONOUS));
+
+INSTANTIATE_TEST_CASE_P(Asynchronous,
+                        FileSystemProviderBufferingFileStreamReaderTest,
+                        testing::Values(ASYNCHRONOUS));
+
+}  // namespace file_system_provider
+}  // namespace chromeos