Issue 5264 - Return page objects instead of page IDs

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
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 "use strict";
 
 (function()
 {
   let nonEmptyPageMaps = new Set();
 
   let PageMap = ext.PageMap = function()
   {
     this._map = new Map();
   };
   PageMap.prototype = {
-    _deleteByPageId(id)
-    {
-      for (let page of this.keys().filter(page => page.id == id))
-        this.delete(page);
+    _delete(id)
+    {
+      this._map.delete(id);
+
+      if (this._map.size == 0)
+        nonEmptyPageMaps.delete(this);
     },
     keys()
     {
-      return Array.from(this._map.keys());
+      return Array.from(this._map.keys()).map(ext.getPage);

[kzar, 2017/05/23 11:47:59]

I wonder if you could do `return Array.prototype.map.call(this._map.keys(),
ext.getPage)`?

[Sebastian Noack, 2017/05/23 12:16:54]

Please don't. Array generics are non-standard and deprecated:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...

[Manish Jethani, 2017/05/23 12:32:27]

I think Dave meant Array.prototype.map rather than Array.map. The former isn't
going anywhere, though for better or worse it won't work with a non-array-like
object.

To map an iterable regardless of its type you have to use a function like this: 

https://codereview.adblockplus.org/29417597/patch/29417598/29417600

If you have generators support, then this: 

function* mapIterable(iterable, callback)
{
  for (let entry of iterable)
    yield callback(entry);
}

Here we don't have too many keys though, it's OK to create an intermediate
array.

[Sebastian Noack, 2017/05/23 15:09:18]

Sorry, you are right, I mixed things up, calling array method's through the
prototype isn't generally an issue, but as you pointed out it only works with
array-like objects.

I wouldn't be opposed to make keys() a generator function. All browsers this
code is running on support generators, and it doesn't seem to make the code any
more complicated. However, this would imply that there is no usage of keys()
which access the result like an array, I didn't check this.

Alternatively, we could create an array like below:

  let pages = [];
  for (let key of this._map.keys())
    pages.push(ext.getPage(key));
  return pages;

In the end it doesn't matter much, so as far as I'm concerned, just pick the
approach that seems best to you.

[kzar, 2017/05/23 16:15:04]

Acknowledged.

     },
     get(page)
     {
-      return this._map.get(page);
+      return this._map.get(page.id);
     },
     set(page, value)
     {
-      this._map.set(page, value);
+      this._map.set(page.id, value);
       nonEmptyPageMaps.add(this);
     },
     has(page)
     {
-      return this._map.has(page);
+      return this._map.has(page.id);
     },
     clear()
     {
       this._map.clear();
       nonEmptyPageMaps.delete(this);
     },
     delete(page)
     {
-      this._map.delete(page);
-
-      if (this._map.size == 0)
-        nonEmptyPageMaps.delete(this);
+      this._delete(page.id);
     }
   };
 
   ext._removeFromAllPageMaps = pageId =>
   {
     for (let pageMap of nonEmptyPageMaps)
-      pageMap._deleteByPageId(pageId);
+      pageMap._delete(pageId);
   };
 
   /* Pages */
 
   let Page = ext.Page = function(tab)
   {
     this.id = tab.id;
     this._url = tab.url && new URL(tab.url);
 
     this.browserAction = new BrowserAction(tab.id);
@@ skipping 652 matching lines @@
   ext.windows = {
     create(createData, callback)
     {
       chrome.windows.create(createData, createdWindow =>
       {
         afterTabLoaded(callback)(createdWindow.tabs[0]);
       });
     }
   };
 }());