Move client certificates retrieval logic out of the SSL sockets.

content/browser/loader/resource_loader.h

 // Copyright (c) 2012 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.
 
 #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"
 #include "content/browser/ssl/ssl_error_handler.h"
 #include "content/public/browser/resource_controller.h"
 #include "net/url_request/url_request.h"
 
+namespace net {
+class ClientCertStore;
+}
+
 namespace content {
 class ResourceDispatcherHostLoginDelegate;
 class ResourceLoaderDelegate;
 class ResourceRequestInfoImpl;
 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,
                  ResourceLoaderDelegate* delegate);
   virtual ~ResourceLoader();
 
   void StartRequest();
   void CancelRequest(bool from_renderer);
 
   void ReportUploadProgress();
 
   bool is_transferring() const { return is_transferring_; }
   void MarkAsTransferring();
   void WillCompleteTransfer();
   void CompleteTransfer(scoped_ptr<ResourceHandler> new_handler);
 
   net::URLRequest* request() { return request_.get(); }
   ResourceRequestInfoImpl* GetRequestInfo();
 
   void ClearLoginDelegate();
   void ClearSSLClientAuthHandler();
 
   // IPC message handlers:
   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,
                                   bool* defer) OVERRIDE;
   virtual void OnAuthRequired(net::URLRequest* request,
                               net::AuthChallengeInfo* info) OVERRIDE;
   virtual void OnCertificateRequested(net::URLRequest* request,
                                       net::SSLCertRequestInfo* info) OVERRIDE;
   virtual void OnSSLCertificateError(net::URLRequest* request,
                                      const net::SSLInfo& info,
@@ skipping 44 matching lines @@
 
   uint64 last_upload_position_;
   bool waiting_for_upload_progress_ack_;
   base::TimeTicks last_upload_ticks_;
 
   // Indicates that we are in a state of being transferred to a new downstream
   // consumer.  We are waiting for a notification to complete the transfer, at
   // 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);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_LOADER_RESOURCE_LOADER_H_