Add StoragePartition's ProtocolHandlers at URLRequestContext construction time.

content/browser/storage_partition_impl_map.cc

Index: content/browser/storage_partition_impl_map.cc
diff --git a/content/browser/storage_partition_impl_map.cc b/content/browser/storage_partition_impl_map.cc
index e1381e12c6f01270caad0225a5c46428e24f46c3..ea631fc24f569856cdc752bf0e701a70f5324b95 100644
--- a/content/browser/storage_partition_impl_map.cc
+++ b/content/browser/storage_partition_impl_map.cc
@@ -45,28 +45,90 @@ namespace content {
 
 namespace {
 
-class BlobProtocolHandler : public webkit_blob::BlobProtocolHandler {
+class BlobProtocolHandler : public net::URLRequestJobFactory::ProtocolHandler {
  public:
-  BlobProtocolHandler(
-      webkit_blob::BlobStorageController* blob_storage_controller,
-      fileapi::FileSystemContext* file_system_context,
-      base::MessageLoopProxy* loop_proxy)
-      : webkit_blob::BlobProtocolHandler(blob_storage_controller,
-                                         file_system_context,
-                                         loop_proxy) {}
+  static BlobProtocolHandler* Create(
+      ChromeBlobStorageContext* blob_storage_context,
+      fileapi::FileSystemContext* file_system_context) {
+    BlobProtocolHandler* blob_protocol_handler = new BlobProtocolHandler();
+    BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, base::Bind(
+        &BlobProtocolHandler::IOThreadDelegate::Init,
+        base::Unretained(blob_protocol_handler->io_thread_delegate_),
+        make_scoped_refptr(blob_storage_context),
+        make_scoped_refptr(file_system_context)));
+    return blob_protocol_handler;
+  }
 
-  virtual ~BlobProtocolHandler() {}
+  virtual ~BlobProtocolHandler() {
+    BrowserThread::DeleteSoon(BrowserThread::IO,
+                              FROM_HERE,
+                              io_thread_delegate_);
+  }
 
- private:
-  virtual scoped_refptr<webkit_blob::BlobData>
-      LookupBlobData(net::URLRequest* request) const {
-    const ResourceRequestInfoImpl* info =
-        ResourceRequestInfoImpl::ForRequest(request);
-    if (!info)
-      return NULL;
-    return info->requested_blob_data();
+  virtual net::URLRequestJob* MaybeCreateJob(
+      net::URLRequest* request,
+      net::NetworkDelegate* network_delegate) const OVERRIDE {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+    return io_thread_delegate_->webkit_blob_protocol_handler_impl_->
+        MaybeCreateJob(request, network_delegate);
   }
 
+ private:
+  // |IOThreadDelegate| is needed because |BlobProtocolHandler| is constructed
+  // on the UI thread while ChromeBlobStorageContext::controller() must be
+  // called on the IO thread and must be called prior to instantiating
+  // |webkit_blob_protocol_handler_impl_|.
+  class IOThreadDelegate {
+   public:
+    IOThreadDelegate() {}
+
+    void Init(ChromeBlobStorageContext* blob_storage_context,
+              fileapi::FileSystemContext* file_system_context) {

[mmenke, 2013/01/08 17:19:26]

I think this is more complicated than necessary.

Can't we get rid of IOThreadDelegate, and just lazy init on the first call to
MaybeCreateJob?  If MaybeCreateJob is called, then we're now owned by the
JabFactoryImpl, which is running on the IO thread.

[pauljensen, 2013/01/21 06:24:56]

Done.  The lazy-init does require making webkit_blob_protocol_handler_impl_
mutable (because MaybeCreateJob() is const) but I think this is okay as
MaybeCreateJob() is only called on one thread.  I had used all the nested
classes to avoid the mutable keyword previously but it's a lot cleaner without
the intervening class.

+      DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+      webkit_blob_protocol_handler_impl_.reset(
+          new WebKitBlobProtocolHandlerImpl(blob_storage_context->controller(),
+                                            file_system_context));
+    }
+
+    // An implementation of webkit_blob::BlobProtocolHandler that gets
+    // the BlobData from ResourceRequestInfoImpl.
+    class WebKitBlobProtocolHandlerImpl
+        : public webkit_blob::BlobProtocolHandler {
+     public:
+      WebKitBlobProtocolHandlerImpl(
+          webkit_blob::BlobStorageController* blob_storage_controller,
+          fileapi::FileSystemContext* file_system_context)
+          : webkit_blob::BlobProtocolHandler(
+                blob_storage_controller, file_system_context,
+                BrowserThread::GetMessageLoopProxyForThread(
+                    BrowserThread::FILE)) {}
+
+      virtual ~WebKitBlobProtocolHandlerImpl() {}
+
+     private:
+      // webkit_blob::BlobProtocolHandler implementation.
+      virtual scoped_refptr<webkit_blob::BlobData>
+          LookupBlobData(net::URLRequest* request) const OVERRIDE {
+        const ResourceRequestInfoImpl* info =
+            ResourceRequestInfoImpl::ForRequest(request);
+        if (!info)
+          return NULL;
+        return info->requested_blob_data();
+      }
+
+      DISALLOW_COPY_AND_ASSIGN(WebKitBlobProtocolHandlerImpl);
+    };
+
+    scoped_ptr<WebKitBlobProtocolHandlerImpl>
+    webkit_blob_protocol_handler_impl_;

[mmenke, 2013/01/08 17:19:26]

Think just calling this "webkit_blob_protocol_handler_" so it fits on one line
is preferable.

[pauljensen, 2013/01/21 06:24:56]

With the mutable keyword it won't fit either way so I left it alone.

+
+    DISALLOW_COPY_AND_ASSIGN(IOThreadDelegate);
+  };
+
+  BlobProtocolHandler() : io_thread_delegate_(new IOThreadDelegate()) {}
+
+  IOThreadDelegate* io_thread_delegate_;
+
   DISALLOW_COPY_AND_ASSIGN(BlobProtocolHandler);
 };
 
@@ -78,9 +140,9 @@ class DeveloperProtocolHandler
  public:
   DeveloperProtocolHandler(
       AppCacheService* appcache_service,
-      BlobStorageController* blob_storage_controller)
+      ChromeBlobStorageContext* blob_storage_context)
       : appcache_service_(appcache_service),
-        blob_storage_controller_(blob_storage_controller) {}
+        blob_storage_context_(blob_storage_context) {}
   virtual ~DeveloperProtocolHandler() {}
 
   virtual net::URLRequestJob* MaybeIntercept(
@@ -101,7 +163,7 @@ class DeveloperProtocolHandler
     // Next check for chrome://blob-internals/, which uses its own job type.
     if (ViewBlobInternalsJobFactory::IsSupportedURL(request->url())) {
       return ViewBlobInternalsJobFactory::CreateJobForRequest(
-          request, network_delegate, blob_storage_controller_);
+          request, network_delegate, blob_storage_context_->controller());
     }
 
 #if defined(USE_TCMALLOC)
@@ -140,55 +202,9 @@ class DeveloperProtocolHandler
 
  private:
   AppCacheService* appcache_service_;
-  BlobStorageController* blob_storage_controller_;
+  ChromeBlobStorageContext* blob_storage_context_;
 };
 
-void InitializeURLRequestContext(
-    net::URLRequestContextGetter* context_getter,
-    AppCacheService* appcache_service,
-    FileSystemContext* file_system_context,
-    ChromeBlobStorageContext* blob_storage_context) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  if (!context_getter)
-    return;  // tests.
-
-  // This code only modifies the URLRequestJobFactory on the context
-  // to handle blob: URLs, filesystem: URLs, and to let AppCache intercept
-  // the appropriate requests.  This is in addition to the slew of other
-  // initializtion that is done in during creation of the URLRequestContext.
-  // We cannot yet centralize this code because URLRequestContext needs
-  // to be created before the StoragePartition context.
-  //
-  // TODO(ajwong): Fix the ordering so all the initialization is in one spot.
-  net::URLRequestContext* context = context_getter->GetURLRequestContext();
-  net::URLRequestJobFactory* job_factory =
-      const_cast<net::URLRequestJobFactory*>(context->job_factory());
-
-  // Note: if this is called twice with 2 request contexts that share one job
-  // factory (as is the case with a media request context and its related
-  // normal request context) then this will early exit.
-  if (job_factory->IsHandledProtocol(chrome::kBlobScheme))
-    return;  // Already initialized this JobFactory.
-
-  bool set_protocol = job_factory->SetProtocolHandler(
-      chrome::kBlobScheme,
-      new BlobProtocolHandler(
-          blob_storage_context->controller(),
-          file_system_context,
-          BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE)));
-  DCHECK(set_protocol);
-  set_protocol = job_factory->SetProtocolHandler(
-      chrome::kFileSystemScheme,
-      CreateFileSystemProtocolHandler(file_system_context));
-  DCHECK(set_protocol);
-
-  job_factory->AddInterceptor(
-      new DeveloperProtocolHandler(appcache_service,
-                                   blob_storage_context->controller()));
-
-  // TODO(jam): Add the ProtocolHandlerRegistryIntercepter here!
-}
-
 // These constants are used to create the directory structure under the profile
 // where renderers with a non-default storage partition keep their persistent
 // state. This will contain a set of directories that partially mirror the
@@ -479,12 +495,28 @@ StoragePartitionImpl* StoragePartitionImplMap::Get(
                                    partition_path);
   partitions_[partition_config] = partition;
 
+  ChromeBlobStorageContext* blob_storage_context =
+      ChromeBlobStorageContext::GetFor(browser_context_);
+  scoped_ptr<net::URLRequestJobFactory::ProtocolHandler> blob_protocol_handler(
+      BlobProtocolHandler::Create(blob_storage_context,
+                                  partition->GetFileSystemContext()));
+  scoped_ptr<net::URLRequestJobFactory::ProtocolHandler>
+  file_system_protocol_handler(
+      CreateFileSystemProtocolHandler(partition->GetFileSystemContext()));
+  scoped_ptr<net::URLRequestJobFactory::Interceptor> developer_protocol_handler(
+      new DeveloperProtocolHandler(partition->GetAppCacheService(),
+                                   blob_storage_context));
+
   // These calls must happen after StoragePartitionImpl::Create().
   partition->SetURLRequestContext(
       partition_domain.empty() ?
-      browser_context_->GetRequestContext() :
-      browser_context_->GetRequestContextForStoragePartition(
-          partition->GetPath(), in_memory));
+      GetContentClient()->browser()->CreateRequestContext(browser_context_,
+          blob_protocol_handler.Pass(), file_system_protocol_handler.Pass(),
+          developer_protocol_handler.Pass()) :
+      GetContentClient()->browser()->CreateRequestContextForStoragePartition(
+          browser_context_, partition->GetPath(), in_memory,
+          blob_protocol_handler.Pass(), file_system_protocol_handler.Pass(),
+          developer_protocol_handler.Pass()));
   partition->SetMediaURLRequestContext(
       partition_domain.empty() ?
       browser_context_->GetMediaRequestContext() :
@@ -593,17 +625,6 @@ void StoragePartitionImplMap::PostCreateInitialization(
                    make_scoped_refptr(
                        browser_context_->GetSpecialStoragePolicy())));
 
-    // Add content's URLRequestContext's hooks.
-    BrowserThread::PostTask(
-        BrowserThread::IO, FROM_HERE,
-        base::Bind(
-            &InitializeURLRequestContext,
-            make_scoped_refptr(partition->GetURLRequestContext()),
-            make_scoped_refptr(partition->GetAppCacheService()),
-            make_scoped_refptr(partition->GetFileSystemContext()),
-            make_scoped_refptr(
-                ChromeBlobStorageContext::GetFor(browser_context_))));

[mmenke, 2013/01/08 17:19:26]

Does this really need to be removed?  We are still creating the context in
StoragePartitionImplMap::Get()...

[pauljensen, 2013/01/21 06:24:56]

The reason I started all this was to eliminate InitializeURLRequestContext()
because it uses URLRequestJobFactory::SetProtocol() which we're trying to remove
(crbug.com/146602).  The const_cast also seems dangerous as it's indicative of
changing the URLRequestJobFactory's behavior after its been constructed.  When
URLRequestContext/URLRequestJobFactory creation is moved to the StoragePartition
this can all be done here, but safely.

-
     // We do not call InitializeURLRequestContext() for media contexts because,
     // other than the HTTP cache, the media contexts share the same backing
     // objects as their associated "normal" request context.  Thus, the previous