philc/vimium — pull request #830 by sainaen (2013-05-07 17:59:07+00:00)
Title: removeTab() shouldn't add callback to handlers list if it's undefined

There is an error thrown with the code from current `master` branch:

```
Error in event handler for 'tabs.onSelectionChanged':
Cannot call method 'call' of undefined
TypeError: Cannot call method 'call' of undefined
```

It happens after you close the tab with `x`. Inspired by the success of my other pull request I decided to track it down. =)

As a result I found that there is special case: when command is `background` and `noRepeat` at the same time it's handled by that line in [main.coffee](https://github.com/philc/vimium/blob/master/background_scripts/main.coffee#L490):

```coffee
BackgroundCommands[registryEntry.command]()
```

And there is only one such command (`background` + `noRepeat`) — `removeTab`, which looks like this:

```coffee
removeTab: (callback) ->
chrome.tabs.getSelected(null, (tab) ->
chrome.tabs.remove(tab.id)
# We can't just call the callback here because we need to wait
# for the selection to change to consider this action done.
selectionChangedHandlers.push(callback))
```

As you can see it adds given callback to handlers list, but here `callback` is undefined, because this function was called on line 490 without any arguments.

I think we could fix with additional if statement to `removeTab()`, like this:

```coffee
removeTab: (callback) ->
chrome.tabs.getSelected(null, (tab) ->
chrome.tabs.remove(tab.id)
selectionChangedHandlers.push(callback) if callback?)
```

Am I right? Is it ok to fix this problem like that? Or it will be better to check if callback is defined before calling it, like this:

```coffee
chrome.tabs.onSelectionChanged.addListener (tabId, selectionInfo) ->
if (selectionChangedHandlers.length > 0)
selectionChangedHandlers.pop()?.call()
```