feat: migrate webRequest module to NetworkService (Part 7) (#19820)

* fix: gin treats Function as Dictionary when doing convertions

* fix: check if listener exists

* fix: listener callback should be executed in next tick

* feat: make InProgressRequest work

* test: re-enable protocol test that relies on webRequest

* chore: merge conditions

shell/browser/api/atom_api_web_request_ns.cc

@@ -236,6 +236,10 @@ const char* WebRequestNS::GetTypeName() {
   return "WebRequest";
 }
 
+bool WebRequestNS::HasListener() const {
+  return !(simple_listeners_.empty() && response_listeners_.empty());
+}
+
 int WebRequestNS::OnBeforeRequest(extensions::WebRequestInfo* info,
                                   const network::ResourceRequest& request,
                                   net::CompletionOnceCallback callback,
@@ -316,16 +320,25 @@ template <typename Listener, typename Listeners, typename Event>
 void WebRequestNS::SetListener(Event event,
                                Listeners* listeners,
                                gin::Arguments* args) {
+  v8::Local<v8::Value> arg;
+
   // { urls }.
   std::set<URLPattern> patterns;
   gin::Dictionary dict(args->isolate());
-  args->GetNext(&dict) && dict.Get("urls", &patterns);
+  if (args->GetNext(&arg) && !arg->IsFunction()) {
+    // Note that gin treats Function as Dictionary when doing convertions, so we
+    // have to explicitly check if the argument is Function before trying to
+    // convert it to Dictionary.
+    if (gin::ConvertFromV8(args->isolate(), arg, &dict)) {
+      dict.Get("urls", &patterns);
+      args->GetNext(&arg);
+    }
+  }
 
   // Function or null.
-  v8::Local<v8::Value> value;
   Listener listener;
-  if (!args->GetNext(&listener) &&
+  if (arg.IsEmpty() ||
-      !(args->GetNext(&value) && value->IsNull())) {
+      !(gin::ConvertFromV8(args->isolate(), arg, &listener) || arg->IsNull())) {
     args->ThrowTypeError("Must pass null or a Function");
     return;
   }
@@ -340,6 +353,9 @@ template <typename... Args>
 void WebRequestNS::HandleSimpleEvent(SimpleEvent event,
                                      extensions::WebRequestInfo* request_info,
                                      Args... args) {
+  if (!base::Contains(simple_listeners_, event))
+    return;
+
   const auto& info = simple_listeners_[event];
   if (!MatchesFilterCondition(request_info, info.url_patterns))
     return;
@@ -357,6 +373,9 @@ int WebRequestNS::HandleResponseEvent(ResponseEvent event,
                                       net::CompletionOnceCallback callback,
                                       Out out,
                                       Args... args) {
+  if (!base::Contains(response_listeners_, event))
+    return net::OK;
+
   const auto& info = response_listeners_[event];
   if (!MatchesFilterCondition(request_info, info.url_patterns))
     return net::OK;
@@ -395,7 +414,11 @@ void WebRequestNS::OnListenerResult(uint64_t id,
       ReadFromResponse(isolate, &dict, out);
   }
 
-  std::move(callbacks_[id]).Run(result);
+  // The ProxyingURLLoaderFactory expects the callback to be executed
+  // asynchronously, because it used to work on IO thread before NetworkService.
+  base::SequencedTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::BindOnce(std::move(callbacks_[id]), result));
+  callbacks_.erase(id);
 }
 
 // static

shell/browser/api/atom_api_web_request_ns.h

@@ -57,6 +57,7 @@ class WebRequestNS : public gin::Wrappable<WebRequestNS>, public WebRequestAPI {
   ~WebRequestNS() override;
 
   // WebRequestAPI:
+  bool HasListener() const override;
   int OnBeforeRequest(extensions::WebRequestInfo* info,
                       const network::ResourceRequest& request,
                       net::CompletionOnceCallback callback,

shell/browser/net/proxying_url_loader_factory.cc

@@ -697,14 +697,13 @@ void ProxyingURLLoaderFactory::CreateLoaderAndStart(
     return;
   }
 
-  // Pass-through to the original factory.
+  if (!web_request_api()->HasListener()) {
-  target_factory_->CreateLoaderAndStart(std::move(loader), routing_id,
+    // Pass-through to the original factory.
-                                        request_id, options, request,
+    target_factory_->CreateLoaderAndStart(
-                                        std::move(client), traffic_annotation);
+        std::move(loader), routing_id, request_id, options, request,
-
+        std::move(client), traffic_annotation);
-  // TODO(zcbenz): Remove the |CreateLoaderAndStart| call and create
+    return;
-  // InProgressRequest when the webRequest API is used.
+  }
-  return;
 
   // The request ID doesn't really matter. It just needs to be unique
   // per-BrowserContext so extensions can make sense of it.  Note that

shell/browser/net/proxying_url_loader_factory.h

@@ -31,6 +31,7 @@ class WebRequestAPI {
                               const std::set<std::string>& set_headers,
                               int error_code)>;
 
+  virtual bool HasListener() const = 0;
   virtual int OnBeforeRequest(extensions::WebRequestInfo* info,
                               const network::ResourceRequest& request,
                               net::CompletionOnceCallback callback,

spec-main/api-protocol-spec.ts

@@ -532,7 +532,7 @@ describe('protocol module', () => {
       expect({ ...qs.parse(r.data) }).to.deep.equal(postData)
     })
 
-    it.skip('can use custom session', async () => {
+    it('can use custom session', async () => {
       const customSession = session.fromPartition('custom-ses', { cache: false })
       customSession.webRequest.onBeforeRequest((details, callback) => {
         expect(details.url).to.equal('http://fake-host/')