Issue 5130 - Changes listener to register <all_urls> and filter non ws http requests

ext/background.js

Index: ext/background.js
===================================================================
--- a/ext/background.js
+++ b/ext/background.js
@@ -558,16 +558,24 @@
           }
         }
       });
     });
   });
 
   chrome.webRequest.onBeforeRequest.addListener(details =>
   {
+    // Filter out requests from non web protocols since Chrome version
+    // 58 added support for webSocket connections to the webRequests API

[Jon Sonesen, 2017/04/20 19:19:58]

this comment should likely be improved. perhaps we can decide on that here

[Sebastian Noack, 2017/04/20 19:34:15]

"Filter out requests from non web protocols. Ideally, we'd explicitly specify
the protocols we are interested in (i.e. http://, https://, ws:// and wss://)
with the url patterns, given below, when adding this listener. But
unfortunately, Chrome <=57 doesn't support the WebSocket protocol and is causing
an error if it is given."

[Jon Sonesen, 2017/04/20 19:53:06]

Acknowledged. Thank you, I think that is good.

+    let blockURLs = new RegExp("https?|wss?");
+    let url = new URL(details.url);

[Sebastian Noack, 2017/04/20 19:34:15]

We are now creating two URL objects, one to validate the protocol here, and on
we pass to ext.webRequest.onBeforeRequest._dispatch(). Please reuse the URL
object which we create here below.

[Jon Sonesen, 2017/04/20 19:53:06]

Ah, sorry about that.

+    if (!blockURLs.exec(url.protocol))

[Sebastian Noack, 2017/04/20 19:34:15]

Since the regular expression doesn't make the code here much simpler, I'd rather
check for the 4 possible values explicitly (which is also faster):

  if (url.protocol == 'http:' || url.protocol == 'https:' ||
      url.protocol == 'ws:' || url.protocol == 'wss:')

[Sebastian Noack, 2017/04/20 19:36:22]

I accidentally inverted the logic, should be of course:

   if (url.protocol != 'http:' && url.protocol != 'https:' &&
       url.protocol != 'ws:' && url.protocol != 'wss:')

[Jon Sonesen, 2017/04/20 19:53:06]

Yeah, happy to use this instead.

+    {

[Sebastian Noack, 2017/04/20 19:34:15]

We generally omit the braces if there is only one line in the block.

[Jon Sonesen, 2017/04/20 19:53:06]

Done.

+      return {cancel: false};

[Sebastian Noack, 2017/04/20 19:34:15]

For consistency with the other bailout condition below, and to avoid creating an
unnecessary object, please omit the optional return value (which is equivalent
to returning {cancel: false}).

[Jon Sonesen, 2017/04/20 19:53:06]

Done.

+    }
     // The high-level code isn't interested in requests that aren't
     // related to a tab or requests loading a top-level document,
     // those should never be blocked.
     if (details.tabId == -1 || details.type == "main_frame")
       return;
 
     // We are looking for the frame that contains the element which
     // has triggered this request. For most requests (e.g. images) we
@@ -588,17 +596,17 @@
         type.toUpperCase(),
         new Page({id: details.tabId}),
         frame
       );
 
       if (results.indexOf(false) != -1)
         return {cancel: true};
     }
-  }, {urls: ["https://*/*", "http://*/*", "ws://*/*", "wss://*/*"]}, ["blocking"]);
+  }, {urls: ["<all_urls>"]}, ["blocking"]);
 
 
   /* Message passing */
 
   chrome.runtime.onMessage.addListener((message, rawSender, sendResponse) =>
   {
     let sender = {};