Remove tabs from TabMap onLoading instead of onBeforeNavigate on Safari

The "beforeNavigate" event on Safari isn't dispatched when a new URL was entered in the address bar. So we can not rely on it. But using the onLoading event instead, for validating the TabMap works as well on Safari. However on Chrome we have to stick to onBeforeNavigate, because of the TabMap must be validated before chrome.webRequest.onHeadersReceived.

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

http://codereview.adblockplus.org/5735785512828928/diff/5629499534213120/safa...
File safari/ext/background.js (right):

http://codereview.adblockplus.org/5735785512828928/diff/5629499534213120/safa...
safari/ext/background.js:113: TabMap = function(deleteTabOnLoading)
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.

http://codereview.adblockplus.org/5735785512828928/diff/5629499534213120/safa...
safari/ext/background.js:115: this._tabs = [];
This property is no longer in use.

http://codereview.adblockplus.org/5735785512828928/diff/5629499534213120/safa...
safari/ext/background.js:158: tab.onLoading.addListener(this.delete);
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?

http://codereview.adblockplus.org/5735785512828928/diff/5629499534213120/safa...
safari/ext/background.js:171: TabMap.prototype["delete"] = function(tab)
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:12]

http://codereview.adblockplus.org/5735785512828928/diff/5629499534213120/safa...
File safari/ext/background.js (right):

http://codereview.adblockplus.org/5735785512828928/diff/5629499534213120/safa...
safari/ext/background.js:113: TabMap = function(deleteTabOnLoading)
On 2014/01/23 13:34:13, Wladimir Palant wrote:
> 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.

Done.

http://codereview.adblockplus.org/5735785512828928/diff/5629499534213120/safa...
safari/ext/background.js:115: this._tabs = [];
On 2014/01/23 13:34:13, Wladimir Palant wrote:
> This property is no longer in use.

Done.

http://codereview.adblockplus.org/5735785512828928/diff/5629499534213120/safa...
safari/ext/background.js:158: tab.onLoading.addListener(this.delete);
On 2014/01/23 13:34:13, Wladimir Palant wrote:
> 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?

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

http://codereview.adblockplus.org/5735785512828928/diff/5629499534213120/safa...
safari/ext/background.js:171: TabMap.prototype["delete"] = function(tab)
On 2014/01/23 13:34:13, Wladimir Palant wrote:
> 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.

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:36]

LGTM but I would still like to see the nit fixed.

http://codereview.adblockplus.org/5735785512828928/diff/5629499534213120/safa...
File safari/ext/background.js (right):

http://codereview.adblockplus.org/5735785512828928/diff/5629499534213120/safa...
safari/ext/background.js:171: TabMap.prototype["delete"] = function(tab)
On 2014/01/23 13:51:13, Sebastian Noack wrote:
> It leads to a syntax error, at least in Safari and Chrome, because it is a
> reserved keyword.

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]

http://codereview.adblockplus.org/5735785512828928/diff/5629499534213120/safa...
File safari/ext/background.js (right):

http://codereview.adblockplus.org/5735785512828928/diff/5629499534213120/safa...
safari/ext/background.js:171: TabMap.prototype["delete"] = function(tab)
On 2014/01/23 14:44:37, Wladimir Palant wrote:
> On 2014/01/23 13:51:13, Sebastian Noack wrote:
> > It leads to a syntax error, at least in Safari and Chrome, because it is a
> > reserved keyword.
> 
> 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.

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.