Plumb network stack information about existence of cached copy

content/renderer/render_view_browsertest.cc

Index: content/renderer/render_view_browsertest.cc
diff --git a/content/renderer/render_view_browsertest.cc b/content/renderer/render_view_browsertest.cc
index 7ffa3fa224bdf5fc290403450f776f3d1c7a39ab..df4b8d6c9c44f23707b70336ec82d676284970bf 100644
--- a/content/renderer/render_view_browsertest.cc
+++ b/content/renderer/render_view_browsertest.cc
@@ -4,28 +4,48 @@
 
 #include "base/basictypes.h"
 
+#include "base/bind.h"
+#include "base/callback.h"
 #include "base/memory/shared_memory.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/win/windows_version.h"
 #include "content/common/ssl_status_serialization.h"
 #include "content/common/view_messages.h"
+#include "content/public/browser/browser_context.h"
+#include "content/public/browser/browser_thread.h"
 #include "content/public/browser/native_web_keyboard_event.h"
+#include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/render_process_host.h"
+#include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_ui_controller_factory.h"
 #include "content/public/common/bindings_policy.h"
+#include "content/public/common/content_switches.h"
 #include "content/public/common/page_zoom.h"
 #include "content/public/common/url_constants.h"
 #include "content/public/common/url_utils.h"
 #include "content/public/renderer/document_state.h"
 #include "content/public/renderer/history_item_serialization.h"
 #include "content/public/renderer/navigation_state.h"
+#include "content/public/test/browser_test_utils.h"
 #include "content/public/test/render_view_test.h"
+#include "content/public/test/test_utils.h"
 #include "content/renderer/render_view_impl.h"
+#include "content/shell/browser/shell.h"
+#include "content/shell/browser/shell_browser_context.h"
 #include "content/shell/browser/shell_content_browser_client.h"
 #include "content/shell/common/shell_content_client.h"
+#include "content/shell/renderer/shell_content_renderer_client.h"
+#include "content/test/content_browser_test.h"
+#include "content/test/content_browser_test_utils.h"
 #include "content/test/mock_keyboard.h"
 #include "net/base/net_errors.h"
 #include "net/cert/cert_status_flags.h"
+#include "net/disk_cache/disk_cache.h"
+#include "net/http/failing_http_transaction_factory.h"
+#include "net/http/http_cache.h"
+#include "net/url_request/url_request_context.h"
+#include "net/url_request/url_request_context_getter.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/WebKit/public/platform/WebData.h"
 #include "third_party/WebKit/public/platform/WebHTTPBody.h"
@@ -117,8 +137,6 @@ class WebUITestWebUIControllerFactory : public WebUIControllerFactory {
   }
 };
 
-}  // namespace
-
 class RenderViewImplTest : public RenderViewTest {
  public:
   RenderViewImplTest() {
@@ -304,6 +322,167 @@ class RenderViewImplTest : public RenderViewTest {
   scoped_ptr<MockKeyboard> mock_keyboard_;
 };
 
+class TestShellContentRendererClient : public ShellContentRendererClient {
+ public:
+  TestShellContentRendererClient()
+      : latest_error_valid_(false),
+        latest_error_reason_(0),
+        latest_error_stale_copy_in_cache_(false) {}
+
+  virtual bool ShouldSuppressErrorPage(RenderFrame* render_frame,
+                                       const GURL& url) OVERRIDE {
+    return url == GURL("http://example.com/suppress");
+  }

[mmenke, 2014/01/29 16:14:35]

Is this function needed?

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

Nope; copy and paste error.  Done.

+
+  virtual void GetNavigationErrorStrings(
+      content::RenderView* render_view,
+      blink::WebFrame* frame,
+      const blink::WebURLRequest& failed_request,
+      const blink::WebURLError& error,
+      std::string* error_html,
+      base::string16* error_description) OVERRIDE {
+    if (error_html)
+      *error_html = "A suffusion of yellow.";
+    latest_error_valid_ = true;
+    latest_error_reason_ = error.reason;
+    latest_error_stale_copy_in_cache_ = error.staleCopyInCache;
+  }
+
+  bool GetLatestError(int* error_code, bool* stale_cache_entry_present) {
+    if (latest_error_valid_) {
+      *error_code = latest_error_reason_;
+      *stale_cache_entry_present = latest_error_stale_copy_in_cache_;
+    }
+    return latest_error_valid_;
+  }
+
+ private:
+  bool latest_error_valid_;
+  int latest_error_reason_;
+  bool latest_error_stale_copy_in_cache_;
+};
+
+
+// For actual, browser side tests targetting code in RenderView
+// or RenderViewImpl.
+class RenderViewBrowserTest : public ContentBrowserTest {

[mmenke, 2014/01/29 16:14:35]

optional:  For large files like this, I think it makes future CLs a little
simpler if the test fixture and relevant classes are defined just above the
tests that use them.

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

Done.

+ public:
+  RenderViewBrowserTest() : renderer_client_(NULL) {}
+
+  virtual void SetUpCommandLine(CommandLine* command_line) OVERRIDE {
+    // To allow interaction with in-process renderer and use
+    // of a test ContentRendererClient.

[mmenke, 2014/01/29 16:14:35]

nit:  Per style guide, comments should be sentences.  Suggest just adding
"Needed" in front.  For some reason, I'm fine with the class level comment, but
this one just seems weird.

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

Done.

+    command_line->AppendSwitch(switches::kSingleProcess);
+  }
+
+  virtual void SetUp() OVERRIDE {
+    // Override setting of renderer client.
+    renderer_client_ = new TestShellContentRendererClient();
+    SetContentRendererClient(
+        scoped_ptr<ContentRendererClient>(renderer_client_).Pass());
+
+    ContentBrowserTest::SetUp();
+  }
+
+  // Navigates to the given URL and waits for |num_navigations| to occur, and
+  // the title to change to |expected_title|.
+  void NavigateToURLAndWaitForTitle(const GURL& url,
+                                    const std::string& expected_title,
+                                    int num_navigations) {
+    content::TitleWatcher title_watcher(
+        shell()->web_contents(), base::ASCIIToUTF16(expected_title));
+
+    content::NavigateToURLBlockUntilNavigationsComplete(
+        shell(), url, num_navigations);
+
+    EXPECT_EQ(base::ASCIIToUTF16(expected_title),
+              title_watcher.WaitAndGetTitle());
+  }
+
+  // Returns true if there is a valid error stored; in this case
+  // |*error| will be set to that value.

[mmenke, 2014/01/29 16:14:35]

Suggest just saying "... in this case, |*error_code| and
|*stale_cache_entry_present| will be updated appropriately."

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

Done.

+  // Must be called after the renderer thread is created.
+  bool GetLatestErrorFromRendererClient(
+      int* error_code, bool* stale_cache_entry_present) {
+    bool result = false;
+
+    PostTaskToInProcessRendererAndWait(
+        base::Bind(&RenderViewBrowserTest::GetLatestErrorFromRendererClient0,
+                   renderer_client_, &result, error_code,
+                   stale_cache_entry_present));
+    return result;
+  }
+
+ private:
+  // Must be run on renderer thread.
+  static void GetLatestErrorFromRendererClient0(
+      TestShellContentRendererClient* renderer_client,
+      bool* result, int* error_code, bool* stale_cache_entry_present) {
+    *result = renderer_client->GetLatestError(
+        error_code, stale_cache_entry_present);
+  }
+
+  // Actually owned by our superclass, so safe to keep a bare pointer.
+  TestShellContentRendererClient* renderer_client_;
+};
+
+// Must be called on IO thread.
+void InterceptNetworkTransactions(net::URLRequestContextGetter* getter,
+                                  net::Error error) {
+  DCHECK(content::BrowserThread::CurrentlyOn(BrowserThread::IO));
+  net::HttpCache* cache(
+      getter->GetURLRequestContext()->http_transaction_factory()->GetCache());
+  DCHECK(cache);
+  scoped_ptr<net::FailingHttpTransactionFactory> factory(
+      new net::FailingHttpTransactionFactory(cache->GetSession(), error));
+  // Throw away old version; since we're a browser test, we don't
+  // need to restore the old state.

[mmenke, 2014/01/29 16:14:35]

nit: --we

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

Done, but I'll note that when I searched the file for instances of the word
"we", I found several that I hadn't added :-}.

[mmenke, 2014/01/29 22:18:23]

Not worth worrying about.  I don't think it's a huge deal (And don't feel too
strongly about it myself), but enough people have complained about me using it
that I figure it's best to try and minimize adding more instances of it.

+  cache->SetHttpNetworkTransactionFactoryForTesting(
+      factory.PassAs<net::HttpTransactionFactory>());
+}
+
+void CallOnUIThreadValidatingReturn(const base::Closure& callback,
+                                    int rv) {
+  DCHECK_EQ(net::OK, rv);
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE, callback);
+}
+
+// Must be called on IO thread.  The callback will be called on
+// completion of cache clearing on the UI thread.
+void BackendClearCache(disk_cache::Backend** backend,

[mmenke, 2014/01/29 16:14:35]

optional:  Could we just make backend a scoped_ptr<disk_cache::Backend*>?

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

Done.

+                       const base::Closure& callback,
+                       int rv) {
+  DCHECK(*backend);
+  DCHECK_EQ(net::OK, rv);
+  (*backend)->DoomAllEntries(
+      base::Bind(&CallOnUIThreadValidatingReturn, callback));
+  // Not deleting the backend, just the spot allocated for storing
+  // the pointer.
+  delete backend;
+  return;

[mmenke, 2014/01/29 16:14:35]

Return not needed.

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

Done.

+}
+
+// Must be called on IO thread.  The callback will be called on
+// completion of cache clearing on the UI thread.
+void ClearCache(net::URLRequestContextGetter* getter,
+                const base::Closure& callback) {
+  DCHECK(content::BrowserThread::CurrentlyOn(BrowserThread::IO));
+  net::HttpCache* cache(
+      getter->GetURLRequestContext()->http_transaction_factory()->GetCache());
+  DCHECK(cache);
+  disk_cache::Backend** backend = new disk_cache::Backend*;
+  *backend = NULL;
+  if (net::OK == cache->GetBackend(
+          backend, base::Bind(&BackendClearCache, backend, callback))) {
+    // Completed synchronously, so our job to call the callback.
+    BackendClearCache(backend, callback, net::OK);
+    // *backend has been deleted.
+  }
+}
+
+}  // namespace

[mmenke, 2014/01/29 16:14:35]

I'm assuming the anonymous namespace can't just be extended to the end of the
file.

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

I've seen both idioms, and have a very small preference for the idiom that
leaves the test in the exposed namespace as a symbol of "these are the meat of
the file".  Having said that, I basically don't care and would be happy to
extend it, but that wasn't the idiom in this file when I got here, so I'm taking
the "leave things the way you found them" attitude.  If the file OWNER wants me
to extend it, I'm perfectly fine with that.  

+
 // Test that we get form state change notifications when input fields change.
 TEST_F(RenderViewImplTest, DISABLED_OnNavStateChanged) {
   // Don't want any delay for form state sync changes. This will still post a
@@ -2240,4 +2419,51 @@ TEST_F(RenderViewImplTest, FocusElementCallsFocusedNodeChanged) {
   render_thread_->sink().ClearMessages();
 }
 
+IN_PROC_BROWSER_TEST_F(RenderViewBrowserTest, ConfirmCacheInformationPlumbed) {
+  ASSERT_TRUE(test_server()->Start());
+
+  // Load cache with entry with "nocache" set, to create stale
+  // cache.

[mmenke, 2014/01/29 16:14:35]

nit:  "Load cache with entry" -> "Load page" or "Load URL"?

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

Done.

+  GURL test_url(test_server()->GetURL("files/nocache.html"));
+  NavigateToURLAndWaitForTitle(test_url, "Nocache Test Page", 1);
+
+  // Reload same URL after forcing an error from the the network layer;
+  // confirm that the error page is told the cached copy exists.
+  int renderer_id =
+      shell()->web_contents()->GetMainFrame()->GetProcess()->GetID();
+  scoped_refptr<net::URLRequestContextGetter> url_request_context_getter =
+      ShellContentBrowserClient::Get()->browser_context()->
+          GetRequestContextForRenderProcess(renderer_id);
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&InterceptNetworkTransactions, url_request_context_getter,
+                 net::ERR_FAILED));
+
+  // An error results in one completed navigation.
+  content::NavigateToURLBlockUntilNavigationsComplete(shell(), test_url, 1);
+  int error_code = net::OK;
+  bool stale_cache_entry_present = false;
+  ASSERT_TRUE(GetLatestErrorFromRendererClient(
+      &error_code, &stale_cache_entry_present));
+  ASSERT_EQ(net::ERR_FAILED, error_code);
+  ASSERT_TRUE(stale_cache_entry_present);

[mmenke, 2014/01/29 16:14:35]

Should these two be expects instead?  Just thinking that if the error code is
different, knowing the value of stale_cache_entry_present may still be useful.

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

Done.

+
+  // Clear the cache and repeat; confirm lack of entry in cache reported.
+  scoped_refptr<MessageLoopRunner> runner = new MessageLoopRunner;
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&ClearCache, url_request_context_getter,
+                 runner->QuitClosure()));
+  runner->Run();
+
+  content::NavigateToURLBlockUntilNavigationsComplete(shell(), test_url, 1);
+
+  error_code = net::OK;
+  stale_cache_entry_present = true;

[mmenke, 2014/01/29 16:14:35]

optional:  Rather than initialize these here (and above), maybe initialize them
in GetLatestErrorFromRendererClient?

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

The problem is that it's not clear what to initialize them to.  The current code
initializes them to values that are different than the ones expected, implicitly
to guard against GetLatestErrorFromRendererClient() not setting the values.  I'd
be basically happy to have the test code rely on
GetLatestErrorFromRendererClient() always either setting the values or returning
false, but that'd either leave me with declaring variables without initializing
them above (which feels a bit icky) or setting them to values that are relevant
(opposite expected) for the first probe but not resetting them for the second
probe.  Given that set of options, I think the current set/reset is best, but if
you have other feelings I'd be interested.

+  ASSERT_TRUE(GetLatestErrorFromRendererClient(
+      &error_code, &stale_cache_entry_present));
+  ASSERT_EQ(net::ERR_FAILED, error_code);
+  ASSERT_FALSE(stale_cache_entry_present);

[mmenke, 2014/01/29 16:14:35]

Should these two be expects instead?

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

Done.

+}
+
 }  // namespace content