Remove tabs from TabMap onLoading instead of onBeforeNavigate on Safari

safari/ext/background.js

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
  * Copyright (C) 2006-2013 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 92 matching lines @@
     },
     sendMessage: function(message, responseCallback)
     {
       _sendMessage(
         message, responseCallback,
         this._tab.page, this._tab
       );
     }
   };
 
-  TabMap = function(deleteTabOnLoading)

[Wladimir Palant, 2014/01/23 13:34:13]

Having to change a parameter name just because the implementation changes is a
clear sign of bad naming. This should describe what the parameter is doing, not
how it is implemented - e.g. deleteOnPageUnload.

[Sebastian Noack, 2014/01/23 13:51:13]

Done.

-  {
-    this._tabs = [];

[Wladimir Palant, 2014/01/23 13:34:13]

This property is no longer in use.

[Sebastian Noack, 2014/01/23 13:51:13]

Done.

+  TabMap = function(deleteOnPageUnload)
+  {
     this._data = [];
+    this._deleteOnPageUnload = deleteOnPageUnload;
 
     this.delete = this.delete.bind(this);
     this._delete = this._delete.bind(this);
-
-    this._deleteTabOnLoading = deleteTabOnLoading;
   };
   TabMap.prototype =
   {
     _indexOf: function(tab)
     {
       for (var i = 0; i < this._data.length; i++)
         if (this._data[i].tab._tab == tab._tab)
           return i;
 
       return -1;
@@ skipping 15 matching lines @@
     {
       var idx = this._indexOf(tab);
 
       if (idx != -1)
         this._data[idx].value = value;
       else
       {
         this._data.push({value: value, tab: tab});
 
         tab.onRemoved.addListener(this._delete);
-        if (this._deleteTabOnLoading)
+        if (this._deleteOnPageUnload)
           tab.onLoading.addListener(this.delete);

[Wladimir Palant, 2014/01/23 13:34:13]

What if about:blank is loaded into the tab or some other special page? Will
onLoading still fire or do we need to listen to the unload event in content
scripts?

[Sebastian Noack, 2014/01/23 13:51:13]

I've just checked that. And surprisingly our content scripts also runs on
about:blank and even on pages within third-party extension bundles. :D

       }
     },
     has: function(tab)
     {
       return this._indexOf(tab) != -1;
     },
     clear: function()
     {
       while (this._data.length > 0)
         this.delete(this._data[0].tab);
-    }
-  };
-  TabMap.prototype["delete"] = function(tab)

[Wladimir Palant, 2014/01/23 13:34:13]

I am still wondering why this method isn't defined along with other prototype
functions above. delete is a reserved keyword but {delete: function() ...} is
valid regardless.

[Sebastian Noack, 2014/01/23 13:51:13]

It leads to a syntax error, at least in Safari and Chrome, because it is a
reserved keyword.

[Wladimir Palant, 2014/01/23 14:44:37]

No, for me it doesn't (tested in Safari 7.0 and Chrome 32). Maybe you tested it
outside an expression meaning that the object was interpreted as a block. Either
way, {"delete": ...} should always work, even if some older Safari version has a
problem without the quotation marks.

[Sebastian Noack, 2014/01/23 15:27:44]

You are right. I tried it in the console, and in the console you have to wrap
the object into parentheses, if you want to use reserved keywords as key.
Otherwise you get a syntax error. So I thought first that this isn't possible.

-  {
-    var idx = this._indexOf(tab);
-
-    if (idx != -1)
-    {
-      tab = this._data[idx].tab;
-      this._data.splice(idx, 1);
-
-      tab.onRemoved.removeListener(this._delete);
-      tab.onLoading.removeListener(this.delete);
+    },
+    delete: function(tab)
+    {
+      var idx = this._indexOf(tab);
+
+      if (idx != -1)
+      {
+        tab = this._data[idx].tab;
+        this._data.splice(idx, 1);
+
+        tab.onRemoved.removeListener(this._delete);
+        tab.onLoading.removeListener(this.delete);
+      }
     }
   };
 
   ext.tabs = {
     onLoading: new LoadingTabEventTarget(safari.application),
     onCompleted: new TabEventTarget(safari.application, "navigate", true),
     onActivated: new TabEventTarget(safari.application, "activate", true),
     onRemoved: new TabEventTarget(safari.application, "close", true)
   };
 
@@ skipping 516 matching lines @@
     for (var i = 0; i < contextMenuItems.length; i++)
     {
       if (contextMenuItems[i].id == event.command)
       {
         contextMenuItems[i].onclick(event.userInfo.srcUrl, new Tab(safari.application.activeBrowserWindow.activeTab));
         break;
       }
     }
   }, false);
 })();