Follow-up changes to https://codereview.chromium.org/795773002/

content/browser/loader/resource_loader_unittest.cc

 // Copyright (c) 2013 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 "content/browser/loader/resource_loader.h"
 
 #include "base/files/file.h"
 #include "base/files/file_util.h"
 #include "base/message_loop/message_loop_proxy.h"
 #include "base/run_loop.h"
@@ skipping 22 matching lines @@
 using storage::ShareableFileReference;
 
 namespace content {
 namespace {
 
 // Stub client certificate store that returns a preset list of certificates for
 // each request and records the arguments of the most recent request for later
 // inspection.
 class ClientCertStoreStub : public net::ClientCertStore {
  public:
-  ClientCertStoreStub(const net::CertificateList& certs)
-      : response_(certs), async_(false), request_count_(0) {}
+  // Creates a new ClientCertStoreStub that returns |response| on query. It
+  // saves the number of requests and most recently certificate authorities list
+  // in |requested_authorities| and |request_count|, respectively. The caller is
+  // responsible for ensuring those pointers outlive the ClientCertStoreStub.
+  //
+  // TODO(ppi): Make the stub independent from the internal representation of
+  // SSLCertRequestInfo. For now it seems that we cannot neither save the

[mmenke, 2015/01/23 21:43:27]

While you're here...."cannot neither" -> "can neither"

[davidben, 2015/01/23 21:50:56]

Done.

+  // scoped_refptr<> (since it is never passed to us) nor copy the entire
+  // CertificateRequestInfo (since there is no copy constructor).
+  ClientCertStoreStub(const net::CertificateList& response,
+                      int* request_count,
+                      std::vector<std::string>* requested_authorities)
+      : response_(response),
+        async_(false),
+        requested_authorities_(requested_authorities),
+        request_count_(request_count) {
+    requested_authorities_->clear();
+    *request_count_ = 0;
+  }
 
   ~ClientCertStoreStub() override {}
 
   // Configures whether the certificates are returned asynchronously or not.
   void set_async(bool async) { async_ = async; }
 
-  // Returns |cert_authorities| field of the certificate request passed in the
-  // most recent call to GetClientCerts().
-  // TODO(ppi): Make the stub independent from the internal representation of
-  // SSLCertRequestInfo. For now it seems that we cannot neither save the
-  // scoped_refptr<> (since it is never passed to us) nor copy the entire
-  // CertificateRequestInfo (since there is no copy constructor).
-  std::vector<std::string> requested_authorities() {
-    return requested_authorities_;
-  }
-
-  // Returns the number of calls to GetClientCerts().
-  int request_count() {
-    return request_count_;
-  }
-
   // net::ClientCertStore:
   void GetClientCerts(const net::SSLCertRequestInfo& cert_request_info,
                       net::CertificateList* selected_certs,
                       const base::Closure& callback) override {
-    ++request_count_;
-    requested_authorities_ = cert_request_info.cert_authorities;
+    *requested_authorities_ = cert_request_info.cert_authorities;
+    ++(*request_count_);
+
     *selected_certs = response_;
     if (async_) {
       base::MessageLoop::current()->PostTask(FROM_HERE, callback);
     } else {
       callback.Run();
     }
   }
 
  private:
   const net::CertificateList response_;
   bool async_;
-  int request_count_;
-  std::vector<std::string> requested_authorities_;
+  std::vector<std::string>* requested_authorities_;
+  int* request_count_;
 };
 
 // Arbitrary read buffer size.
 const int kReadBufSize = 1024;
 
 // Dummy implementation of ResourceHandler, instance of which is needed to
 // initialize ResourceLoader.
 class ResourceHandlerStub : public ResourceHandler {
  public:
   explicit ResourceHandlerStub(net::URLRequest* request)
@@ skipping 275 matching lines @@
   net::URLRequest* raw_ptr_to_request_;
   scoped_ptr<ResourceLoader> loader_;
 };
 
 // Verifies if a call to net::UrlRequest::Delegate::OnCertificateRequested()
 // causes client cert store to be queried for certificates and if the returned
 // certificates are correctly passed to the content browser client for
 // selection.
 TEST_F(ResourceLoaderTest, ClientCertStoreLookup) {
   // Set up the test client cert store.
+  int store_request_count;
+  std::vector<std::string> store_requested_authorities;
   net::CertificateList dummy_certs(1, scoped_refptr<net::X509Certificate>(
       new net::X509Certificate("test", "test", base::Time(), base::Time())));
-  scoped_ptr<ClientCertStoreStub> test_store(
-      new ClientCertStoreStub(dummy_certs));
-  EXPECT_EQ(0, test_store->request_count());
-
-  // Ownership of the |test_store| is about to be turned over to ResourceLoader.
-  // We need to keep raw pointer copies to access these objects later.
-  ClientCertStoreStub* raw_ptr_to_store = test_store.get();
+  scoped_ptr<ClientCertStoreStub> test_store(new ClientCertStoreStub(
+      dummy_certs, &store_request_count, &store_requested_authorities));
   resource_context_.SetClientCertStore(test_store.Pass());
 
   // Prepare a dummy certificate request.
   scoped_refptr<net::SSLCertRequestInfo> cert_request_info(
       new net::SSLCertRequestInfo());
   std::vector<std::string> dummy_authority(1, "dummy");
   cert_request_info->cert_authorities = dummy_authority;
 
   // Plug in test content browser client.
   SelectCertificateBrowserClient test_client;
   ContentBrowserClient* old_client = SetBrowserClientForTesting(&test_client);
 
   // Everything is set up. Trigger the resource loader certificate request event
   // and run the message loop.
   loader_->OnCertificateRequested(raw_ptr_to_request_, cert_request_info.get());
   base::RunLoop().RunUntilIdle();
 
   // Restore the original content browser client.
   SetBrowserClientForTesting(old_client);
 
   // Check if the test store was queried against correct |cert_authorities|.
-  EXPECT_EQ(1, raw_ptr_to_store->request_count());
-  EXPECT_EQ(dummy_authority, raw_ptr_to_store->requested_authorities());
+  EXPECT_EQ(1, store_request_count);
+  EXPECT_EQ(dummy_authority, store_requested_authorities);
 
   // Check if the retrieved certificates were passed to the content browser
   // client.
   EXPECT_EQ(1, test_client.call_count());
   EXPECT_EQ(dummy_certs, test_client.passed_certs());
 }
 
 // Verifies if a call to net::URLRequest::Delegate::OnCertificateRequested()
 // on a platform with a NULL client cert store still calls the content browser
 // client for selection.
@@ skipping 17 matching lines @@
   SetBrowserClientForTesting(old_client);
 
   // Check if the SelectClientCertificate was called on the content browser
   // client.
   EXPECT_EQ(1, test_client.call_count());
   EXPECT_EQ(net::CertificateList(), test_client.passed_certs());
 }
 
 TEST_F(ResourceLoaderTest, ClientCertStoreAsyncCancel) {
   // Set up the test client cert store.
+  int store_request_count;
+  std::vector<std::string> store_requested_authorities;
   scoped_ptr<ClientCertStoreStub> test_store(
-      new ClientCertStoreStub(net::CertificateList()));
+      new ClientCertStoreStub(net::CertificateList(), &store_request_count,
+                              &store_requested_authorities));
   test_store->set_async(true);
-  EXPECT_EQ(0, test_store->request_count());
-
-  // Ownership of the |test_store| is about to be turned over to ResourceLoader.
-  // We need to keep raw pointer copies to access these objects later.
-  ClientCertStoreStub* raw_ptr_to_store = test_store.get();
+  EXPECT_EQ(0, store_request_count);
   resource_context_.SetClientCertStore(test_store.Pass());
 
   // Prepare a dummy certificate request.
   scoped_refptr<net::SSLCertRequestInfo> cert_request_info(
       new net::SSLCertRequestInfo());
   std::vector<std::string> dummy_authority(1, "dummy");
   cert_request_info->cert_authorities = dummy_authority;
 
   // Everything is set up. Trigger the resource loader certificate request
   // event.
   loader_->OnCertificateRequested(raw_ptr_to_request_, cert_request_info.get());
 
   // Check if the test store was queried against correct |cert_authorities|.
-  EXPECT_EQ(1, raw_ptr_to_store->request_count());
-  EXPECT_EQ(dummy_authority, raw_ptr_to_store->requested_authorities());
+  EXPECT_EQ(1, store_request_count);
+  EXPECT_EQ(dummy_authority, store_requested_authorities);
 
   // Cancel the request before the store calls the callback.
   loader_.reset();
 
   // Pump the event loop. There shouldn't be a crash when the callback is run.
   base::RunLoop().RunUntilIdle();
 }
 
 TEST_F(ResourceLoaderTest, ResumeCancelledRequest) {
   raw_ptr_resource_handler_->set_defer_request_on_will_start(true);
@@ skipping 279 matching lines @@
   ASSERT_TRUE(base::ReadFileToString(temp_path(), &contents));
   EXPECT_EQ(test_data(), contents);
 
   // Release the loader. The file should be gone now.
   ReleaseLoader();
   base::RunLoop().RunUntilIdle();
   EXPECT_FALSE(base::PathExists(temp_path()));
 }
 
 }  // namespace content