Issue 5161 - Use maps and sets where appropriate

ext/background.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2017 eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 10 matching lines @@
 {
   let nonEmptyPageMaps = new Set();
 
   let PageMap = ext.PageMap = function()
   {
     this._map = new Map();
   };
   PageMap.prototype = {
     _delete(id)
     {
-      let map = this._map;
-
-      let deleted = map.delete(id);
-
-      if (deleted && map.size == 0)
+      this._map.delete(id);
+
+      if (this._map.size == 0)
         nonEmptyPageMaps.delete(this);
     },
     keys()
     {
-      return [...ext.mapIterable(this._map.keys(), ext.getPage)];
+      return this._map.keys();
     },
     get(page)
     {
       return this._map.get(page.id);
     },
     set(page, value)
     {
-      let map = this._map;
-      let prevSize = map.size;
-
-      map.set(page.id, value);
-
-      if (prevSize == 0)
-        nonEmptyPageMaps.add(this);
+      this._map.set(page.id, value);
+      nonEmptyPageMaps.add(this);
     },
     has(page)
     {
       return this._map.has(page.id);
     },
     clear()
     {
-      let map = this._map;

[Sebastian Noack, 2017/04/29 22:46:32]

Why do you import this property into a local variable?

[Manish Jethani, 2017/05/04 14:47:33]

I wrongly assumed that this would make it faster. Now I know that's not the
case, at least in v8. I've changed it.

-
-      if (map.size == 0)
-        return;
-
-      map.clear();
+      this._map.clear();
       nonEmptyPageMaps.delete(this);
     },
     delete(page)
     {
       this._delete(page.id);
     }
   };
 
   ext._removeFromAllPageMaps = pageId =>
   {
@@ skipping 93 matching lines @@
     let frames = framesOfTabs.get(tabId);
     if (!frames)
     {
       frames = new Map();
       framesOfTabs.set(tabId, frames);
     }
 
     let frame = frames.get(frameId);
     if (!frame)
     {
-      frame = Object.create(null);

[Sebastian Noack, 2017/04/29 22:46:32]

Why do we use an object with no prototype here again? If it is not used as a map
we should just use an object literal.

[Manish Jethani, 2017/05/04 14:47:33]

Again I assumed that an object with no prototype would have faster property
lookups (both hits and misses). This is in fact the case in Firefox after a
recent update, but in Chrome it actually slows things down. I've changed it
back.

+      frame = {};
       frames.set(frameId, frame);
     }
 
     return frame;
   }
 
   function updatePageFrameStructure(frameId, tabId, url, parentFrameId)
   {
     if (frameId == 0)
     {
@@ skipping 315 matching lines @@
   });
 
 
   /* Web requests */
 
   let framesOfTabs = new Map();
 
   ext.getFrame = (tabId, frameId) =>
   {
     let frames = framesOfTabs.get(tabId);
-    if (frames)
-      return frames.get(frameId);

[Sebastian Noack, 2017/04/29 22:46:32]

How about:

  return frames && frames.get(frameId);

[Manish Jethani, 2017/05/04 14:47:33]

Done.

+    return frames && frames.get(frameId);
   };
 
   let handlerBehaviorChangedQuota =
     chrome.webRequest.MAX_HANDLER_BEHAVIOR_CHANGED_CALLS_PER_10_MINUTES;
 
   function propagateHandlerBehaviorChange()
   {
     // Make sure to not call handlerBehaviorChanged() more often than allowed
     // by chrome.webRequest.MAX_HANDLER_BEHAVIOR_CHANGED_CALLS_PER_10_MINUTES.
     // Otherwise Chrome notifies the user that this extension is causing issues.
@@ skipping 29 matching lines @@
     {
       chrome.webNavigation.getAllFrames({tabId: tab.id}, details =>
       {
         if (details && details.length > 0)
         {
           let frames = new Map();
           framesOfTabs.set(tab.id, frames);
 
           for (let detail of details)
           {
-            let frame = Object.create(null);
-            frame.url = new URL(detail.url);
-
+            let frame = {url: new URL(detail.url)};
             frames.set(detail.frameId, frame);
-          }
-
-          for (let detail of details)
-          {
-            let {parentFrameId} = detail;
-
-            if (parentFrameId != -1)
-              frames.get(detail.frameId).parent = frames.get(parentFrameId);
+
+            if (detail.parentFrameId != -1)
+              frame.parent = frames.get(detail.parentFrameId);
           }
         }
       });
     });
   });
 
   chrome.webRequest.onBeforeRequest.addListener(details =>
   {
     // 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;
+
+    // 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.
+    let url = new URL(details.url);
+    if (url.protocol != "http:" && url.protocol != "https:" &&
+        url.protocol != "ws:" && url.protocol != "wss:")
       return;
 
     // We are looking for the frame that contains the element which
     // has triggered this request. For most requests (e.g. images) we
     // can just use the request's frame ID, but for subdocument requests
     // (e.g. iframes) we must instead use the request's parent frame ID.
     let {frameId, type} = details;
     if (type == "sub_frame")
-    {
       frameId = details.parentFrameId;
-      type = "SUBDOCUMENT";
-    }
 
     let frame = ext.getFrame(details.tabId, frameId);
     if (frame)
     {
       let results = ext.webRequest.onBeforeRequest._dispatch(
-        new URL(details.url),
-        type.toUpperCase(),
-        new Page({id: details.tabId}),
-        frame
+        url, type, new Page({id: details.tabId}), frame
       );
 
       if (results.indexOf(false) != -1)
         return {cancel: true};
     }
   }, {urls: ["<all_urls>"]}, ["blocking"]);
 
 
   /* Message passing */
 
@@ skipping 10 matching lines @@
         url: new URL(rawSender.url),
         get parent()
         {
           let frames = framesOfTabs.get(rawSender.tab.id);
 
           if (!frames)
             return null;
 
           let frame = frames.get(rawSender.frameId);
           if (frame)
             return frame.parent || null;

[Sebastian Noack, 2017/04/29 22:46:32]

This seems unrelated.

[Manish Jethani, 2017/05/04 14:47:33]

frame.parent is either an object or undefined. I thought we'd consistently
return null here (see further above in this code). We could also try to make
sure it's always either an object or null to begin with, which we were trying
earlier, but that's too much work to maintain. It's better IMO not to set the
parent property on the frame object if there's no parent object available.

We could also just return undefined if we don't care about that.

 
           return frames.get(0) || null;
         }
       };
     }
 
     return ext.onMessage._dispatch(
       message, sender, sendResponse
     ).indexOf(true) != -1;
   });
@@ skipping 92 matching lines @@
   ext.windows = {
     create(createData, callback)
     {
       chrome.windows.create(createData, createdWindow =>
       {
         afterTabLoaded(callback)(createdWindow.tabs[0]);
       });
     }
   };
 }());