Plumb network stack information about existence of cached copy

net/http/test_network_transaction.cc

Index: net/http/test_network_transaction.cc
diff --git a/net/http/test_network_transaction.cc b/net/http/test_network_transaction.cc
new file mode 100644
index 0000000000000000000000000000000000000000..e875ae03221e06cada99563d9fd0471650b0e0af
--- /dev/null
+++ b/net/http/test_network_transaction.cc
@@ -0,0 +1,134 @@
+// Copyright (c) 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 "net/http/test_network_transaction.h"
+
+#include "base/bind.h"
+#include "base/logging.h"
+#include "base/memory/scoped_ptr.h"

[mmenke, 2014/01/23 20:40:52]

This is needed in the header file, rather than here.

[Randy Smith (Not in Mondays), 2014/01/23 22:12:33]

Done.

+#include "base/message_loop/message_loop.h"
+#include "net/base/load_timing_info.h"
+#include "net/base/upload_progress.h"
+#include "net/http/http_cache.h"

[mmenke, 2014/01/23 20:40:52]

Don't need this, though may want to forward declare it in the header.

[Randy Smith (Not in Mondays), 2014/01/23 22:12:33]

Done.

+#include "net/url_request/url_request_context_getter.h"

[mmenke, 2014/01/23 20:40:52]

Not needed

[Randy Smith (Not in Mondays), 2014/01/23 22:12:33]

Done.

+
+namespace net {
+
+TestNetworkTransaction::TestNetworkTransaction(enum Error err) : err_(err) {
+  DCHECK_NE(OK, err);

[mmenke, 2014/01/23 20:59:19]

This should be a CHECK, so it surfaces during release builds as well.

Also should probably be a CHECK_LT(err, OK); (Or GT, if you prefer - I think LE
is a little more intuitive, but up to you).

[mmenke, 2014/01/23 20:59:19]

Should have a similar check on creation of the factory.  Makes for a clearer
stack trace.

[Randy Smith (Not in Mondays), 2014/01/23 22:12:33]

Done.
Yeah, I've seen people argue for constant first in these comparisons, and it
seems to be our predominant idiom, so I generally do that, even though it looks
a bit weird to me (i.e. I'm doing GT).

[Randy Smith (Not in Mondays), 2014/01/23 22:12:33]

Quite right.  Done.

[rvargas (doing something else), 2014/01/23 23:24:49]

I would actually argue for readability as opposed to constant first. Constant
first applies only to test macros (EXPECT_).

But I'm actually curious about why preferring GT or LT vs NE; it seems like the
only value that we don't want here is OK so IMO we should check against that.

[mmenke, 2014/01/23 23:27:18]

Positive values generally indicate success.  What happens when a function that
returns OK success and a negative value on failure instead returns a positive
value seems like something we should avoid worrying about, hence we shouldn't
allow positive values here.

[rvargas (doing something else), 2014/01/24 02:10:00]

That doesn't match the definition of Start (the method that is supposed to
return this error code). Start returns a plain error code not error code |
bytes.

[rvargas (doing something else), 2014/01/24 16:18:42]

I guess what I said doesn't make a lot of sense, so let me try again:

If this checks that from the error codes that Start can return the caller uses a
failure one, then EQ looks better to me. If we also want to check that the
caller is not using a return code that doesn't make any sense at all, then LT
makes sense, but reading that code trows me off as to what is the intention of
the check if an explicit error code is used. In that case, I think LT(error, 0)
is a better check for "this is an error condition".

Also, this would be a programmer error (improper use of the interface) so I
think it should be a DCHECK, not a CHECK... we want to fail tests, not crash
them (more of a philosophical point because in practice release bots enable
DCHECKs).

[Randy Smith (Not in Mondays), 2014/01/25 21:11:31]

With regard to the ordering, I'm happy to yield to reviewer preference to put
the variable first.

With regard to LT verse NE I'm going to say that leaving theoretical arguments
aside, I want to catch the largest category of inputs that don't make sense
given the intent of the class, and use LT.

With regard to DCHECK vs. CHECK, I'm going to say that given that the release
bots enable DCHECK, it doesn't actually matter.  In the context of it not
mattering, I'm going to go with DCHECK, as my internal sense is that CHECK
should only be used in cases where you're clear it's preferable that chrome
provide the horrible UE of crashing rather than trying to limp along in some
fashion (even though this code will never show up in user run code).  However, I
don't really care; I'm just picking one of the options, and if one of you cares
strongly and the other doesn't, I'm happy to go with that person.  

If it turns out that the release bots didn't enable DCHECK, I'd use CHECK.

+}
+
+TestNetworkTransaction::~TestNetworkTransaction() {}
+
+int TestNetworkTransaction::Start(const HttpRequestInfo* request_info,
+                                  const CompletionCallback& callback,
+                                  const BoundNetLog& net_log)  {
+  base::MessageLoop::current()->PostTask(
+      FROM_HERE, base::Bind(callback, err_));
+  return ERR_IO_PENDING;
+}
+
+int TestNetworkTransaction::RestartIgnoringLastError(
+    const CompletionCallback& callback)  {
+  return ERR_FAILED;
+}
+
+int TestNetworkTransaction::RestartWithCertificate(
+    X509Certificate* client_cert,
+    const CompletionCallback& callback)  {
+  return ERR_FAILED;
+}
+
+int TestNetworkTransaction::RestartWithAuth(
+    const AuthCredentials& credentials,
+    const CompletionCallback& callback)  {
+  return ERR_FAILED;
+}
+
+bool TestNetworkTransaction::IsReadyToRestartForAuth()  {
+  return false;
+}
+
+int TestNetworkTransaction::Read(IOBuffer* buf, int buf_len,
+                                 const CompletionCallback& callback)  {
+  NOTREACHED();
+  return ERR_FAILED;
+}
+
+void TestNetworkTransaction::StopCaching()  {}
+
+bool TestNetworkTransaction::GetFullRequestHeaders(
+    HttpRequestHeaders* headers) const  {
+  return false;
+}
+
+int64 TestNetworkTransaction::GetTotalReceivedBytes() const  {
+  return 0;
+}
+
+void TestNetworkTransaction::DoneReading()  {
+  NOTREACHED();
+}
+
+const HttpResponseInfo* TestNetworkTransaction::GetResponseInfo() const  {
+  return NULL;
+}
+
+LoadState TestNetworkTransaction::GetLoadState() const  {
+  return LOAD_STATE_IDLE;
+}
+
+UploadProgress TestNetworkTransaction::GetUploadProgress() const  {
+  return UploadProgress();
+}
+
+bool TestNetworkTransaction::GetLoadTimingInfo(
+    LoadTimingInfo* load_timing_info) const  {
+  // Shouldn't be relevant; using the minimal set from
+  // http_transaction_unittest.cc MockNetworkTransaction::GetLoadTimingInfo().
+  load_timing_info->socket_reused = true;
+  load_timing_info->send_start = base::TimeTicks::Now();
+  load_timing_info->send_end = base::TimeTicks::Now();

[mmenke, 2014/01/23 20:40:52]

Suggest just:

*load_timing_info = LoadTimingInfo();

[Randy Smith (Not in Mondays), 2014/01/23 22:12:33]

I'm wary of doing that, just because I don't know how the load timing info is
used, and it wouldn't match the effect of
MockNetworkTransaction::GetLoadTimingInfo() (socket_reused would be false). 
Ricardo, do you have an opinion?

[mmenke, 2014/01/23 22:30:14]

It's for HAR file generation, the navigation timing API, and the resource timing
API.  Actually, we can just do nothing and return false, since we never go a
stream (Real HttpNetworkTransactions do that, too).

[Randy Smith (Not in Mondays), 2014/01/25 21:11:31]

returning false.  

+  return true;

[rvargas (doing something else), 2014/01/23 23:24:49]

don't do anything and return false?

[Randy Smith (Not in Mondays), 2014/01/25 21:11:31]

Done.

+}
+
+void TestNetworkTransaction::SetPriority(RequestPriority priority)  {}
+
+void TestNetworkTransaction::SetWebSocketHandshakeStreamCreateHelper(
+    WebSocketHandshakeStreamBase::CreateHelper* create_helper)  {
+  NOTREACHED();
+}
+
+void TestNetworkTransaction::SetBeforeNetworkStartCallback(
+    const BeforeNetworkStartCallback& callback)  {
+}
+
+int TestNetworkTransaction::ResumeNetworkStart()  {
+  NOTREACHED();
+  return ERR_FAILED;
+}
+
+TestNetworkTransactionFactory::TestNetworkTransactionFactory(
+    enum Error err) : err_(err) {}
+
+TestNetworkTransactionFactory::~TestNetworkTransactionFactory() {}
+
+// HttpTransactionFactory:
+int TestNetworkTransactionFactory::CreateTransaction(
+    RequestPriority priority,
+    scoped_ptr<HttpTransaction>* trans) {
+  trans->reset(new TestNetworkTransaction(err_));
+  return OK;
+}
+
+HttpCache* TestNetworkTransactionFactory::GetCache() {
+  return NULL;
+}
+
+HttpNetworkSession* TestNetworkTransactionFactory::GetSession() {
+  return NULL;                        // TODO(rdsmith): Is this really safe?

[mmenke, 2014/01/23 20:40:52]

Can't we just pass in a real session?  Seems the safest thing to do, and at
least when we have the cache, we can get one from it.

[Randy Smith (Not in Mondays), 2014/01/25 21:11:31]

Done.

+}
+
+}  // namespace net
+