Make global marks consistent with other tab creation operations.

This makes global marks use the same code for opening new tabs as other
operations.  Thus, the new tab appears in the same position as it would
if it were created, for example, via the vomnibar.

To do this properly, we move the three tab-creating functions into an
object which can be (and is) exported (TabOperations).

Also fixes these three operations such that they all take the same
arguments; that is (request, callback).  Not all of these callbacks are
being used.  But we should be consistent.

2 files changed, 29 insertions, 30 deletions

diff --git a/background_scripts/main.coffee b/background_scripts/main.coffee
index 4cb78441..40d570ee 100644
--- a/background_scripts/main.coffee
+++ b/background_scripts/main.coffee
@@ -187,31 +187,28 @@ getCompletionKeysRequest = (request, keysToCheck = "") ->
   completionKeys: generateCompletionKeys(keysToCheck)
   validFirstKeys: validFirstKeys
 
-#
-# Opens the url in the current tab.
-#
-openUrlInCurrentTab = (request) ->
-  chrome.tabs.getSelected(null,
-    (tab) -> chrome.tabs.update(tab.id, { url: Utils.convertToUrl(request.url) }))
+TabOperations =
+  # Opens the url in the current tab.
+  openUrlInCurrentTab: (request, callback = (->)) ->
+    chrome.tabs.getSelected null, (tab) ->
+      callback = (->) unless typeof callback == "function"
+      chrome.tabs.update tab.id, { url: Utils.convertToUrl(request.url) }, callback
 
-#
-# Opens request.url in new tab and switches to it if request.selected is true.
-#
-openUrlInNewTab = (request, callback) ->
-  chrome.tabs.getSelected null, (tab) ->
-    tabConfig =
-      url: Utils.convertToUrl request.url
-      index: tab.index + 1
-      selected: true
-      windowId: tab.windowId
+  # Opens request.url in new tab and switches to it if request.selected is true.
+  openUrlInNewTab: (request, callback = (->)) ->
+    chrome.tabs.getSelected null, (tab) ->
+      tabConfig =
+        url: Utils.convertToUrl request.url
+        index: tab.index + 1
+        selected: true
+        windowId: tab.windowId
       openerTabId: tab.id
-    # FIXME(smblott). openUrlInNewTab is being called in two different ways with different arguments.  We
-    # should refactor it such that this check on callback isn't necessary.
-    callback = (->) unless typeof callback == "function"
-    chrome.tabs.create tabConfig, callback
+      callback = (->) unless typeof callback == "function"
+      chrome.tabs.create tabConfig, callback
 
-openUrlInIncognito = (request) ->
-  chrome.windows.create({ url: Utils.convertToUrl(request.url), incognito: true})
+  openUrlInIncognito: (request, callback = (->)) ->
+    callback = (->) unless typeof callback == "function"
+    chrome.windows.create {url: Utils.convertToUrl(request.url), incognito: true}, callback
 
 #
 # Copies or pastes some data (request.data) to/from the clipboard.
@@ -257,7 +254,7 @@ BackgroundCommands =
       if url == "pages/blank.html"
         # "pages/blank.html" does not work in incognito mode, so fall back to "chrome://newtab" instead.
         url = if tab.incognito then "chrome://newtab" else chrome.runtime.getURL url
-      openUrlInNewTab { url }, callback
+      TabOperations.openUrlInNewTab { url }, callback
   duplicateTab: (callback) ->
     chrome.tabs.getSelected(null, (tab) ->
       chrome.tabs.duplicate(tab.id)
@@ -297,8 +294,8 @@ BackgroundCommands =
               scrollX: tabQueueEntry.scrollX,
               scrollY: tabQueueEntry.scrollY)
           callback()))
-  openCopiedUrlInCurrentTab: (request) -> openUrlInCurrentTab({ url: Clipboard.paste() })
-  openCopiedUrlInNewTab: (request) -> openUrlInNewTab({ url: Clipboard.paste() })
+  openCopiedUrlInCurrentTab: (request) -> TabOperations.openUrlInCurrentTab({ url: Clipboard.paste() })
+  openCopiedUrlInNewTab: (request) -> TabOperations.openUrlInNewTab({ url: Clipboard.paste() })
   togglePinTab: (request) ->
     chrome.tabs.getSelected(null, (tab) ->
       chrome.tabs.update(tab.id, { pinned: !tab.pinned }))
@@ -653,9 +650,9 @@ portHandlers =
 sendRequestHandlers =
   getCompletionKeys: getCompletionKeysRequest
   getCurrentTabUrl: getCurrentTabUrl
-  openUrlInNewTab: openUrlInNewTab
-  openUrlInIncognito: openUrlInIncognito
-  openUrlInCurrentTab: openUrlInCurrentTab
+  openUrlInNewTab: TabOperations.openUrlInNewTab
+  openUrlInIncognito: TabOperations.openUrlInIncognito
+  openUrlInCurrentTab: TabOperations.openUrlInCurrentTab
   openOptionsPageInNewTab: openOptionsPageInNewTab
   registerFrame: registerFrame
   unregisterFrame: unregisterFrame
@@ -726,7 +723,7 @@ showUpgradeMessage = ->
           Settings.set "previousVersion", currentVersion
           chrome.notifications.onClicked.addListener (id) ->
             if id == notificationId
-              openUrlInNewTab url: "https://github.com/philc/vimium#release-notes"
+              TabOperations.openUrlInNewTab url: "https://github.com/philc/vimium#release-notes"
     else
       # We need to wait for the user to accept the "notifications" permission.
       chrome.permissions.onAdded.addListener showUpgradeMessage
@@ -741,3 +738,5 @@ chrome.windows.getAll { populate: true }, (windows) ->
       chrome.tabs.sendMessage(tab.id, { name: "getScrollPosition" }, createScrollPositionHandler())
 
 showUpgradeMessage()
+
+root.TabOperations = TabOperations
diff --git a/background_scripts/marks.coffee b/background_scripts/marks.coffee
index 6e5f08ba..70ec1c17 100644
--- a/background_scripts/marks.coffee
+++ b/background_scripts/marks.coffee
@@ -82,7 +82,7 @@ Marks =
           @gotoPositionInTab extend markInfo, tabId: tab.id
       else
         # There is no existing matching tab, we'll have to create one.
-        chrome.tabs.create { url: @getBaseUrl markInfo.url }, (tab) =>
+        TabOperations.openUrlInNewTab { url: @getBaseUrl markInfo.url }, (tab) =>
           # Note. tabLoadedHandlers is defined in "main.coffee".  The handler below will be called when the tab
           # is loaded, its DOM is ready and it registers with the background page.
           tabLoadedHandlers[tab.id] = => @gotoPositionInTab extend markInfo, tabId: tab.id