Rework page icon handling.

Setting the page icon is now driven from the corrently-active frame.

- Eliminates a race condition.
- Icon matches actual frame state (not tab URL state).
- Exclusion-rule changes propagate to all frames.

3 files changed, 40 insertions, 64 deletions

diff --git a/background_scripts/main.coffee b/background_scripts/main.coffee
index a5d7ac31..a6e5a4eb 100644
--- a/background_scripts/main.coffee
+++ b/background_scripts/main.coffee
@@ -362,9 +362,6 @@ updateOpenTabs = (tab, deleteFrames = false) ->
   # Frames are recreated on refresh
   delete frameIdsForTab[tab.id] if deleteFrames
 
-setBrowserActionIcon = (tabId,path) ->
-  chrome.browserAction.setIcon({ tabId: tabId, path: path })
-
 chrome.browserAction.setBadgeBackgroundColor
   # This is Vimium blue (from the icon).
   # color: [102, 176, 226, 255]
@@ -384,35 +381,24 @@ setBadge = do ->
       # We wait a few moments. This avoids badge flicker when there are rapid changes.
       timer = setTimeout updateBadge(badge, sender.tab.id), 50
 
-# 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.
-# Exported via root because it's called from the page popup.
-root.updateActiveState = 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) ->
-    setBadge { badge: "" }, tab: { id: tabId }
-    chrome.tabs.sendMessage tabId, { name: "getActiveState" }, (response) ->
-      if response
-        isCurrentlyEnabled = response.enabled
-        currentPasskeys = response.passKeys
-        currentURL = response.url
-        config = isEnabledForUrl { url: currentURL }, { tab: tab }
-        enabled = config.isEnabledForUrl
-        passKeys = config.passKeys
-        if (enabled and passKeys)
-          setBrowserActionIcon(tabId,partialIcon)
-        else if (enabled)
-          setBrowserActionIcon(tabId,enabledIcon)
-        else
-          setBrowserActionIcon(tabId,disabledIcon)
-        # Propagate the new state only if it has changed.
-        if (isCurrentlyEnabled != enabled || currentPasskeys != passKeys)
-          chrome.tabs.sendMessage(tabId, { name: "setState", enabled: enabled, passKeys: passKeys, incognito: tab.incognito })
-      else
-        setBrowserActionIcon tabId, disabledIcon
+# Here's how we set the page icon.  The default is "disabled", so if we do nothing else, then we get the
+# grey-out disabled icon.  Thereafter, we only set tab-specific icons, so there's no need to update the icon
+# when we visit a tab on which Vimium isn't running.
+#
+# For active tabs, when a frame starts, it requests its active state via isEnabledForUrl.  We also check the
+# state every time a frame gets the focus.  Once the frame learns its active state, it updates the current
+# tab's badge (but only if that frame has the focus).
+#
+# Exclusion rule changes (from either the options page or the page popup) propagate via the subsequent focus
+# change.  In particular, whenever a frame next gets the focus, it requests its new state and sets the icon
+# accordingly.
+#
+setIcon = (request, sender) ->
+  path = switch request.icon
+    when "enabled" then "icons/browser_action_enabled.png"
+    when "partial" then "icons/browser_action_partial.png"
+    when "disabled" then "icons/browser_action_disabled.png"
+  chrome.browserAction.setIcon tabId: sender.tab.id, path: path
 
 handleUpdateScrollPosition = (request, sender) ->
   updateScrollPosition(sender.tab, request.scrollX, request.scrollY)
@@ -429,7 +415,6 @@ chrome.tabs.onUpdated.addListener (tabId, changeInfo, tab) ->
     runAt: "document_start"
   chrome.tabs.insertCSS tabId, cssConf, -> chrome.runtime.lastError
   updateOpenTabs(tab) if changeInfo.url?
-  updateActiveState(tabId)
 
 chrome.tabs.onAttached.addListener (tabId, attachedInfo) ->
   # We should update all the tabs in the old window and the new window.
@@ -466,8 +451,6 @@ chrome.tabs.onRemoved.addListener (tabId) ->
   delete frameIdsForTab[tabId]
   delete urlForTab[tabId]
 
-chrome.tabs.onActiveChanged.addListener (tabId, selectInfo) -> updateActiveState(tabId)
-
 unless chrome.sessions
   chrome.windows.onRemoved.addListener (windowId) -> delete tabQueue[windowId]
 
@@ -681,6 +664,7 @@ sendRequestHandlers =
   refreshCompleter: refreshCompleter
   createMark: Marks.create.bind(Marks)
   gotoMark: Marks.goto.bind(Marks)
+  setIcon: setIcon
   setBadge: setBadge
 
 # We always remove chrome.storage.local/findModeRawQueryListIncognito on startup.
diff --git a/content_scripts/vimium_frontend.coffee b/content_scripts/vimium_frontend.coffee
index 3577e6f3..b96157c1 100644
--- a/content_scripts/vimium_frontend.coffee
+++ b/content_scripts/vimium_frontend.coffee
@@ -162,6 +162,7 @@ initializePreDomReady = ->
     isEnabledForUrl = false
     chrome.runtime.sendMessage = ->
     chrome.runtime.connect = ->
+    window.removeEventListener "focus", onFocus
 
   requestHandlers =
     hideUpgradeNotification: -> HUD.hideUpgradeNotification()
@@ -173,8 +174,6 @@ initializePreDomReady = ->
     getScrollPosition: -> scrollX: window.scrollX, scrollY: window.scrollY
     setScrollPosition: (request) -> setScrollPosition request.scrollX, request.scrollY
     executePageCommand: executePageCommand
-    getActiveState: getActiveState
-    setState: setState
     currentKeyQueue: (request) ->
       keyQueue = request.keyQueue
       handlerStack.bubbleEvent "registerKeyQueue", { keyQueue: keyQueue }
@@ -183,7 +182,7 @@ initializePreDomReady = ->
     # 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' or request.name == 'setState'
+    return unless isEnabledForUrl
     # These requests are delivered to the options page, but there are no handlers there.
     return if request.handler in [ "registerFrame", "frameFocused", "unregisterFrame" ]
     sendResponse requestHandlers[request.name](request, sender)
@@ -210,35 +209,21 @@ window.initializeWhenEnabled = ->
     for type in [ "keydown", "keypress", "keyup", "click", "focus", "blur", "mousedown" ]
       do (type) -> installListener window, type, (event) -> handlerStack.bubbleEvent type, event
     installListener document, "DOMActivate", (event) -> handlerStack.bubbleEvent 'DOMActivate', event
-    installListener window, "focus", registerFocus
     installedListeners = true
     FindModeHistory.init()
 
-setState = (request) ->
-  isEnabledForUrl = request.enabled
-  passKeys = request.passKeys
-  isIncognitoMode = request.incognito
-  initializeWhenEnabled() if isEnabledForUrl
-  handlerStack.bubbleEvent "registerStateChange",
-    enabled: isEnabledForUrl
-    passKeys: passKeys
-
-getActiveState = ->
-  Mode.updateBadge()
-  # getActiveState is called in each frame within the tab.  However, only the response from the first frame is
-  # handled on the background page.  Therefore, exclusion rule changes are not propagated to other frames.
-  # So, we force a state update for this frame, just in case.
-  # FIXME(smblott): This could be avoided if settings were propagated via chrome.storage.
-  checkIfEnabledForUrl()
-  return enabled: isEnabledForUrl, passKeys: passKeys, url: window.location.toString()
-
 #
-# The backend needs to know which frame has focus, and the active URL.
+# Whenever we get the focus:
+# - Reload settings (they may have changed).
+# - Tell the background page this frame's URL.
+# - Check if we should be enabled.
 #
-registerFocus = ->
-  # settings may have changed since the frame last had focus
-  settings.load()
-  chrome.runtime.sendMessage handler: "frameFocused", frameId: frameId, url: window.location.toString()
+onFocus = (event) ->
+  if event.target == window
+    settings.load()
+    chrome.runtime.sendMessage handler: "frameFocused", frameId: frameId, url: window.location.toString()
+    checkIfEnabledForUrl()
+window.addEventListener "focus", onFocus
 
 #
 # Initialization tasks that must wait for the document to be ready.
@@ -577,6 +562,15 @@ checkIfEnabledForUrl = (onStartUp = false) ->
     handlerStack.bubbleEvent "registerStateChange",
       enabled: isEnabledForUrl
       passKeys: passKeys
+    # Update the page icon, if necessary.
+    if document.hasFocus()
+      chrome.runtime.sendMessage
+        handler: "setIcon"
+        icon:
+          if isEnabledForUrl and not passKeys then "enabled"
+          else if isEnabledForUrl then "partial"
+          else "disabled"
+
 
 # Exported to window, but only for DOM tests.
 window.refreshCompletionKeys = (response) ->
diff --git a/pages/options.coffee b/pages/options.coffee
index 61b055c2..f60f3bb4 100644
--- a/pages/options.coffee
+++ b/pages/options.coffee
@@ -295,8 +295,6 @@ initPopupPage = ->
       Option.saveOptions()
       $("saveOptions").innerHTML = "Saved"
       $("saveOptions").disabled = true
-      chrome.tabs.query { windowId: chrome.windows.WINDOW_ID_CURRENT, active: true }, (tabs) ->
-        chrome.extension.getBackgroundPage().updateActiveState(tabs[0].id)
 
     $("saveOptions").addEventListener "click", saveOptions