protocol: cleanup

atom/browser/api/atom_api_protocol.cc

@@ -25,12 +25,16 @@ namespace mate {
 template<>
 struct Converter<const net::URLRequest*> {
   static v8::Local<v8::Value> ToV8(v8::Isolate* isolate,
-                                    const net::URLRequest* val) {
-    return mate::ObjectTemplateBuilder(isolate)
-        .SetValue("method", val->method())
-        .SetValue("url", val->url().spec())
-        .SetValue("referrer", val->referrer())
-        .Build()->NewInstance();
+                                  const net::URLRequest* val) {
+    if (val) {
+      return mate::ObjectTemplateBuilder(isolate)
+          .SetValue("method", val->method())
+          .SetValue("url", val->url().spec())
+          .SetValue("referrer", val->referrer())
+          .Build()->NewInstance();
+    } else {
+      return v8::Null(isolate);
+    }
   }
 };
 
@@ -65,7 +69,7 @@ class CustomProtocolRequestJob : public AdapterRequestJob {
 
   // AdapterRequestJob:
   void GetJobTypeInUI() override {
-    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
     v8::Locker locker(registry_->isolate());
     v8::HandleScope handle_scope(registry_->isolate());
@@ -73,7 +77,8 @@ class CustomProtocolRequestJob : public AdapterRequestJob {
     // Call the JS handler.
     Protocol::JsProtocolHandler callback =
         registry_->GetProtocolHandler(request()->url().scheme());
-    v8::Local<v8::Value> result = callback.Run(request());
+    v8::Local<v8::Value> result =
+        callback.Run(v8::Null(registry_->isolate()), request());
 
     // Determine the type of the job we are going to create.
     if (result->IsString()) {
@@ -202,6 +207,51 @@ Protocol::JsProtocolHandler Protocol::GetProtocolHandler(
   return protocol_handlers_[scheme];
 }
 
+void Protocol::OnRegisterProtocol(const std::string& scheme,
+                                  const JsProtocolHandler& callback,
+                                  bool is_handled) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  v8::Locker locker(isolate());
+  v8::HandleScope handle_scope(isolate());
+
+  if (is_handled || ContainsKey(protocol_handlers_, scheme)) {
+    callback.Run(v8::Exception::Error(
+        mate::StringToV8(isolate(), "The Scheme is already registered")),
+                         nullptr);
+    return;
+  }
+
+  protocol_handlers_[scheme] = callback;
+  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+                          base::Bind(&Protocol::RegisterProtocolInIO,
+                                     base::Unretained(this), scheme));
+}
+
+void Protocol::OnInterceptProtocol(const std::string& scheme,
+                                   const JsProtocolHandler& callback,
+                                   bool is_handled) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  v8::Locker locker(isolate());
+  v8::HandleScope handle_scope(isolate());
+  if (!is_handled) {
+    callback.Run(v8::Exception::Error(
+        mate::StringToV8(isolate(), "Scheme does not exist.")), nullptr);
+    return;
+  }
+
+  if (ContainsKey(protocol_handlers_, scheme)) {
+    callback.Run(v8::Exception::Error(
+        mate::StringToV8(isolate(), "Cannot intercept custom protocols.")),
+                         nullptr);
+    return;
+  }
+
+  protocol_handlers_[scheme] = callback;
+  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+                          base::Bind(&Protocol::InterceptProtocolInIO,
+                                     base::Unretained(this), scheme));
+}
+
 mate::ObjectTemplateBuilder Protocol::GetObjectTemplateBuilder(
     v8::Isolate* isolate) {
   return mate::ObjectTemplateBuilder(isolate)
@@ -216,15 +266,10 @@ mate::ObjectTemplateBuilder Protocol::GetObjectTemplateBuilder(
 void Protocol::RegisterProtocol(v8::Isolate* isolate,
                                 const std::string& scheme,
                                 const JsProtocolHandler& callback) {
-  if (ContainsKey(protocol_handlers_, scheme) ||
-      job_factory_->IsHandledProtocol(scheme))
-    return node::ThrowError(isolate, "The scheme is already registered");
-
-  protocol_handlers_[scheme] = callback;
-  BrowserThread::PostTask(BrowserThread::IO,
-                          FROM_HERE,
-                          base::Bind(&Protocol::RegisterProtocolInIO,
-                                     base::Unretained(this), scheme));
+  IsHandledProtocol(scheme,
+                    base::Bind(&Protocol::OnRegisterProtocol,
+                               base::Unretained(this),
+                               scheme, callback));
 }
 
 void Protocol::UnregisterProtocol(v8::Isolate* isolate,
@@ -245,24 +290,22 @@ void Protocol::RegisterStandardSchemes(
   atom::AtomBrowserClient::SetCustomSchemes(schemes);
 }
 
-bool Protocol::IsHandledProtocol(const std::string& scheme) {
-  return job_factory_->IsHandledProtocol(scheme);
+void Protocol::IsHandledProtocol(const std::string& scheme,
+                                 const CompletionCallback& callback) {
+  BrowserThread::PostTaskAndReplyWithResult(BrowserThread::IO, FROM_HERE,
+      base::Bind(&AtomURLRequestJobFactory::IsHandledProtocol,
+                 base::Unretained(job_factory_), scheme),
+      callback);
 }
 
 void Protocol::InterceptProtocol(v8::Isolate* isolate,
                                  const std::string& scheme,
                                  const JsProtocolHandler& callback) {
-  if (!job_factory_->HasProtocolHandler(scheme))
-    return node::ThrowError(isolate, "Scheme does not exist.");
-
-  if (ContainsKey(protocol_handlers_, scheme))
-    return node::ThrowError(isolate, "Cannot intercept custom procotols");
-
-  protocol_handlers_[scheme] = callback;
-  BrowserThread::PostTask(BrowserThread::IO,
-                          FROM_HERE,
-                          base::Bind(&Protocol::InterceptProtocolInIO,
-                                     base::Unretained(this), scheme));
+  BrowserThread::PostTaskAndReplyWithResult(BrowserThread::IO, FROM_HERE,
+      base::Bind(&AtomURLRequestJobFactory::HasProtocolHandler,
+                 base::Unretained(job_factory_), scheme),
+      base::Bind(&Protocol::OnInterceptProtocol,
+                 base::Unretained(this), scheme, callback));
 }
 
 void Protocol::UninterceptProtocol(v8::Isolate* isolate,
@@ -279,7 +322,7 @@ void Protocol::UninterceptProtocol(v8::Isolate* isolate,
 }
 
 void Protocol::RegisterProtocolInIO(const std::string& scheme) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   job_factory_->SetProtocolHandler(scheme, new CustomProtocolHandler(this));
   BrowserThread::PostTask(BrowserThread::UI,
@@ -290,7 +333,7 @@ void Protocol::RegisterProtocolInIO(const std::string& scheme) {
 }
 
 void Protocol::UnregisterProtocolInIO(const std::string& scheme) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   job_factory_->SetProtocolHandler(scheme, NULL);
   BrowserThread::PostTask(BrowserThread::UI,
@@ -301,7 +344,7 @@ void Protocol::UnregisterProtocolInIO(const std::string& scheme) {
 }
 
 void Protocol::InterceptProtocolInIO(const std::string& scheme) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   ProtocolHandler* original_handler = job_factory_->GetProtocolHandler(scheme);
   if (original_handler == NULL) {
@@ -322,7 +365,7 @@ void Protocol::InterceptProtocolInIO(const std::string& scheme) {
 }
 
 void Protocol::UninterceptProtocolInIO(const std::string& scheme) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   CustomProtocolHandler* handler = static_cast<CustomProtocolHandler*>(
       job_factory_->GetProtocolHandler(scheme));

atom/browser/api/atom_api_protocol.h

@@ -26,8 +26,10 @@ namespace api {
 
 class Protocol : public mate::EventEmitter {
  public:
-  typedef base::Callback<v8::Local<v8::Value>(const net::URLRequest*)>
-          JsProtocolHandler;
+  using JsProtocolHandler =
+      base::Callback<v8::Local<v8::Value>(v8::Local<v8::Value>,
+                                          const net::URLRequest*)>;
+  using CompletionCallback = base::Callback<void(bool)>;
 
   static mate::Handle<Protocol> Create(
       v8::Isolate* isolate, AtomBrowserContext* browser_context);
@@ -46,6 +48,15 @@ class Protocol : public mate::EventEmitter {
  private:
   typedef std::map<std::string, JsProtocolHandler> ProtocolHandlersMap;
 
+  // Callback called if protocol can be registered.
+  void OnRegisterProtocol(const std::string& scheme,
+                          const JsProtocolHandler& callback,
+                          bool is_handled);
+  // Callback called if protocol can be intercepted.
+  void OnInterceptProtocol(const std::string& scheme,
+                           const JsProtocolHandler& callback,
+                           bool is_handled);
+
   // Register schemes to standard scheme list.
   void RegisterStandardSchemes(const std::vector<std::string>& schemes);
 
@@ -57,9 +68,8 @@ class Protocol : public mate::EventEmitter {
   void UnregisterProtocol(v8::Isolate* isolate, const std::string& scheme);
 
   // Returns whether a scheme has been registered.
-  // FIXME Should accept a callback and be asynchronous so we do not have to use
-  // locks.
-  bool IsHandledProtocol(const std::string& scheme);
+  void IsHandledProtocol(const std::string& scheme,
+                         const CompletionCallback& callback);
 
   // Intercept/unintercept an existing protocol handler.
   void InterceptProtocol(v8::Isolate* isolate,

atom/browser/net/atom_url_request_job_factory.cc

@@ -6,9 +6,12 @@
 #include "atom/browser/net/atom_url_request_job_factory.h"
 
 #include "base/stl_util.h"
+#include "content/public/browser/browser_thread.h"
 #include "net/base/load_flags.h"
 #include "net/url_request/url_request.h"
 
+using content::BrowserThread;
+
 namespace atom {
 
 typedef net::URLRequestJobFactory::ProtocolHandler ProtocolHandler;
@@ -22,9 +25,7 @@ AtomURLRequestJobFactory::~AtomURLRequestJobFactory() {
 bool AtomURLRequestJobFactory::SetProtocolHandler(
     const std::string& scheme,
     ProtocolHandler* protocol_handler) {
-  DCHECK(CalledOnValidThread());
-
-  base::AutoLock locked(lock_);
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   if (!protocol_handler) {
     ProtocolHandlerMap::iterator it = protocol_handler_map_.find(scheme);
@@ -45,10 +46,9 @@ bool AtomURLRequestJobFactory::SetProtocolHandler(
 ProtocolHandler* AtomURLRequestJobFactory::ReplaceProtocol(
     const std::string& scheme,
     ProtocolHandler* protocol_handler) {
-  DCHECK(CalledOnValidThread());
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(protocol_handler);
 
-  base::AutoLock locked(lock_);
   if (!ContainsKey(protocol_handler_map_, scheme))
     return nullptr;
   ProtocolHandler* original_protocol_handler = protocol_handler_map_[scheme];
@@ -58,9 +58,8 @@ ProtocolHandler* AtomURLRequestJobFactory::ReplaceProtocol(
 
 ProtocolHandler* AtomURLRequestJobFactory::GetProtocolHandler(
     const std::string& scheme) const {
-  DCHECK(CalledOnValidThread());
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-  base::AutoLock locked(lock_);
   ProtocolHandlerMap::const_iterator it = protocol_handler_map_.find(scheme);
   if (it == protocol_handler_map_.end())
     return nullptr;
@@ -69,7 +68,6 @@ ProtocolHandler* AtomURLRequestJobFactory::GetProtocolHandler(
 
 bool AtomURLRequestJobFactory::HasProtocolHandler(
     const std::string& scheme) const {
-  base::AutoLock locked(lock_);
   return ContainsKey(protocol_handler_map_, scheme);
 }
 
@@ -77,9 +75,8 @@ net::URLRequestJob* AtomURLRequestJobFactory::MaybeCreateJobWithProtocolHandler(
     const std::string& scheme,
     net::URLRequest* request,
     net::NetworkDelegate* network_delegate) const {
-  DCHECK(CalledOnValidThread());
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-  base::AutoLock locked(lock_);
   ProtocolHandlerMap::const_iterator it = protocol_handler_map_.find(scheme);
   if (it == protocol_handler_map_.end())
     return nullptr;
@@ -101,7 +98,8 @@ net::URLRequestJob* AtomURLRequestJobFactory::MaybeInterceptResponse(
 
 bool AtomURLRequestJobFactory::IsHandledProtocol(
     const std::string& scheme) const {
-  DCHECK(CalledOnValidThread());
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
   return HasProtocolHandler(scheme) ||
       net::URLRequest::IsHandledProtocol(scheme);
 }

atom/browser/net/atom_url_request_job_factory.h

@@ -56,12 +56,10 @@ class AtomURLRequestJobFactory : public net::URLRequestJobFactory {
   bool IsSafeRedirectTarget(const GURL& location) const override;
 
  private:
-  typedef std::map<std::string, ProtocolHandler*> ProtocolHandlerMap;
+  using ProtocolHandlerMap = std::map<std::string, ProtocolHandler*>;
 
   ProtocolHandlerMap protocol_handler_map_;
 
-  mutable base::Lock lock_;
-
   DISALLOW_COPY_AND_ASSIGN(AtomURLRequestJobFactory);
 };
 

docs/api/protocol.md

@@ -28,7 +28,7 @@ was emitted.
 * `handler` Function
 
 Registers a custom protocol of `scheme`, the `handler` would be called with
-`handler(request)` when the a request with registered `scheme` is made.
+`handler(error, request)` when the a request with registered `scheme` is made.
 
 You need to return a request job in the `handler` to specify which type of
 response you would like to send.
@@ -45,11 +45,12 @@ Unregisters the custom protocol of `scheme`.
 
 `value` is an array of custom schemes to be registered to the standard.
 
-## protocol.isHandledProtocol(scheme)
+## protocol.isHandledProtocol(scheme, callback)
 
 * `scheme` String
+* `callback` Function
 
-Returns whether the `scheme` can be handled already.
+`callback` returns a boolean whether the `scheme` can be handled already.
 
 ## protocol.interceptProtocol(scheme, handler)
 

spec/api-protocol-spec.coffee

@@ -7,17 +7,19 @@ protocol = remote.require 'protocol'
 
 describe 'protocol module', ->
   describe 'protocol.registerProtocol', ->
-    it 'throws error when scheme is already registered', (done) ->
-      register = -> protocol.registerProtocol('test1', ->)
+    it 'error when scheme is already registered', (done) ->
+      register = ->
+        protocol.registerProtocol 'test1', (error, request) ->
+          assert error instanceof Error
+          protocol.unregisterProtocol 'test1'
+          done()
       protocol.once 'registered', (event, scheme) ->
         assert.equal scheme, 'test1'
-        assert.throws register, /The scheme is already registered/
-        protocol.unregisterProtocol 'test1'
-        done()
+        register()
       register()
 
     it 'calls the callback when scheme is visited', (done) ->
-      protocol.registerProtocol 'test2', (request) ->
+      protocol.registerProtocol 'test2', (error, request) ->
         assert.equal request.url, 'test2://test2'
         protocol.unregisterProtocol 'test2'
         done()
@@ -169,16 +171,28 @@ describe 'protocol module', ->
           protocol.unregisterProtocol 'atom-file-job'
 
   describe 'protocol.isHandledProtocol', ->
-    it 'returns true if the scheme can be handled', ->
-      assert.equal protocol.isHandledProtocol('file'), true
-      assert.equal protocol.isHandledProtocol('http'), true
-      assert.equal protocol.isHandledProtocol('https'), true
-      assert.equal protocol.isHandledProtocol('atom'), false
+    it 'returns true if the file scheme can be handled', (done) ->
+      protocol.isHandledProtocol 'file', (result) ->
+        assert.equal result, true
+        done()
+    it 'returns true if the http scheme can be handled', (done) ->
+      protocol.isHandledProtocol 'http', (result) ->
+        assert.equal result, true
+        done()
+    it 'returns true if the https scheme can be handled', (done) ->
+      protocol.isHandledProtocol 'https', (result) ->
+        assert.equal result, true
+        done()
+    it 'returns false if the atom scheme cannot be handled', (done) ->
+      protocol.isHandledProtocol 'atom', (result) ->
+        assert.equal result, false
+        done()
 
   describe 'protocol.interceptProtocol', ->
-    it 'throws error when scheme is not a registered one', ->
-      register = -> protocol.interceptProtocol('test-intercept', ->)
-      assert.throws register, /Scheme does not exist/
+    it 'throws error when scheme is not a registered one', (done) ->
+      protocol.interceptProtocol 'test-intercept', (error, request) ->
+        assert error instanceof Error
+        done()
 
     it 'throws error when scheme is a custom protocol', (done) ->
       protocol.once 'unregistered', (event, scheme) ->
@@ -186,9 +200,9 @@ describe 'protocol module', ->
         done()
       protocol.once 'registered', (event, scheme) ->
         assert.equal scheme, 'atom'
-        register = -> protocol.interceptProtocol('test-intercept', ->)
-        assert.throws register, /Scheme does not exist/
-        protocol.unregisterProtocol scheme
+        protocol.interceptProtocol 'test-intercept', (error, request) ->
+          assert error instanceof Error
+          protocol.unregisterProtocol scheme
       protocol.registerProtocol('atom', ->)
 
     it 'returns original job when callback returns nothing', (done) ->
@@ -206,7 +220,7 @@ describe 'protocol module', ->
           error: (xhr, errorType, error) ->
             free()
             assert false, 'Got error: ' + errorType + ' ' + error
-      protocol.interceptProtocol targetScheme, (request) ->
+      protocol.interceptProtocol targetScheme, (error, request) ->
         if process.platform is 'win32'
           pathInUrl = path.normalize request.url.substr(8)
           assert.equal pathInUrl.toLowerCase(), __filename.toLowerCase()