Remove tabs from TabMap onLoading instead of onBeforeNavigate on Safari

safari/ext/background.js

Index: safari/ext/background.js
===================================================================
--- a/safari/ext/background.js
+++ b/safari/ext/background.js
@@ -84,7 +84,6 @@
     this.browserAction = new BrowserAction(this);
 
     this.onLoading = new LoadingTabEventTarget(tab);
-    this.onBeforeNavigate = new TabEventTarget(tab, "beforeNavigate", false);
     this.onCompleted = new TabEventTarget(tab, "navigate", false);
     this.onActivated = new TabEventTarget(tab, "activate", false);
     this.onRemoved = new TabEventTarget(tab, "close", false);
@@ -111,76 +110,80 @@
     }
   };
 
-  TabMap = function(deleteTabOnBeforeNavigate)
+  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.

-    this._values = [];
+    this._data = [];
 
-    this._deleteOnEvent = this._deleteOnEvent.bind(this);
-    this._deleteTabOnBeforeNavigate = deleteTabOnBeforeNavigate;
+    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;
+    },
+    _delete: function(tab)
+    {
+      // delay so that other onClosed listeners can still look this tab up
+      setTimeout(this.delete.bind(this, tab), 0);
+    },
     get: function(tab) {
       var idx;
 
-      if (!tab || (idx = this._tabs.indexOf(tab._tab)) == -1)
+      if (!tab || (idx = this._indexOf(tab)) == -1)
         return null;
 
-      return this._values[idx];
+      return this._data[idx].value;
     },
     set: function(tab, value)
     {
-      var idx = this._tabs.indexOf(tab._tab);
+      var idx = this._indexOf(tab);
 
       if (idx != -1)
-        this._values[idx] = value;
+        this._data[idx].value = value;
       else
       {
-        this._tabs.push(tab._tab);
-        this._values.push(value);
+        this._data.push({value: value, tab: tab});
 
-        tab._tab.addEventListener("close", this._deleteOnEvent, false);
-        if (this._deleteTabOnBeforeNavigate)
-          tab._tab.addEventListener("beforeNavigate", this._deleteOnEvent, false);
+        tab.onRemoved.addListener(this._delete);
+        if (this._deleteTabOnLoading)
+          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._tabs.indexOf(tab._tab) != -1;
+      return this._indexOf(tab) != -1;
     },
     clear: function()
     {
-      while (this._tabs.length > 0)
-        this._delete(this._tabs[0]);
-    },
-    _delete: function(tab)
-    {
-      var idx = this._tabs.indexOf(tab);
-
-      if (idx != -1)
-      {
-        this._tabs.splice(idx, 1);
-        this._values.splice(idx, 1);
-
-        tab.removeEventListener("close", this._deleteOnEvent, false);
-        tab.removeEventListener("beforeNavigate", this._deleteOnEvent, false);
-      }
-    },
-    _deleteOnEvent: function(event)
-    {
-      // delay so that other event handlers can still look this tab up
-      setTimeout(this._delete.bind(this, event.target), 0);
+      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.

   {
-    this._delete(tab._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),
-    onBeforeNavigate: new TabEventTarget(safari.application, "beforeNavigate", true),
     onCompleted: new TabEventTarget(safari.application, "navigate", true),
     onActivated: new TabEventTarget(safari.application, "activate", true),
     onRemoved: new TabEventTarget(safari.application, "close", true)