Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 5735785512828928: Remove tabs from TabMap onLoading instead of onBeforeNavigate on Safari (Closed)

Created:
Jan. 23, 2014, 11:33 a.m. by Sebastian Noack
Modified:
Jan. 23, 2014, 3:28 p.m.
Visibility:
Public.

Description

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.

Patch Set 1 #

Total comments: 10

Patch Set 2 : Addressed comments #

Patch Set 3 : Apparently you can used the reserved keyword "delete" as key when defining inline objects #

Unified diffs Side-by-side diffs Delta from patch set Stats (+57 lines, -48 lines) Patch
M chrome/ext/background.js View 1 2 5 chunks +22 lines, -14 lines 0 comments Download
M safari/ext/background.js View 1 2 2 chunks +35 lines, -34 lines 0 comments Download

Messages

Total messages: 5
Sebastian Noack
Jan. 23, 2014, 11:40 a.m. (2014-01-23 11:40:42 UTC) #1
Wladimir Palant
http://codereview.adblockplus.org/5735785512828928/diff/5629499534213120/safari/ext/background.js File safari/ext/background.js (right): http://codereview.adblockplus.org/5735785512828928/diff/5629499534213120/safari/ext/background.js#newcode113 safari/ext/background.js:113: TabMap = function(deleteTabOnLoading) Having to change a parameter name ...
Jan. 23, 2014, 1:34 p.m. (2014-01-23 13:34:13 UTC) #2
Sebastian Noack
http://codereview.adblockplus.org/5735785512828928/diff/5629499534213120/safari/ext/background.js File safari/ext/background.js (right): http://codereview.adblockplus.org/5735785512828928/diff/5629499534213120/safari/ext/background.js#newcode113 safari/ext/background.js:113: TabMap = function(deleteTabOnLoading) On 2014/01/23 13:34:13, Wladimir Palant wrote: ...
Jan. 23, 2014, 1:51 p.m. (2014-01-23 13:51:12 UTC) #3
Wladimir Palant
LGTM but I would still like to see the nit fixed. http://codereview.adblockplus.org/5735785512828928/diff/5629499534213120/safari/ext/background.js File safari/ext/background.js (right): ...
Jan. 23, 2014, 2:44 p.m. (2014-01-23 14:44:36 UTC) #4
Sebastian Noack
Jan. 23, 2014, 3:27 p.m. (2014-01-23 15:27:44 UTC) #5
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.

Powered by Google App Engine
This is Rietveld