Add net::SourceStream and net::FilterSourceStream

net/filter/mock_source_stream.cc

+// Copyright 2015 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 "net/filter/mock_source_stream.h"
+
+#include "base/logging.h"
+#include "net/base/io_buffer.h"
+
+namespace net {
+
+MockSourceStream::MockSourceStream()
+    : SourceStream(SourceStream::TYPE_NONE),
+      awaiting_completion_(false),
+      dest_buffer_(nullptr),
+      dest_buffer_size_(0) {}
+
+MockSourceStream::~MockSourceStream() {
+  DCHECK(results_.empty());
+}
+
+int MockSourceStream::Read(IOBuffer* dest_buffer,
+                           int buffer_size,
+                           const CompletionCallback& callback) {
+  DCHECK(!awaiting_completion_);
+  DCHECK(!results_.empty());

[Randy Smith (Not in Mondays), 2016/08/22 22:37:16]

I'd vote gently against these DCHECKs.  My rule of thumb is that DCHECKs in
testing code are for testing to see if unexpected things are happening in the
*testing* code.  If the code under test could conceivably cause something to
happen, even if that reflects a bug in the code under test, the result should be
a test failure, not a crash.  As I understand this code, if a downstream filter
calls read more times than it should, or calls it without waiting for the
callback, this could happen.  So if MockSourceStream could be used to test real
downstream filters, I'd think it shouldn't have these DCHECKs but instead
surface these errors gracefully.  That should probably include setting an error
indicator on the MockSourceStream that test code can check, since you can't rely
on code under test to pass through errors.

[mmenke, 2016/08/22 22:46:42]

I tend to prefer this, just because if something unexpected happens at anything
below the test fixture, tests can hang.  However, if we want to go that route,
I'd use EXPECT/ASSERT here (Maybe along with returning an error code
synchronously) and Test::HasFailure() in the test body...But that seems really
flaky to me, to the point that checking for failures in the test body at every
point that could have reached this code really isn't worth the effort.  Code
clarity matters more than not hanging, for test code.

[xunjieli, 2016/08/29 16:25:29]

Acknowledged. Thanks for the suggestions and the different opinions!  The parts
on test hanging and the need to check HasFailure() in the test are excellent
points. Based my experience with this code for the past few weeks, I agree that
test crash is much easier to diagnose compared to test hanging. I will keep the
DCHECK as it is.

+
+  if (results_.empty())
+    return ERR_UNEXPECTED;
+
+  QueuedResult r = results_.front();
+  DCHECK_GE(buffer_size, r.len);

[Randy Smith (Not in Mondays), 2016/08/22 22:37:17]

Same comment here--this could be the result of an error in the code under test
(I believe?) and hence should probably not be a DCHECK.

[xunjieli, 2016/08/29 16:25:29]

Acknowledged.

+  if (r.mode == ASYNC) {
+    awaiting_completion_ = true;
+    dest_buffer_ = dest_buffer;
+    dest_buffer_size_ = buffer_size;
+    callback_ = callback;
+    return ERR_IO_PENDING;
+  }
+
+  results_.pop();
+  memcpy(dest_buffer->data(), r.data, r.len);
+  return r.error == OK ? r.len : r.error;
+}
+
+std::string MockSourceStream::OrderedTypeStringList() const {
+  return "";
+}
+
+MockSourceStream::QueuedResult::QueuedResult(const char* data,
+                                             int len,
+                                             Error error,
+                                             Mode mode)
+    : data(data), len(len), error(error), mode(mode) {}
+
+void MockSourceStream::AddReadResult(const char* data,
+                                     int len,
+                                     Error error,
+                                     Mode mode) {
+  QueuedResult result(data, len, error, mode);
+  results_.push(result);
+}
+
+void MockSourceStream::CompleteNextRead() {
+  DCHECK(awaiting_completion_);

[Randy Smith (Not in Mondays), 2016/08/22 22:37:16]

And here, unless its part of the contract that the testing code should always
test to make sure that this object is awaiting completion before calling
CompleteNextRead() (in which case that should be documented in the header file).

[xunjieli, 2016/08/29 16:25:29]

Done. I added documentation in the header file.

+
+  awaiting_completion_ = false;
+  QueuedResult r = results_.front();
+  DCHECK_EQ(ASYNC, r.mode);
+  results_.pop();
+  DCHECK_GE(dest_buffer_size_, r.len);
+  memcpy(dest_buffer_->data(), r.data, r.len);
+  dest_buffer_ = nullptr;
+  callback_.Run(r.error == OK ? r.len : r.error);
+}
+
+}  // namespace net