Move client certificates retrieval logic out of the SSL sockets.

content/browser/loader/resource_loader.h

Index: content/browser/loader/resource_loader.h
diff --git a/content/browser/loader/resource_loader.h b/content/browser/loader/resource_loader.h
index 14a67ec9e9c3587f047882a40b0ea27682dfc5c2..a80105e2baf704ee403cd9690e85d8250b4a8c56 100644
--- a/content/browser/loader/resource_loader.h
+++ b/content/browser/loader/resource_loader.h
@@ -5,6 +5,7 @@
 #ifndef CONTENT_BROWSER_LOADER_RESOURCE_LOADER_H_
 #define CONTENT_BROWSER_LOADER_RESOURCE_LOADER_H_
 
+#include "base/gtest_prod_util.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "content/browser/loader/resource_handler.h"
@@ -12,6 +13,10 @@
 #include "content/public/browser/resource_controller.h"
 #include "net/url_request/url_request.h"
 
+namespace net {
+class ClientCertStore;
+}
+
 namespace content {
 class ResourceDispatcherHostLoginDelegate;
 class ResourceLoaderDelegate;
@@ -21,9 +26,9 @@ class SSLClientAuthHandler;
 // This class is responsible for driving the URLRequest (i.e., calling Start,
 // Read, and servicing events).  It has a ResourceHandler, which is typically a
 // chain of ResourceHandlers, and is the ResourceController for its handler.
-class ResourceLoader : public net::URLRequest::Delegate,
-                       public SSLErrorHandler::Delegate,
-                       public ResourceController {
+class CONTENT_EXPORT ResourceLoader : public net::URLRequest::Delegate,

[Ryan Sleevi, 2013/02/01 22:48:37]

This is incorrect, in that ResourceLoader should *not* be exported as part of
the content module.

Perhaps you meant CONTENT_EXPORT_PRIVATE, which allows it to be used in
content_unittests?

[ppi, 2013/02/04 19:35:54]

I would have meant that if we had CONTENT_EXPORT_PRIVATE. This was attempted to
be introduced here: https://chromiumcodereview.appspot.com/10735028/ , but
eventually wasn't.

I do see a readability gain in explicit marking of the parts exported only for
the sake of the unittests, although I am not sure if I should introduce
CONTENT_EXPORT_PRIVATE in this CL. Please let me know if you think I should do
so.

+                                      public SSLErrorHandler::Delegate,
+                                      public ResourceController {
  public:
   ResourceLoader(scoped_ptr<net::URLRequest> request,
                  scoped_ptr<ResourceHandler> handler,
@@ -50,6 +55,12 @@ class ResourceLoader : public net::URLRequest::Delegate,
   void OnUploadProgressACK();
 
  private:
+  FRIEND_TEST_ALL_PREFIXES(ResourceLoaderTest, ClientCertStoreLookup);
+
+  // Hook for testing - swaps the client cert store with the provided one.
+  void swap_client_cert_store_for_testing(
+      scoped_ptr<net::ClientCertStore>& store);

[Ryan Sleevi, 2013/02/01 22:48:37]

style:

1) It is not accepted by Google C++ Style or Chromium C++ style to pass
non-const reference arguments:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Refere...

2) Neither is it necessary, because of the calling conventions explained on
http://www.chromium.org/developers/smart-pointer-guidelines , scoped_ptr<> has a
C++11 move emulation, that lets you just have

swap_client_cert_store_for_testing(scoped_ptr<net::ClientCertStore> store)  //
Note how it's passed by-val, not by-ref

You then can use .Pass() to pass an existing scoped_ptr in to that method

3) As per
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
, this should minimally be

SwapClientCertStoreForTesting() (note the CamelCaps here)

4) Since your test creates its own ResourceLoader, this may be better as a
private ctor that takes a net::ClientCertStore as the fourth argument. This is
also consistent with the earlier remarks about wanting to shift to a D-I long
term.

[ppi, 2013/02/04 19:35:54]

Thanks! In patch set 7 I have moved to using a private constructor. Ad. 2) - I
was aiming at "swap", rather than "set" behavior. If I see this correctly, that
wouldn't be possible with pass-by-value (althought I am not arguing that it was
a good idea).

+
   // net::URLRequest::Delegate implementation:
   virtual void OnReceivedRedirect(net::URLRequest* request,
                                   const GURL& new_url,
@@ -114,6 +125,8 @@ class ResourceLoader : public net::URLRequest::Delegate,
   // which point we'll receive a new ResourceHandler.
   bool is_transferring_;
 
+  scoped_ptr<net::ClientCertStore> client_cert_store_;
+
   base::WeakPtrFactory<ResourceLoader> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ResourceLoader);