Better state management for passkeys.

3 files changed, 55 insertions, 50 deletions

diff --git a/background_scripts/main.coffee b/background_scripts/main.coffee
index 68d1236b..5412ead5 100644
--- a/background_scripts/main.coffee
+++ b/background_scripts/main.coffee
@@ -377,37 +377,38 @@ updateOpenTabs = (tab) ->
   # Frames are recreated on refresh
   delete framesForTab[tab.id]
 
-# Updates the browserAction icon to indicated whether Vimium is enabled or disabled on the current page.
-# Also disables Vimium if it is currently enabled but should be disabled according to the url blacklist.
+setBrowserActionIcon = (tabId,path) ->
+  chrome.browserAction.setIcon({ tabId: tabId, path: path })
+
+# Updates the browserAction icon to indicate whether Vimium is enabled or disabled on the current page.
+# Also propagates new enabled/disabled/passkeys state to active window, if necessary.
 # This lets you disable Vimium on a page without needing to reload.
-#
-# Three situations are considered:
-# 1. Active tab is disabled -> disable icon
-# 2. Active tab is enabled and should be enabled -> enable icon
-# 3. Active tab is enabled but should be disabled -> disable icon and disable vimium
 updateActiveState = (tabId) ->
   enabledIcon = "icons/browser_action_enabled.png"
   disabledIcon = "icons/browser_action_disabled.png"
   partialIcon = "icons/browser_action_partial.png"
-  chrome.tabs.get(tabId, (tab) ->
-    # Default to disabled state in case we can't connect to Vimium, primarily for the "New Tab" page.
-    chrome.browserAction.setIcon({ path: disabledIcon })
-    chrome.tabs.sendMessage(tabId, { name: "getActiveState" }, (response) ->
-      isCurrentlyEnabled = (response? && response.enabled)
-      enabledConfig = isEnabledForUrl({url: tab.url})
-      shouldBeEnabled = enabledConfig.isEnabledForUrl
-      shouldHavePassKeys = enabledConfig.passKeys
-
-      if (isCurrentlyEnabled)
+  chrome.tabs.get tabId, (tab) ->
+    chrome.tabs.sendMessage tabId, { name: "getActiveState" }, (response) ->
+      if response
+        isCurrentlyEnabled = response.enabled
+        currentPasskeys = response.passKeys
+        # TODO:
+        # isEnabledForUrl is quite expensive to run each time we change tab.  Perhaps memoize it?
+        shouldHaveConfig = isEnabledForUrl({url: tab.url})
+        shouldBeEnabled = shouldHaveConfig.isEnabledForUrl
+        shouldHavePassKeys = shouldHaveConfig.passKeys
         if (shouldBeEnabled and shouldHavePassKeys)
-          chrome.browserAction.setIcon({ path: partialIcon })
+          setBrowserActionIcon(tabId,partialIcon)
         else if (shouldBeEnabled)
-          chrome.browserAction.setIcon({ path: enabledIcon })
+          setBrowserActionIcon(tabId,enabledIcon)
         else
-          chrome.browserAction.setIcon({ path: disabledIcon })
-          chrome.tabs.sendMessage(tabId, { name: "disableVimium" })
+          setBrowserActionIcon(tabId,disabledIcon)
+        # Propagate the new state only if it has changed.
+        if (isCurrentlyEnabled != shouldBeEnabled || currentPasskeys != shouldHavePassKeys)
+          chrome.tabs.sendMessage(tabId, { name: "setState", enabled: shouldBeEnabled, passKeys: shouldHavePassKeys })
       else
-        chrome.browserAction.setIcon({ path: disabledIcon })))
+        # We didn't get a response from the front end, so Vimium isn't running.
+        setBrowserActionIcon(tabId,disabledIcon)
 
 handleUpdateScrollPosition = (request, sender) ->
   updateScrollPosition(sender.tab, request.scrollX, request.scrollY)
diff --git a/content_scripts/vimium_frontend.coffee b/content_scripts/vimium_frontend.coffee
index 34473e96..d34ea761 100644
--- a/content_scripts/vimium_frontend.coffee
+++ b/content_scripts/vimium_frontend.coffee
@@ -13,7 +13,8 @@ findModeQueryHasResults = false
 findModeAnchorNode = null
 isShowingHelpDialog = false
 keyPort = null
-# Users can disable Vimium on URL patterns via the settings page.
+# Users can disable Vimium on URL patterns via the settings page.  The following two variables
+# (isEnabledForUrl and passKeys) control Vimium's enabled/disabled behaviour.
 isEnabledForUrl = true
 passKeys = null
 keyQueue = null
@@ -117,43 +118,45 @@ initializePreDomReady = ->
     getScrollPosition: -> scrollX: window.scrollX, scrollY: window.scrollY
     setScrollPosition: (request) -> setScrollPosition request.scrollX, request.scrollY
     executePageCommand: executePageCommand
-    getActiveState: -> { enabled: isEnabledForUrl }
-    disableVimium: disableVimium
+    getActiveState: -> { enabled: isEnabledForUrl, passKeys: passKeys }
+    setState: setState
     currentKeyQueue: (request) -> keyQueue = request.keyQueue
 
   chrome.runtime.onMessage.addListener (request, sender, sendResponse) ->
     # in the options page, we will receive requests from both content and background scripts. ignore those
     # from the former.
     return if sender.tab and not sender.tab.url.startsWith 'chrome-extension://'
-    return unless isEnabledForUrl or request.name == 'getActiveState'
+    return unless isEnabledForUrl or request.name == 'getActiveState' or request.name == 'setState'
     sendResponse requestHandlers[request.name](request, sender)
     # Ensure the sendResponse callback is freed.
     false
 
-#
-# This is called once the background page has told us that Vimium should be enabled for the current URL.
-#
-initializeWhenEnabled = ->
-  document.addEventListener("keydown", onKeydown, true)
-  document.addEventListener("keypress", onKeypress, true)
-  document.addEventListener("keyup", onKeyup, true)
-  document.addEventListener("focus", onFocusCapturePhase, true)
-  document.addEventListener("blur", onBlurCapturePhase, true)
-  document.addEventListener("DOMActivate", onDOMActivate, true)
-  enterInsertModeIfElementIsFocused()
+# Wrapper to install event listeners.  Syntactic sugar.
+installListener = (event, callback) -> document.addEventListener(event, callback, true)
 
 #
-# Used to disable Vimium without needing to reload the page.
-# This is called if the current page's url is blacklisted using the popup UI.
+# This is called once the background page has told us that Vimium should be enabled for the current URL.
+# We enable/disable Vimium by toggling isEnabledForUrl.  The alternative, installing or uninstalling
+# listeners, is error prone.  It's more difficult to keep track of the state.
 #
-disableVimium = ->
-  document.removeEventListener("keydown", onKeydown, true)
-  document.removeEventListener("keypress", onKeypress, true)
-  document.removeEventListener("keyup", onKeyup, true)
-  document.removeEventListener("focus", onFocusCapturePhase, true)
-  document.removeEventListener("blur", onBlurCapturePhase, true)
-  document.removeEventListener("DOMActivate", onDOMActivate, true)
-  isEnabledForUrl = false
+installedListeners = false
+initializeWhenEnabled = (newPassKeys=undefined) ->
+  isEnabledForUrl = true
+  passKeys = passKeys if typeof(newPassKeys) != 'undefined'
+  if (!installedListeners)
+    installListener "keydown", (event) -> if isEnabledForUrl then onKeydown(event) else true
+    installListener "keypress", (event) -> if isEnabledForUrl then onKeypress(event) else true
+    installListener "keyup", (event) -> if isEnabledForUrl then onKeyup(event) else true
+    installListener "focus", (event) -> if isEnabledForUrl then onFocusCapturePhase(event) else true
+    installListener "blur", (event) -> if isEnabledForUrl then onBlurCapturePhase(event)
+    installListener "DOMActivate", (event) -> if isEnabledForUrl then onDOMActivate(event)
+    enterInsertModeIfElementIsFocused()
+    installedListeners = true
+
+setState = (request) ->
+  isEnabledForUrl = request.enabled
+  passKeys = request.passKeys
+  initializeWhenEnabled(passKeys) if isEnabledForUrl and !installedListeners
 
 #
 # The backend needs to know which frame has focus.
@@ -484,8 +487,7 @@ checkIfEnabledForUrl = ->
   chrome.runtime.sendMessage { handler: "isEnabledForUrl", url: url }, (response) ->
     isEnabledForUrl = response.isEnabledForUrl
     if (isEnabledForUrl)
-      initializeWhenEnabled()
-      passKeys = response.passKeys
+      initializeWhenEnabled(response.passKeys)
     else if (HUD.isReady())
       # Quickly hide any HUD we might already be showing, e.g. if we entered insert mode on page load.
       HUD.hide()
diff --git a/pages/popup.coffee b/pages/popup.coffee
index 5332a596..41fc17a9 100644
--- a/pages/popup.coffee
+++ b/pages/popup.coffee
@@ -4,9 +4,11 @@ onLoad = ->
     # Check if we have an existing exclusing rule for this page.
     isEnabled = chrome.extension.getBackgroundPage().isEnabledForUrl(url: tab.url)
     if isEnabled.matchingUrl
+      console.log isEnabled
       # There is an existing rule for this page.
       pattern = isEnabled.matchingUrl
-      pattern += " " + isEnabled.passKeys if isEnabled.passKeys
+      if isEnabled.passKeys
+        pattern += " " + isEnabled.passKeys
       document.getElementById("popupInput").value = pattern
     else
       # No existing exclusion rule.