From 41bdac83d2fd450569013dd5cfdb78239143ba24 Mon Sep 17 00:00:00 2001
From: Stephen Blott
Date: Mon, 1 Sep 2014 12:07:24 +0100
Subject: Structured passkeys, internally and on the options and popup pages.

---
 background_scripts/exclusions.coffee   | 77 ++++++++++++++++++++++++++++
 background_scripts/main.coffee         | 87 ++++++++++----------------------
 background_scripts/settings.coffee     | 17 ++++---
 content_scripts/vimium_frontend.coffee | 15 +++---
 lib/exclusion_rule.coffee              | 46 +++++++++++++++++
 manifest.json                          |  3 ++
 pages/options.coffee                   | 23 +++++++--
 pages/options.html                     | 68 ++++++++++++++-----------
 pages/popup.coffee                     | 91 ++++++++++++++++++++++++++++------
 pages/popup.html                       | 22 +++++---
 tests/unit_tests/exclusion_test.coffee | 31 ++++++------
 11 files changed, 331 insertions(+), 149 deletions(-)
 create mode 100644 background_scripts/exclusions.coffee
 create mode 100644 lib/exclusion_rule.coffee

diff --git a/background_scripts/exclusions.coffee b/background_scripts/exclusions.coffee
new file mode 100644
index 00000000..326989bf
--- /dev/null
+++ b/background_scripts/exclusions.coffee
@@ -0,0 +1,77 @@
+root = exports ? window
+
+RegexpCache =
+  cache: {}
+  get: (pattern) ->
+    if regexp = @cache[pattern]
+      regexp
+    else
+      @cache[pattern] = new RegExp("^" + pattern.replace(/\*/g, ".*") + "$")
+
+# The Exclusions class manages the exclusion rule setting.
+# An exclusion is an object with two attributes: pattern and passKeys.
+# The exclusions are an array of such objects (because the order matters).
+
+root.Exclusions = Exclusions =
+
+  rules: Settings.get("exclusionRules")
+
+  # Return the first exclusion rule matching the URL, or null.
+  getRule: (url) ->
+    for rule in @rules
+      return rule if url.match(RegexpCache.get(rule.pattern))
+    return null
+
+  setRules: (rules) ->
+    @rules = rules.filter (rule) -> rule and rule.pattern
+    Settings.set("exclusionRules",@rules)
+
+  postUpdateHook: (rules) ->
+    @rules = rules
+
+  # Update an existing rule or add a new rule.
+  updateOrAdd: (newRule) ->
+    seen = false
+    @rules.push(newRule)
+    @setRules(@rules.map (rule) -> if rule.pattern == newRule.pattern then (if seen then null else seen = newRule) else rule)
+
+  remove: (pattern) ->
+    @setRules(@rules.filter((rule) -> rule.pattern != pattern))
+
+  # DOM handling for the options page; populate the exclusionRules option.
+  populateOption: (exclusionRulesElement,enableSaveButton) ->
+    populate = =>
+      while exclusionRulesElement.firstChild
+        exclusionRulesElement.removeChild(exclusionRulesElement.firstChild)
+      for rule in @rules
+        exclusionRulesElement.appendChild(ExclusionRule.buildRuleElement(rule,enableSaveButton))
+      exclusionRulesElement.appendChild(ExclusionRule.buildRuleElement({pattern: "", passKeys: ""},enableSaveButton))
+    populate()
+    return {
+      saveOption: =>
+        @setRules(ExclusionRule.extractRule(element) for element in exclusionRulesElement.getElementsByClassName('exclusionRow'))
+        populate()
+      restoreToDefault: =>
+        Settings.clear("exclusionRules")
+        populate()
+    }
+
+# Development and debug only.
+# Enable this (temporarily) to restore legacy exclusion rules from backup.
+if false and Settings.has("excludedUrlsBackup")
+  Settings.clear("exclusionRules")
+  Settings.set("excludedUrls",Settings.get("excludedUrlsBackup"))
+
+if not Settings.has("exclusionRules") and Settings.has("excludedUrls")
+  # Migration from the legacy exclusion rules (settings: "excludedUrls" -> "exclusionRules").
+
+  parseLegacyRules = (lines) ->
+    for line in lines.trim().split("\n").map((line) -> line.trim())
+      if line.length and line.indexOf("#") != 0 and line.indexOf('"') != 0
+        parse = line.split(/\s+/)
+        { pattern: parse[0], passKeys: parse[1..].join("") }
+
+  Exclusions.setRules(parseLegacyRules(Settings.get("excludedUrls")))
+  # We'll keep a backup of the excludedUrls setting, just in case (and for testing).
+  Settings.set("excludedUrlsBackup",Settings.get("excludedUrls")) if not Settings.has("excludedUrlsBackup")
+  Settings.clear("excludedUrls")
diff --git a/background_scripts/main.coffee b/background_scripts/main.coffee
index dda1beae..4111ac06 100644
--- a/background_scripts/main.coffee
+++ b/background_scripts/main.coffee
@@ -73,57 +73,25 @@ getCurrentTabUrl = (request, sender) -> sender.tab.url
 # whether any keys should be passed through to the underlying page.
 #
 root.isEnabledForUrl = isEnabledForUrl = (request) ->
-  # Excluded URLs are stored as a series of URL expressions and optional passKeys, separated by newlines.
-  # Lines for which the first non-blank character is "#" or '"' are comments.
-  excludedLines = (line.trim() for line in Settings.get("excludedUrls").split("\n"))
-  excludedSpecs = (line.split(/\s+/) for line in excludedLines when line and line.indexOf("#") != 0 and line.indexOf('"') != 0)
-  for spec in excludedSpecs
-    url = spec[0]
-    # The user can add "*" to the URL which means ".*"
-    regexp = new RegExp("^" + url.replace(/\*/g, ".*") + "$")
-    if request.url.match(regexp)
-      passKeys = spec[1..].join("")
-      if passKeys
-        # Enabled, but not for these keys.
-        return { isEnabledForUrl: true, passKeys: passKeys, matchingUrl: url }
-      # Wholly disabled.
-      return { isEnabledForUrl: false, passKeys: "", matchingUrl: url }
-  # Enabled (the default).
-  { isEnabledForUrl: true, passKeys: undefined, matchingUrl: undefined }
-
-# Called by the popup UI. Strips leading/trailing whitespace and ignores new empty strings.  If an existing
-# exclusion rule has been changed, then the existing rule is updated.  Otherwise, the new rule is added.
-root.addExcludedUrl = (url) ->
-  return unless url = url.trim()
-
-  parse = url.split(/\s+/)
-  url = parse[0]
-  passKeys = parse[1..].join(" ")
-  newSpec = (if passKeys then url + " " + passKeys else url)
-
-  excludedUrls = Settings.get("excludedUrls").split("\n")
-  excludedUrls.push(newSpec)
-
-  # Update excludedUrls.
-  # Try to keep the list as unchanged as possible: same order, same comments, same blank lines.
-  seenNew = false
-  newExcludedUrls = []
-  for spec in excludedUrls
-    spec = spec.trim()
-    parse = spec.split(/\s+/)
-    # Keep just one copy of the new exclusion rule.
-    if parse.length and parse[0] == url
-      if !seenNew
-        newExcludedUrls.push(newSpec)
-        seenNew = true
-      continue
-    # And just keep everything else.
-    newExcludedUrls.push(spec)
-    
-  Settings.set("excludedUrls", newExcludedUrls.join("\n"))
-
-  chrome.tabs.query({ windowId: chrome.windows.WINDOW_ID_CURRENT, active: true },
-    (tabs) -> updateActiveState(tabs[0].id))
+  rule = Exclusions.getRule(request.url)
+  return { rule: rule, isEnabledForUrl: true,  passKeys: rule.passKeys } if rule and rule.passKeys
+  return { rule: rule, isEnabledForUrl: false, passKeys: "" } if rule
+  return { rule: rule, isEnabledForUrl: true,  passKeys: "" }
+
+# Called by the popup UI.  If an existing exclusion rule has been changed, then the existing rule is updated.
+# Otherwise, the new rule is added.
+root.addExclusionRule = (pattern,passKeys) ->
+  if pattern = pattern.trim()
+    Exclusions.updateOrAdd({ pattern: pattern, passKeys: passKeys })
+    chrome.tabs.query({ windowId: chrome.windows.WINDOW_ID_CURRENT, active: true },
+      (tabs) -> updateActiveState(tabs[0].id))
+
+# Called by the popup UI.  Remove all existing exclusion rules with this pattern.
+root.removeExclusionRule = (pattern) ->
+  if pattern = pattern.trim()
+    Exclusions.remove(pattern)
+    chrome.tabs.query({ windowId: chrome.windows.WINDOW_ID_CURRENT, active: true },
+      (tabs) -> updateActiveState(tabs[0].id))
 
 saveHelpDialogSettings = (request) ->
   Settings.set("helpDialog_showAdvancedCommands", request.showAdvancedCommands)
@@ -389,24 +357,21 @@ updateActiveState = (tabId) ->
   partialIcon = "icons/browser_action_partial.png"
   chrome.tabs.get tabId, (tab) ->
     chrome.tabs.sendMessage tabId, { name: "getActiveState" }, (response) ->
-      console.log 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)
+        config = isEnabledForUrl({url: tab.url})
+        enabled = config.isEnabledForUrl
+        passKeys = config.passKeys
+        if (enabled and passKeys)
           setBrowserActionIcon(tabId,partialIcon)
-        else if (shouldBeEnabled)
+        else if (enabled)
           setBrowserActionIcon(tabId,enabledIcon)
         else
           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 })
+        if (isCurrentlyEnabled != enabled || currentPasskeys != passKeys)
+          chrome.tabs.sendMessage(tabId, { name: "setState", enabled: enabled, passKeys: passKeys })
       else
         # We didn't get a response from the front end, so Vimium isn't running.
         setBrowserActionIcon(tabId,disabledIcon)
diff --git a/background_scripts/settings.coffee b/background_scripts/settings.coffee
index 34d6e879..63dd851b 100644
--- a/background_scripts/settings.coffee
+++ b/background_scripts/settings.coffee
@@ -35,6 +35,9 @@ root.Settings = Settings =
     searchEngines: (value) ->
       root.Settings.parseSearchEngines value
 
+    exclusionRules: (value) ->
+      root.Exclusions.postUpdateHook value
+
   # postUpdateHooks convenience wrapper
   performPostUpdateHook: (key, value) ->
     @postUpdateHooks[key] value if @postUpdateHooks[key]
@@ -81,14 +84,14 @@ root.Settings = Settings =
       div > .vimiumHintMarker > .matchingCharacter {
       }
       """
-    excludedUrls:
-      """
-      # Disable Vimium on Gmail:
-      http*://mail.google.com/*
+    # Default exclusion rules.
+    exclusionRules:
+      [
+        # Disable Vimium on Google Reader, and use Gmail's own j/k bindings.
+        { pattern: "http*://www.google.com/reader/*", passKeys: "" }
+        { pattern: "http*://mail.google.com/*", passKeys: "jk" }
+      ]
 
-      # Use Facebook's own j/k bindings:
-      http*://www.facebook.com/* jk
-      """
     # NOTE: If a page contains both a single angle-bracket link and a double angle-bracket link, then in
     # most cases the single bracket link will be "prev/next page" and the double bracket link will be
     # "first/last page", so we put the single bracket first in the pattern string so that it gets searched
diff --git a/content_scripts/vimium_frontend.coffee b/content_scripts/vimium_frontend.coffee
index 137b9d1a..e2ffa7f0 100644
--- a/content_scripts/vimium_frontend.coffee
+++ b/content_scripts/vimium_frontend.coffee
@@ -15,6 +15,8 @@ isShowingHelpDialog = false
 keyPort = null
 # Users can disable Vimium on URL patterns via the settings page.  The following two variables
 # (isEnabledForUrl and passKeys) control Vimium's enabled/disabled behaviour.
+# "passKeys" are keys which would normally be handled by Vimium, but are disabled on this tab, and therefore
+# are passed through to the underlying page.
 isEnabledForUrl = true
 passKeys = null
 keyQueue = null
@@ -130,6 +132,7 @@ initializePreDomReady = ->
     # These requests are delivered to the options page, but there are no handlers there.
     return if request.handler == "registerFrame" or request.handler == "frameFocused"
     sendResponse requestHandlers[request.name](request, sender)
+    # Ensure the sendResponse callback is freed.
     false
 
 # Wrapper to install event listeners.  Syntactic sugar.
@@ -141,9 +144,9 @@ installListener = (event, callback) -> document.addEventListener(event, callback
 # listeners, is error prone.  It's more difficult to keep track of the state.
 #
 installedListeners = false
-initializeWhenEnabled = (newPassKeys=undefined) ->
+initializeWhenEnabled = (newPassKeys) ->
   isEnabledForUrl = true
-  passKeys = passKeys if typeof(newPassKeys) != 'undefined'
+  passKeys = newPassKeys
   if (!installedListeners)
     installListener "keydown", (event) -> if isEnabledForUrl then onKeydown(event) else true
     installListener "keypress", (event) -> if isEnabledForUrl then onKeypress(event) else true
@@ -328,14 +331,11 @@ extend window,
 
       false
 
-# Should this keyChar be passed to the underlying page?
+# Decide whether this keyChar be passed to the underlying page.
 # Keystrokes are *never* considered passKeys if the keyQueue is not empty.  So, for example, if 't' is a
 # passKey, then 'gt' and '99t' will neverthless be handled by vimium.
-# TODO: This currently only works for unmodified keys (so not for '<c-a>', or the like).  It's not clear if
-# this is a problem or not.  I don't recall coming across a web page with modifier key bindings.  Such
-# bindings might be too likely to conflict with browsers' native bindings.
 isPassKey = ( keyChar ) ->
-  !keyQueue and passKeys and 0 <= passKeys.indexOf keyChar
+  return !keyQueue and 0 <= passKeys.indexOf(keyChar)
 
 handledKeydownEvents = []
 
@@ -365,7 +365,6 @@ onKeypress = (event) ->
         handleKeyCharForFindMode(keyChar)
         DomUtils.suppressEvent(event)
       else if (!isInsertMode() && !findMode)
-        # Is this keyChar is to be passed to the underlying page?
         if (isPassKey keyChar)
           return undefined
         if (currentCompletionKeys.indexOf(keyChar) != -1)
diff --git a/lib/exclusion_rule.coffee b/lib/exclusion_rule.coffee
new file mode 100644
index 00000000..0942e7cf
--- /dev/null
+++ b/lib/exclusion_rule.coffee
@@ -0,0 +1,46 @@
+root = exports ? window
+
+# Operations to build the DOM on the options page for a single exclusion rule.
+
+root.ExclusionRule =
+
+  # Build a DOM table row (a "tr") for this rule.
+  buildRuleElement: (rule,enableSaveButton) ->
+    pattern = @buildInput(enableSaveButton,rule.pattern,"URL pattern","pattern")
+    passKeys = @buildInput(enableSaveButton,rule.passKeys,"Excluded keys","passKeys")
+    row = document.createElement("tr")
+    row.className = "exclusionRow"
+    remove = document.createElement("input")
+    remove.type = "button"
+    remove.value = "\u2716" # A cross.
+    remove.className = "exclusionRemoveButton"
+    remove.addEventListener "click", ->
+      row.parentNode.removeChild(row)
+      enableSaveButton()
+    row.appendChild(pattern)
+    row.appendChild(passKeys)
+    row.appendChild(remove)
+    # NOTE: Since the order of exclusions matters, it would be nice to have "Move Up" and "Move Down" buttons,
+    # too.  But this option is pretty cluttered already.
+    row
+
+  # Build DOM (a "td" containing an "input") for a single input element.
+  buildInput: (enableSaveButton,value,placeholder,cls) ->
+    input = document.createElement("input")
+    input.setAttribute("placeholder",placeholder)
+    input.type = "text"
+    input.value = value
+    input.className = cls
+    input.addEventListener "keyup", enableSaveButton, false
+    input.addEventListener "change", enableSaveButton, false
+    container = document.createElement("td")
+    container.appendChild(input)
+    container
+
+  # Build a new exclusion rule from the given element.  This is the reverse of the two methods above.
+  extractRule: (element) ->
+    patternElement = element.firstChild
+    passKeysElement = patternElement.nextSibling
+    pattern = patternElement.firstChild.value.trim()
+    passKeys = passKeysElement.firstChild.value.trim()
+    if pattern then { pattern: pattern, passKeys: passKeys } else null
diff --git a/manifest.json b/manifest.json
index 9a4e0167..3f4c877d 100644
--- a/manifest.json
+++ b/manifest.json
@@ -12,7 +12,9 @@
       "background_scripts/commands.js",
       "lib/clipboard.js",
       "background_scripts/sync.js",
+      "lib/exclusion_rule.js",
       "background_scripts/settings.js",
+      "background_scripts/exclusions.js",
       "background_scripts/completion.js",
       "background_scripts/marks.js",
       "background_scripts/main.js"
@@ -35,6 +37,7 @@
              "lib/dom_utils.js",
              "lib/handler_stack.js",
              "lib/clipboard.js",
+             "lib/exclusion_rule.js",
              "content_scripts/link_hints.js",
              "content_scripts/vomnibar.js",
              "content_scripts/scroller.js",
diff --git a/pages/options.coffee b/pages/options.coffee
index d4767da6..be0eccfb 100644
--- a/pages/options.coffee
+++ b/pages/options.coffee
@@ -2,11 +2,17 @@ $ = (id) -> document.getElementById id
 
 bgSettings = chrome.extension.getBackgroundPage().Settings
 
-editableFields = [ "scrollStepSize", "excludedUrls", "linkHintCharacters", "linkHintNumbers",
+editableFields = [ "scrollStepSize", "linkHintCharacters", "linkHintNumbers",
   "userDefinedLinkHintCss", "keyMappings", "filterLinkHints", "previousPatterns",
   "nextPatterns", "hideHud", "regexFindMode", "searchUrl", "searchEngines"]
 
-canBeEmptyFields = ["excludedUrls", "keyMappings", "userDefinedLinkHintCss", "searchEngines"]
+canBeEmptyFields = ["keyMappings", "userDefinedLinkHintCss", "searchEngines"]
+
+# Settings which handle their own DOM and callbacks for the options page.
+# See populateOption in ../background_scripts/exclusions.coffee for an example.
+selfHandlingFields =
+  exclusionRules: (args...) -> chrome.extension.getBackgroundPage().Exclusions.populateOption(args...)
+selfHandlingCallbacks = {}
 
 document.addEventListener "DOMContentLoaded", ->
   populateOptions()
@@ -68,6 +74,10 @@ saveOptions = ->
     $(fieldName).value = fieldValue
     $(fieldName).setAttribute "savedValue", fieldValue
     bgSettings.performPostUpdateHook fieldName, fieldValue
+ 
+  # Self-handling options save themselves.
+  for field of selfHandlingFields
+    selfHandlingCallbacks[field].saveOption() if selfHandlingCallbacks[field].saveOption
 
   $("saveOptions").disabled = true
 
@@ -76,14 +86,17 @@ populateOptions = ->
   for field in editableFields
     val = bgSettings.get(field) or ""
     setFieldValue $(field), val
-  onDataLoaded()
+  # Self-handling options build their own DOM, and provide callbacks for saveOptions and restoreToDefaults.
+  for field of selfHandlingFields
+    selfHandlingCallbacks[field] = selfHandlingFields[field]($(field),enableSaveButton)
 
 restoreToDefaults = ->
-  return unless confirm "Are you sure you want to return Vimium's settings to their defaults?"
-
   for field in editableFields
     val = bgSettings.defaults[field] or ""
     setFieldValue $(field), val
+  # Self-handling options restore their own defaults.
+  for field of selfHandlingFields
+    selfHandlingCallbacks[field].restoreToDefault() if selfHandlingCallbacks[field].restoreToDefault
   onDataLoaded()
   enableSaveButton()
 
diff --git a/pages/options.html b/pages/options.html
index 07dcab1d..c9fc5a63 100644
--- a/pages/options.html
+++ b/pages/options.html
@@ -6,6 +6,7 @@
     <script src="../lib/dom_utils.js"></script>
     <script src="../lib/handler_stack.js"></script>
     <script src="../lib/clipboard.js"></script>
+    <script src="../lib/exclusion_rule.js"></script>
     <script src="../content_scripts/link_hints.js"></script>
     <script src="../content_scripts/vomnibar.js"></script>
     <script src="../content_scripts/scroller.js"></script>
@@ -109,11 +110,6 @@
         width: 40px;
         margin-right: 3px;
       }
-      textarea#excludedUrls {
-        margin-top: 5px;
-        width: 100%;
-        min-height: 100px;
-      }
       textarea#userDefinedLinkHintCss {
         width: 100%;;
         min-height: 100px;
@@ -178,6 +174,30 @@
         padding: 15px 0;
         border-top: 1px solid #eee;
       }
+      /* Ids and classes for rendering exclusionRules */
+      #exclusionScroll {
+        overflow: scroll;
+        overflow-x: hidden;
+        height: 225px;
+        border: 1px solid #bfbfbf;
+        border-radius: 2px;
+        color: #444;
+      }
+      .exclusionRemoveButton {
+        /* cursor: pointer; */
+        /* border: none; */
+        /* background: none; */
+      }
+      input.pattern, input.passKeys {
+        font-family: Consolas, "Liberation Mono", Courier, monospace;
+        font-size: 14px;
+      }
+      .pattern {
+        width: 250px;
+      }
+      .passKeys {
+        width: 120px;
+      }
     </style>
   <link rel="stylesheet" type="text/css" href="../content_scripts/vimium.css" />
 
@@ -196,31 +216,21 @@
           </td>
         </tr>
         <tr>
-          <td colspan="3">
-              Excluded URLs and keys<br/>
-              <div class="help">
-                <div class="example">
-                  <p>
-                    To disable Vimium on a site, use:<br/>
-                    <tt>http*://mail.google.com/*</tt><br/>
-                    This will <i>wholly disable</i> Vimium on Gmail.<br/><br/>
-                    To use Vimium together with a website's own<br/>
-                    key bindings, use:<br/>
-                    <tt>http*://mail.google.com/* jknpc</tt><br/>
-                    This will <i>enable</i> Vimium on Gmail, but pass<br/>
-                    the five listed keys through to Gmail itself.<br/><br/>
-                    One entry per line.<br/>
-                  </p>
-                </div>
+          <td>Excluded URLs<br/>and keys</td>
+          <td>
+            <div class="help">
+              <div class="example">
+                <p>
+                   The left column contains URL patterns.  Vimium will be wholly or partially disabled for URLs matching these patterns.  Patterns are Javascript regular expressions.  Additionally, the symbox "*" matches any zero or more characters.
+                </p>
+                <p>
+                   The right column contains keys which Vimium would would normally handle, but should instead be passed through to the underlying web page (for pages matching the corresponding pattern).  If empty, then Vimium is wholly disabled.
+                </p>
               </div>
-              <!-- Hack: fix a minimum size for the text area (below) so that it is
-                   not too much smaller than its help text (above). -->
-              <!-- FIXME:
-                   This text area should really be broken out into an array
-                   of separate inputs.  However, the whole options page really
-                   needs a workover, so I'm leaving it like this, for now
-                   (Steve, 23 Aug, 14). -->
-              <textarea id="excludedUrls" style="min-height:180px"></textarea>
+            </div>
+            <div id="exclusionScroll">
+               <table id="exclusionRules"></table>
+            </div>
           </td>
         </tr>
         <tbody id='advancedOptions'>
diff --git a/pages/popup.coffee b/pages/popup.coffee
index 41fc17a9..ff943f32 100644
--- a/pages/popup.coffee
+++ b/pages/popup.coffee
@@ -1,27 +1,86 @@
+
+originalRule = undefined
+originalPattern = undefined
+originalPassKeys = undefined
+
 onLoad = ->
   document.getElementById("optionsLink").setAttribute "href", chrome.runtime.getURL("pages/options.html")
   chrome.tabs.getSelected null, (tab) ->
-    # 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
-      if isEnabled.passKeys
-        pattern += " " + isEnabled.passKeys
-      document.getElementById("popupInput").value = pattern
+    if isEnabled.rule
+      # There is an existing exclusion rule for this page.
+      originalRule = isEnabled.rule
+      originalPattern = originalRule.pattern
+      originalPassKeys = originalRule.passKeys
     else
-      # No existing exclusion rule.
+      # There is not an existing exclusion rule.
       # The common use case is to disable Vimium at the domain level.
       # This regexp will match "http://www.example.com/" from "http://www.example.com/path/to/page.html".
-      domain = tab.url.match(/[^\/]*\/\/[^\/]*\//) or tab.url
-      document.getElementById("popupInput").value = domain + "*"
+      domain = (tab.url.match(/[^\/]*\/\/[^\/]*\//) or tab.url) + "*"
+      originalRule = null
+      originalPattern = domain
+      originalPassKeys = ""
+    document.getElementById("popupPattern").value  = originalPattern
+    document.getElementById("popupPassKeys").value = originalPassKeys
+    onChange()
+
+onChange = ->
+  pattern = document.getElementById("popupPattern").value.trim()
+  passKeys = document.getElementById("popupPassKeys").value.trim()
+
+  document.getElementById("popupRemove").disabled =
+    not (originalRule and pattern == originalPattern)
+
+  if originalRule and pattern == originalPattern and passKeys == originalPassKeys
+    document.getElementById("popupExclude").disabled = true
+    document.getElementById("popupExclude").value = "Update Rule"
+
+  else if originalRule and pattern == originalPattern
+    document.getElementById("popupExclude").disabled = false
+    document.getElementById("popupExclude").value = "Update Rule"
+
+  else if originalRule
+    document.getElementById("popupExclude").disabled = false
+    document.getElementById("popupExclude").value = "Add Rule"
+
+  else if pattern
+    document.getElementById("popupExclude").disabled = false
+    document.getElementById("popupExclude").value = "Add Rule"
 
-onExcludeUrl = (e) ->
-  url = document.getElementById("popupInput").value
-  chrome.extension.getBackgroundPage().addExcludedUrl url
-  document.getElementById("excludeConfirm").setAttribute "style", "display: inline-block"
+  else
+    document.getElementById("popupExclude").disabled = true
+    document.getElementById("popupExclude").value = "Add Rule"
+
+showMessage = do ->
+  timer = null
+
+  hideConfirmationMessage = ->
+    document.getElementById("confirmationMessage").setAttribute "style", "display: none"
+    timer = null
+
+  (message) ->
+    document.getElementById("confirmationMessage").setAttribute "style", "display: inline-block"
+    document.getElementById("confirmationMessage").innerHTML = message
+    clearTimeout(timer) if timer
+    timer = setTimeout(hideConfirmationMessage,2000)
+
+addExclusionRule = ->
+  pattern = document.getElementById("popupPattern").value.trim()
+  passKeys = document.getElementById("popupPassKeys").value.trim()
+  chrome.extension.getBackgroundPage().addExclusionRule pattern, passKeys
+  showMessage("Updated.")
+  onLoad()
+
+removeExclusionRule = ->
+  pattern = document.getElementById("popupPattern").value.trim()
+  chrome.extension.getBackgroundPage().removeExclusionRule pattern
+  showMessage("Removed.")
+  onLoad()
 
 document.addEventListener "DOMContentLoaded", ->
-  document.getElementById("popupButton").addEventListener "click", onExcludeUrl, false
+  document.getElementById("popupExclude").addEventListener "click", addExclusionRule, false
+  document.getElementById("popupRemove").addEventListener "click", removeExclusionRule, false
+  for field in ["popupPattern", "popupPassKeys"]
+    for event in ["keyup", "change"]
+      document.getElementById(field).addEventListener event, onChange, false
   onLoad()
diff --git a/pages/popup.html b/pages/popup.html
index 89f1f02a..86982eae 100644
--- a/pages/popup.html
+++ b/pages/popup.html
@@ -6,17 +6,22 @@
         padding: 0px;
       }
 
-      #vimiumPopup { width: 500px; }
+      #vimiumPopup { width: 400px; }
 
       #excludeControls {
         padding: 10px;
       }
 
-      #popupInput {
+      #popupPattern, #popupPassKeys {
+        margin: 5px;
         width: 330px;
+        /* Match the corresponding font and font size used on the options page. */
+        /* TODO (smblott): Match other styles from the options page. */
+        font-family: Consolas, "Liberation Mono", Courier, monospace;
+        font-size: 14px;
       }
 
-      #excludeConfirm {
+      #confirmationMessage {
         display: inline-block;
         width: 18px;
         height: 13px;
@@ -24,7 +29,8 @@
         display: none;
       }
 
-      #popupButton { margin-left: 10px; }
+      #popupRemove { margin: 5px; }
+      #popupExclude { margin: 5px; }
 
       #popupMenu ul {
         list-style: none;
@@ -52,9 +58,11 @@
   <body>
     <div id="vimiumPopup">
       <div id="excludeControls">
-        <input id="popupInput" type="text" />
-        <input id="popupButton" type="button" value="Exclude URL" />
-        <span id="excludeConfirm">Saved.</span>
+        <input id="popupPattern" placeholder="Pattern against which to match URLs..." type="text" /><br/>
+        <input id="popupPassKeys" placeholder="Only exclude these keys..." type="text" /><br/>
+        <input id="popupRemove" type="button" value="Remove Rule" />
+        <input id="popupExclude" type="button" value="Add or Update Rule" />
+        <span id="confirmationMessage">Text is added in popup.coffee.</span>
       </div>
 
       <div id="popupMenu">
diff --git a/tests/unit_tests/exclusion_test.coffee b/tests/unit_tests/exclusion_test.coffee
index 33dbccd3..7fb63df9 100644
--- a/tests/unit_tests/exclusion_test.coffee
+++ b/tests/unit_tests/exclusion_test.coffee
@@ -15,27 +15,28 @@ root.Marks =
 extend(global, require "../../lib/utils.js")
 Utils.getCurrentVersion = -> '1.44'
 extend(global,require "../../background_scripts/sync.js")
+extend(global, require "../../lib/exclusion_rule.js")
 extend(global,require "../../background_scripts/settings.js")
 Sync.init()
+extend(global, require "../../background_scripts/exclusions.js")
 extend(global, require "../../background_scripts/commands.js")
 extend(global, require "../../background_scripts/main.js")
 
 # These tests cover only the most basic aspects of excluded URLs and passKeys.
 #
 context "Excluded URLs and pass keys",
-  setup ->
-    Settings.set 'excludedUrls', 'http://mail.google.com/*\nhttp://www.facebook.com/* jk'
+
+  # These tests have no setup, they use the default values from settings.coffee.
 
   should "be disabled for excluded sites", ->
-    rule = isEnabledForUrl({ url: 'http://mail.google.com/u/0/inbox' })
+    rule = isEnabledForUrl({ url: 'http://www.google.com/calendar/page' })
     assert.isFalse rule.isEnableForUrl
-    assert.isTrue rule.matchingUrl
+    assert.isFalse rule.passKeys
 
   should "be enabled, but with pass keys", ->
-    rule = isEnabledForUrl({ url: 'http://www.facebook.com/pages' })
+    rule = isEnabledForUrl({ url: 'https://mail.google.com/mail/u/0/#inbox' })
     assert.isTrue rule.isEnabledForUrl
     assert.equal rule.passKeys, 'jk'
-    assert.isTrue rule.matchingUrl
 
   should "be enabled", ->
     rule = isEnabledForUrl({ url: 'http://www.twitter.com/pages' })
@@ -45,27 +46,25 @@ context "Excluded URLs and pass keys",
   should "add a new excluded URL", ->
     rule = isEnabledForUrl({ url: 'http://www.example.com/page' })
     assert.isTrue rule.isEnabledForUrl
-    addExcludedUrl("http://www.example.com*")
+    addExclusionRule("http://www.example.com*")
     rule = isEnabledForUrl({ url: 'http://www.example.com/page' })
     assert.isFalse rule.isEnabledForUrl
     assert.isFalse rule.passKeys
-    assert.isTrue rule.matchingUrl
 
   should "add a new excluded URL with passkeys", ->
-    rule = isEnabledForUrl({ url: 'http://www.example.com/page' })
+    rule = isEnabledForUrl({ url: 'http://www.anotherexample.com/page' })
     assert.isTrue rule.isEnabledForUrl
-    addExcludedUrl("http://www.example.com/* jk")
-    rule = isEnabledForUrl({ url: 'http://www.example.com/page' })
+    addExclusionRule("http://www.anotherexample.com/*","jk")
+    rule = isEnabledForUrl({ url: 'http://www.anotherexample.com/page' })
     assert.isTrue rule.isEnabledForUrl
     assert.equal rule.passKeys, 'jk'
-    assert.isTrue rule.matchingUrl
 
   should "update an existing excluded URL with passkeys", ->
-    rule = isEnabledForUrl({ url: 'http://www.facebook.com/page' })
+    rule = isEnabledForUrl({ url: 'http://mail.google.com/page' })
     assert.isTrue rule.isEnabledForUrl
-    addExcludedUrl("http://www.facebook.com/* jknp")
-    rule = isEnabledForUrl({ url: 'http://www.facebook.com/page' })
+    assert.equal rule.passKeys, 'jk'
+    addExclusionRule("http*://mail.google.com/*","jknp")
+    rule = isEnabledForUrl({ url: 'http://mail.google.com/page' })
     assert.isTrue rule.isEnabledForUrl
     assert.equal rule.passKeys, 'jknp'
-    assert.isTrue rule.matchingUrl
 
-- 
cgit v1.2.3


From 75846e61e2a81da35006787e7488b847bbe2a6ad Mon Sep 17 00:00:00 2001
From: Stephen Blott
Date: Tue, 2 Sep 2014 09:20:22 +0100
Subject: Fix regression: reinstate confirmation dialog for restoreToDefaults

---
 pages/options.coffee | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/pages/options.coffee b/pages/options.coffee
index be0eccfb..cb6c1184 100644
--- a/pages/options.coffee
+++ b/pages/options.coffee
@@ -91,6 +91,8 @@ populateOptions = ->
     selfHandlingCallbacks[field] = selfHandlingFields[field]($(field),enableSaveButton)
 
 restoreToDefaults = ->
+  return unless confirm "Are you sure you want to return Vimium's settings to their defaults?"
+
   for field in editableFields
     val = bgSettings.defaults[field] or ""
     setFieldValue $(field), val
-- 
cgit v1.2.3


From f2596ae0eaaa09a1ec5d641f048d7472f19c812b Mon Sep 17 00:00:00 2001
From: Stephen Blott
Date: Tue, 2 Sep 2014 10:44:42 +0100
Subject: Retain excludedUrls setting, so testers can revert to previous
 versions.

---
 background_scripts/exclusions.coffee | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/background_scripts/exclusions.coffee b/background_scripts/exclusions.coffee
index 326989bf..8242c81b 100644
--- a/background_scripts/exclusions.coffee
+++ b/background_scripts/exclusions.coffee
@@ -74,4 +74,6 @@ if not Settings.has("exclusionRules") and Settings.has("excludedUrls")
   Exclusions.setRules(parseLegacyRules(Settings.get("excludedUrls")))
   # We'll keep a backup of the excludedUrls setting, just in case (and for testing).
   Settings.set("excludedUrlsBackup",Settings.get("excludedUrls")) if not Settings.has("excludedUrlsBackup")
-  Settings.clear("excludedUrls")
+  # TODO (smblott): Uncomment the following line.  It's commented for now so that anyone trying out this code
+  # can revert to previous versions.
+  # Settings.clear("excludedUrls")
-- 
cgit v1.2.3


From 76150e7b982b5cc6b68ad0dcf36c70382fdea30d Mon Sep 17 00:00:00 2001
From: Stephen Blott
Date: Tue, 2 Sep 2014 13:47:14 +0100
Subject: Structured exclusion rules: Fix typos and minor issues.

---
 background_scripts/main.coffee         | 6 +++---
 content_scripts/vimium_frontend.coffee | 4 ++--
 manifest.json                          | 1 -
 pages/options.coffee                   | 1 +
 pages/options.html                     | 5 -----
 pages/popup.coffee                     | 3 +--
 6 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/background_scripts/main.coffee b/background_scripts/main.coffee
index 4111ac06..47b215e5 100644
--- a/background_scripts/main.coffee
+++ b/background_scripts/main.coffee
@@ -74,9 +74,9 @@ getCurrentTabUrl = (request, sender) -> sender.tab.url
 #
 root.isEnabledForUrl = isEnabledForUrl = (request) ->
   rule = Exclusions.getRule(request.url)
-  return { rule: rule, isEnabledForUrl: true,  passKeys: rule.passKeys } if rule and rule.passKeys
-  return { rule: rule, isEnabledForUrl: false, passKeys: "" } if rule
-  return { rule: rule, isEnabledForUrl: true,  passKeys: "" }
+  newIsEnabled = !rule or rule.passKeys
+  newPassKeys = if newIsEnabled and rule then rule.passKeys else ""
+  { rule: rule, isEnabledForUrl: newIsEnabled, passKeys: newPassKeys }
 
 # Called by the popup UI.  If an existing exclusion rule has been changed, then the existing rule is updated.
 # Otherwise, the new rule is added.
diff --git a/content_scripts/vimium_frontend.coffee b/content_scripts/vimium_frontend.coffee
index e2ffa7f0..6db0d830 100644
--- a/content_scripts/vimium_frontend.coffee
+++ b/content_scripts/vimium_frontend.coffee
@@ -331,11 +331,11 @@ extend window,
 
       false
 
-# Decide whether this keyChar be passed to the underlying page.
+# Decide whether this keyChar should be passed to the underlying page.
 # Keystrokes are *never* considered passKeys if the keyQueue is not empty.  So, for example, if 't' is a
 # passKey, then 'gt' and '99t' will neverthless be handled by vimium.
 isPassKey = ( keyChar ) ->
-  return !keyQueue and 0 <= passKeys.indexOf(keyChar)
+  return !keyQueue and passKeys and 0 <= passKeys.indexOf(keyChar)
 
 handledKeydownEvents = []
 
diff --git a/manifest.json b/manifest.json
index 3f4c877d..d376dca4 100644
--- a/manifest.json
+++ b/manifest.json
@@ -37,7 +37,6 @@
              "lib/dom_utils.js",
              "lib/handler_stack.js",
              "lib/clipboard.js",
-             "lib/exclusion_rule.js",
              "content_scripts/link_hints.js",
              "content_scripts/vomnibar.js",
              "content_scripts/scroller.js",
diff --git a/pages/options.coffee b/pages/options.coffee
index cb6c1184..1b7faea0 100644
--- a/pages/options.coffee
+++ b/pages/options.coffee
@@ -89,6 +89,7 @@ populateOptions = ->
   # Self-handling options build their own DOM, and provide callbacks for saveOptions and restoreToDefaults.
   for field of selfHandlingFields
     selfHandlingCallbacks[field] = selfHandlingFields[field]($(field),enableSaveButton)
+  onDataLoaded()
 
 restoreToDefaults = ->
   return unless confirm "Are you sure you want to return Vimium's settings to their defaults?"
diff --git a/pages/options.html b/pages/options.html
index c9fc5a63..b0ae8fd5 100644
--- a/pages/options.html
+++ b/pages/options.html
@@ -183,11 +183,6 @@
         border-radius: 2px;
         color: #444;
       }
-      .exclusionRemoveButton {
-        /* cursor: pointer; */
-        /* border: none; */
-        /* background: none; */
-      }
       input.pattern, input.passKeys {
         font-family: Consolas, "Liberation Mono", Courier, monospace;
         font-size: 14px;
diff --git a/pages/popup.coffee b/pages/popup.coffee
index ff943f32..2f3cee2a 100644
--- a/pages/popup.coffee
+++ b/pages/popup.coffee
@@ -7,13 +7,12 @@ onLoad = ->
   document.getElementById("optionsLink").setAttribute "href", chrome.runtime.getURL("pages/options.html")
   chrome.tabs.getSelected null, (tab) ->
     isEnabled = chrome.extension.getBackgroundPage().isEnabledForUrl(url: tab.url)
+    # Check if we have an existing exclusing rule for this page.
     if isEnabled.rule
-      # There is an existing exclusion rule for this page.
       originalRule = isEnabled.rule
       originalPattern = originalRule.pattern
       originalPassKeys = originalRule.passKeys
     else
-      # There is not an existing exclusion rule.
       # The common use case is to disable Vimium at the domain level.
       # This regexp will match "http://www.example.com/" from "http://www.example.com/path/to/page.html".
       domain = (tab.url.match(/[^\/]*\/\/[^\/]*\//) or tab.url) + "*"
-- 
cgit v1.2.3


From 2f27d4590ba30f5a443aedff12d9611a83a4f771 Mon Sep 17 00:00:00 2001
From: Stephen Blott
Date: Tue, 2 Sep 2014 16:46:17 +0100
Subject: Structured exclusion rules: Simplify isEnabledForUrl.

---
 background_scripts/main.coffee | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/background_scripts/main.coffee b/background_scripts/main.coffee
index 47b215e5..4e6f406e 100644
--- a/background_scripts/main.coffee
+++ b/background_scripts/main.coffee
@@ -74,9 +74,11 @@ getCurrentTabUrl = (request, sender) -> sender.tab.url
 #
 root.isEnabledForUrl = isEnabledForUrl = (request) ->
   rule = Exclusions.getRule(request.url)
-  newIsEnabled = !rule or rule.passKeys
-  newPassKeys = if newIsEnabled and rule then rule.passKeys else ""
-  { rule: rule, isEnabledForUrl: newIsEnabled, passKeys: newPassKeys }
+  {
+    rule: rule
+    isEnabledForUrl: not rule or rule.passKeys
+    passKeys: rule?.passKeys or ""
+  }
 
 # Called by the popup UI.  If an existing exclusion rule has been changed, then the existing rule is updated.
 # Otherwise, the new rule is added.
-- 
cgit v1.2.3


From 643e49aa3109b9a25b38ce5d6b59bb11bc6337b1 Mon Sep 17 00:00:00 2001
From: Stephen Blott
Date: Fri, 5 Sep 2014 10:59:20 +0100
Subject: Structured passkeys; changes following code review; major rewrite of
 options.

---
 background_scripts/exclusions.coffee   |  47 +++---
 background_scripts/main.coffee         |   4 +-
 background_scripts/settings.coffee     |   6 +-
 lib/exclusion_rule.coffee              |  46 ------
 manifest.json                          |   1 -
 pages/options.coffee                   | 280 +++++++++++++++++++++------------
 pages/options.html                     |  25 ++-
 pages/popup.coffee                     |  23 +--
 tests/unit_tests/exclusion_test.coffee |  13 +-
 9 files changed, 244 insertions(+), 201 deletions(-)
 delete mode 100644 lib/exclusion_rule.coffee

diff --git a/background_scripts/exclusions.coffee b/background_scripts/exclusions.coffee
index 8242c81b..3a8ef1e7 100644
--- a/background_scripts/exclusions.coffee
+++ b/background_scripts/exclusions.coffee
@@ -23,8 +23,9 @@ root.Exclusions = Exclusions =
     return null
 
   setRules: (rules) ->
+    # Callers map a rule to null to have it deleted, and rules without a pattern are useless.
     @rules = rules.filter (rule) -> rule and rule.pattern
-    Settings.set("exclusionRules",@rules)
+    Settings.set("exclusionRules", @rules)
 
   postUpdateHook: (rules) ->
     @rules = rules
@@ -33,37 +34,29 @@ root.Exclusions = Exclusions =
   updateOrAdd: (newRule) ->
     seen = false
     @rules.push(newRule)
-    @setRules(@rules.map (rule) -> if rule.pattern == newRule.pattern then (if seen then null else seen = newRule) else rule)
+    @setRules @rules.map (rule) ->
+      if rule.pattern == newRule.pattern
+        if seen then null else seen = newRule
+      else
+        rule
 
   remove: (pattern) ->
-    @setRules(@rules.filter((rule) -> rule.pattern != pattern))
-
-  # DOM handling for the options page; populate the exclusionRules option.
-  populateOption: (exclusionRulesElement,enableSaveButton) ->
-    populate = =>
-      while exclusionRulesElement.firstChild
-        exclusionRulesElement.removeChild(exclusionRulesElement.firstChild)
-      for rule in @rules
-        exclusionRulesElement.appendChild(ExclusionRule.buildRuleElement(rule,enableSaveButton))
-      exclusionRulesElement.appendChild(ExclusionRule.buildRuleElement({pattern: "", passKeys: ""},enableSaveButton))
-    populate()
-    return {
-      saveOption: =>
-        @setRules(ExclusionRule.extractRule(element) for element in exclusionRulesElement.getElementsByClassName('exclusionRow'))
-        populate()
-      restoreToDefault: =>
-        Settings.clear("exclusionRules")
-        populate()
-    }
+    @setRules(@rules.filter((rule) -> rule and rule.pattern != pattern))
 
 # Development and debug only.
 # Enable this (temporarily) to restore legacy exclusion rules from backup.
 if false and Settings.has("excludedUrlsBackup")
   Settings.clear("exclusionRules")
-  Settings.set("excludedUrls",Settings.get("excludedUrlsBackup"))
+  Settings.set("excludedUrls", Settings.get("excludedUrlsBackup"))
 
 if not Settings.has("exclusionRules") and Settings.has("excludedUrls")
-  # Migration from the legacy exclusion rules (settings: "excludedUrls" -> "exclusionRules").
+  # Migration from the legacy representation of exclusion rules.
+  #
+  # In Vimium 1.45 and in github/master on 27 August, 2014, exclusion rules are represented by the setting:
+  #   excludedUrls: "http*://www.google.com/reader/*\nhttp*://mail.google.com/* jk"
+  #
+  # The new (equivalent) settings is:
+  #   exclusionRules: [ { pattern: "http*://www.google.com/reader/*", passKeys: "" }, { pattern: "http*://mail.google.com/*", passKeys: "jk" } ]
 
   parseLegacyRules = (lines) ->
     for line in lines.trim().split("\n").map((line) -> line.trim())
@@ -72,8 +65,6 @@ if not Settings.has("exclusionRules") and Settings.has("excludedUrls")
         { pattern: parse[0], passKeys: parse[1..].join("") }
 
   Exclusions.setRules(parseLegacyRules(Settings.get("excludedUrls")))
-  # We'll keep a backup of the excludedUrls setting, just in case (and for testing).
-  Settings.set("excludedUrlsBackup",Settings.get("excludedUrls")) if not Settings.has("excludedUrlsBackup")
-  # TODO (smblott): Uncomment the following line.  It's commented for now so that anyone trying out this code
-  # can revert to previous versions.
-  # Settings.clear("excludedUrls")
+  # We'll keep a backup of the "excludedUrls" setting, just in case.
+  Settings.set("excludedUrlsBackup", Settings.get("excludedUrls")) if not Settings.has("excludedUrlsBackup")
+  Settings.clear("excludedUrls")
diff --git a/background_scripts/main.coffee b/background_scripts/main.coffee
index 4e6f406e..352cfa48 100644
--- a/background_scripts/main.coffee
+++ b/background_scripts/main.coffee
@@ -80,8 +80,8 @@ root.isEnabledForUrl = isEnabledForUrl = (request) ->
     passKeys: rule?.passKeys or ""
   }
 
-# Called by the popup UI.  If an existing exclusion rule has been changed, then the existing rule is updated.
-# Otherwise, the new rule is added.
+# Called by the popup UI.
+# If the URL pattern matches an existing rule, then the existing rule is updated. Otherwise, a new rule is created.
 root.addExclusionRule = (pattern,passKeys) ->
   if pattern = pattern.trim()
     Exclusions.updateOrAdd({ pattern: pattern, passKeys: passKeys })
diff --git a/background_scripts/settings.coffee b/background_scripts/settings.coffee
index 63dd851b..7150fcba 100644
--- a/background_scripts/settings.coffee
+++ b/background_scripts/settings.coffee
@@ -61,6 +61,7 @@ root.Settings = Settings =
   # or strings
   defaults:
     scrollStepSize: 60
+    keyMappings: "# Insert your prefered key mappings here."
     linkHintCharacters: "sadfjklewcmpgh"
     linkHintNumbers: "0123456789"
     filterLinkHints: false
@@ -87,9 +88,8 @@ root.Settings = Settings =
     # Default exclusion rules.
     exclusionRules:
       [
-        # Disable Vimium on Google Reader, and use Gmail's own j/k bindings.
-        { pattern: "http*://www.google.com/reader/*", passKeys: "" }
-        { pattern: "http*://mail.google.com/*", passKeys: "jk" }
+        # Disable Vimium on Gmail.
+        { pattern: "http*://mail.google.com/*", passKeys: "" }
       ]
 
     # NOTE: If a page contains both a single angle-bracket link and a double angle-bracket link, then in
diff --git a/lib/exclusion_rule.coffee b/lib/exclusion_rule.coffee
deleted file mode 100644
index 0942e7cf..00000000
--- a/lib/exclusion_rule.coffee
+++ /dev/null
@@ -1,46 +0,0 @@
-root = exports ? window
-
-# Operations to build the DOM on the options page for a single exclusion rule.
-
-root.ExclusionRule =
-
-  # Build a DOM table row (a "tr") for this rule.
-  buildRuleElement: (rule,enableSaveButton) ->
-    pattern = @buildInput(enableSaveButton,rule.pattern,"URL pattern","pattern")
-    passKeys = @buildInput(enableSaveButton,rule.passKeys,"Excluded keys","passKeys")
-    row = document.createElement("tr")
-    row.className = "exclusionRow"
-    remove = document.createElement("input")
-    remove.type = "button"
-    remove.value = "\u2716" # A cross.
-    remove.className = "exclusionRemoveButton"
-    remove.addEventListener "click", ->
-      row.parentNode.removeChild(row)
-      enableSaveButton()
-    row.appendChild(pattern)
-    row.appendChild(passKeys)
-    row.appendChild(remove)
-    # NOTE: Since the order of exclusions matters, it would be nice to have "Move Up" and "Move Down" buttons,
-    # too.  But this option is pretty cluttered already.
-    row
-
-  # Build DOM (a "td" containing an "input") for a single input element.
-  buildInput: (enableSaveButton,value,placeholder,cls) ->
-    input = document.createElement("input")
-    input.setAttribute("placeholder",placeholder)
-    input.type = "text"
-    input.value = value
-    input.className = cls
-    input.addEventListener "keyup", enableSaveButton, false
-    input.addEventListener "change", enableSaveButton, false
-    container = document.createElement("td")
-    container.appendChild(input)
-    container
-
-  # Build a new exclusion rule from the given element.  This is the reverse of the two methods above.
-  extractRule: (element) ->
-    patternElement = element.firstChild
-    passKeysElement = patternElement.nextSibling
-    pattern = patternElement.firstChild.value.trim()
-    passKeys = passKeysElement.firstChild.value.trim()
-    if pattern then { pattern: pattern, passKeys: passKeys } else null
diff --git a/manifest.json b/manifest.json
index d376dca4..48681675 100644
--- a/manifest.json
+++ b/manifest.json
@@ -12,7 +12,6 @@
       "background_scripts/commands.js",
       "lib/clipboard.js",
       "background_scripts/sync.js",
-      "lib/exclusion_rule.js",
       "background_scripts/settings.js",
       "background_scripts/exclusions.js",
       "background_scripts/completion.js",
diff --git a/pages/options.coffee b/pages/options.coffee
index 1b7faea0..8bc3039e 100644
--- a/pages/options.coffee
+++ b/pages/options.coffee
@@ -1,42 +1,148 @@
-$ = (id) -> document.getElementById id
 
+$ = (id) -> document.getElementById id
 bgSettings = chrome.extension.getBackgroundPage().Settings
 
-editableFields = [ "scrollStepSize", "linkHintCharacters", "linkHintNumbers",
-  "userDefinedLinkHintCss", "keyMappings", "filterLinkHints", "previousPatterns",
-  "nextPatterns", "hideHud", "regexFindMode", "searchUrl", "searchEngines"]
-
-canBeEmptyFields = ["keyMappings", "userDefinedLinkHintCss", "searchEngines"]
-
-# Settings which handle their own DOM and callbacks for the options page.
-# See populateOption in ../background_scripts/exclusions.coffee for an example.
-selfHandlingFields =
-  exclusionRules: (args...) -> chrome.extension.getBackgroundPage().Exclusions.populateOption(args...)
-selfHandlingCallbacks = {}
-
-document.addEventListener "DOMContentLoaded", ->
-  populateOptions()
-
-  for field in editableFields
-    $(field).addEventListener "keyup", onOptionKeyup, false
-    $(field).addEventListener "change", enableSaveButton, false
-    $(field).addEventListener "change", onDataLoaded, false
-
-  $("advancedOptionsLink").addEventListener "click", toggleAdvancedOptions, false
-  $("showCommands").addEventListener "click", (->
-    showHelpDialog chrome.extension.getBackgroundPage().helpDialogHtml(true, true, "Command Listing"), frameId
-  ), false
-  document.getElementById("restoreSettings").addEventListener "click", restoreToDefaults
-  document.getElementById("saveOptions").addEventListener "click", saveOptions
+#
+# Class hierarchy for various types of option.
+class Option
+  # Base class for all option classes.
+  # Abstract. Option does not define @populateElement or @readValueFromElement.
+
+  # Static. Array of all options.
+  @all = []
+
+  constructor: (field,enableSaveButton) ->
+    @field = field
+    @element = $(@field)
+    @element.addEventListener "change", enableSaveButton
+    @fetch()
+    Option.all.push @
+
+  # Fetch a setting from localStorage, remember the @previous value and populate the DOM element.
+  # Return the fetched value.
+  fetch: ->
+    @populateElement @previous = bgSettings.get @field
+    @previous
+
+  # Write this option's new value back to localStorage, if necessary.
+  save: ->
+    value = @readValueFromElement()
+    if not @areEqual value, @previous
+      bgSettings.set @field, @previous = value
+      bgSettings.performPostUpdateHook @field, value
+
+  # Compare values; this is overridden by sub-classes.
+  areEqual: (a,b) -> a == b
+
+  restoreToDefault: ->
+    bgSettings.clear @field
+    @fetch()
+
+  # Abstract method; only implemented in sub-classes.
+  # Populate the option's DOM element (@element) with the setting's current value.
+  # populateElement: (value) -> DO_SOMETHING
+
+  # Abstract method; only implemented in sub-classes.
+  # Extract the setting's new value from the option's DOM element (@element).
+  # readValueFromElement: -> RETURN_SOMETHING
+
+class NumberOption extends Option
+  populateElement: (value) -> @element.value = value
+  readValueFromElement: -> parseFloat @element.value
+
+class TextOption extends Option
+  populateElement: (value) -> @element.value = value
+  readValueFromElement: -> @element.value.trim()
+
+class NonEmptyTextOption extends Option
+  populateElement: (value) -> @element.value = value
+  # If the new value is not empty, then return it. Otherwise, restore the default value.
+  readValueFromElement: -> if value = @element.value.trim() then value else @restoreToDefault()
+
+class CheckBoxOption extends Option
+  populateElement: (value) -> @element.checked = value
+  readValueFromElement: -> @element.checked
+
+class ExclusionRulesOption extends Option
+  constructor: (args...) ->
+    super(args...)
+    $("exclusionAddButton").addEventListener "click", (event) =>
+      @appendRule { pattern: "", passKeys: "" }
+      @maintainExclusionMargin()
+      # Focus the pattern element in the new rule.
+      @element.children[@element.children.length-1].children[0].children[0].focus()
+      # Scroll the new rule into view.
+      exclusionScrollBox = $("exclusionScrollBox")
+      exclusionScrollBox.scrollTop = exclusionScrollBox.scrollHeight
+
+  populateElement: (rules) ->
+    while @element.firstChild
+      @element.removeChild @element.firstChild
+    for rule in rules
+      @appendRule rule
+    @maintainExclusionMargin()
+
+  # Append a row for a new rule.
+  appendRule: (rule) ->
+    content = document.querySelector('#exclusionRuleTemplate').content
+    row = document.importNode content, true
+
+    for field in ["pattern", "passKeys"]
+      element = row.querySelector ".#{field}"
+      element.value = rule[field]
+      for event in [ "keyup", "change" ]
+        element.addEventListener event, enableSaveButton
+
+    remove = row.querySelector ".exclusionRemoveButton"
+    remove.addEventListener "click", (event) =>
+      row = event.target.parentNode.parentNode
+      row.parentNode.removeChild row
+      enableSaveButton()
+      @maintainExclusionMargin()
+
+    @element.appendChild row
+
+  readValueFromElement: ->
+    rules =
+      for element in @element.children
+        pattern = element.children[0].firstChild.value.trim()
+        passKeys = element.children[1].firstChild.value.trim()
+        { pattern: pattern, passKeys: passKeys }
+    rules.filter (rule) -> rule.pattern
+
+  areEqual: (a,b) ->
+    # Flatten each list of rules to a newline-separated string representation, and then use string equality.
+    # This is correct because patterns and passKeys cannot themselves contain newlines.
+    flatten = (rule) -> if rule and rule.pattern then rule.pattern + "\n" + rule.passKeys else ""
+    a.map(flatten).join("\n") == b.map(flatten).join("\n")
+
+  # Hack.  There has to be a better way than...
+  # The y-axis scrollbar for "exclusionRules" is only displayed if it is needed.  When visible, it appears on
+  # top of the enclosed content (partially obscuring it).  Here, we adjust the margin of the "Remove" button to
+  # compensate.
+  maintainExclusionMargin: ->
+    scrollBox = $("exclusionScrollBox")
+    margin = if scrollBox.clientHeight < scrollBox.scrollHeight then "16px" else "0px"
+    for element in scrollBox.getElementsByClassName "exclusionRemoveButton"
+      element.style["margin-right"] = margin
+
+#
+# Operations for page elements.
+enableSaveButton = ->
+  $("saveOptions").removeAttribute "disabled"
 
-window.onbeforeunload = -> "You have unsaved changes to options." unless $("saveOptions").disabled
+saveOptions = ->
+  Option.all.map (option) -> option.save()
+  $("saveOptions").disabled = true
 
-onOptionKeyup = (event) ->
-  if (event.target.getAttribute("type") isnt "checkbox" and
-      event.target.getAttribute("savedValue") isnt event.target.value)
-    enableSaveButton()
+restoreToDefaults = ->
+  return unless confirm "Are you sure you want to return Vimium's settings to their defaults?"
+  Option.all.map (option) -> option.restoreToDefault()
+  maintainLinkHintsView()
+  $("saveOptions").disabled = true
 
-onDataLoaded = ->
+# Display either "linkHintNumbers" or "linkHintCharacters", depending upon "filterLinkHints".
+maintainLinkHintsView = ->
   hide = (el) -> el.parentNode.parentNode.style.display = "none"
   show = (el) -> el.parentNode.parentNode.style.display = "table-row"
   if $("filterLinkHints").checked
@@ -46,76 +152,48 @@ onDataLoaded = ->
     show $("linkHintCharacters")
     hide $("linkHintNumbers")
 
-enableSaveButton = ->
-  $("saveOptions").removeAttribute "disabled"
-
-# Saves options to localStorage.
-saveOptions = ->
-
-  # If the value is unchanged from the default, delete the preference from localStorage; this gives us
-  # the freedom to change the defaults in the future.
-  for fieldName in editableFields
-    field = $(fieldName)
-    switch field.getAttribute("type")
-      when "checkbox"
-        fieldValue = field.checked
-      when "number"
-        fieldValue = parseFloat field.value
+toggleAdvancedOptions =
+  do (advancedMode=false) ->
+    (event) ->
+      if advancedMode
+        $("advancedOptions").style.display = "none"
+        $("advancedOptionsLink").innerHTML = "Show advanced options&hellip;"
       else
-        fieldValue = field.value.trim()
-        field.value = fieldValue
-
-    # If it's empty and not a field that we allow to be empty, restore to the default value
-    if not fieldValue and canBeEmptyFields.indexOf(fieldName) is -1
-      bgSettings.clear fieldName
-      fieldValue = bgSettings.get(fieldName)
-    else
-      bgSettings.set fieldName, fieldValue
-    $(fieldName).value = fieldValue
-    $(fieldName).setAttribute "savedValue", fieldValue
-    bgSettings.performPostUpdateHook fieldName, fieldValue
- 
-  # Self-handling options save themselves.
-  for field of selfHandlingFields
-    selfHandlingCallbacks[field].saveOption() if selfHandlingCallbacks[field].saveOption
+        $("advancedOptions").style.display = "table-row-group"
+        $("advancedOptionsLink").innerHTML = "Hide advanced options"
+      advancedMode = !advancedMode
+      event.preventDefault()
 
-  $("saveOptions").disabled = true
+activateHelpDialog = ->
+  showHelpDialog chrome.extension.getBackgroundPage().helpDialogHtml(true, true, "Command Listing"), frameId
 
-# Restores select box state to saved value from localStorage.
-populateOptions = ->
-  for field in editableFields
-    val = bgSettings.get(field) or ""
-    setFieldValue $(field), val
-  # Self-handling options build their own DOM, and provide callbacks for saveOptions and restoreToDefaults.
-  for field of selfHandlingFields
-    selfHandlingCallbacks[field] = selfHandlingFields[field]($(field),enableSaveButton)
-  onDataLoaded()
-
-restoreToDefaults = ->
-  return unless confirm "Are you sure you want to return Vimium's settings to their defaults?"
+#
+# Initialization.
+document.addEventListener "DOMContentLoaded", ->
 
-  for field in editableFields
-    val = bgSettings.defaults[field] or ""
-    setFieldValue $(field), val
-  # Self-handling options restore their own defaults.
-  for field of selfHandlingFields
-    selfHandlingCallbacks[field].restoreToDefault() if selfHandlingCallbacks[field].restoreToDefault
-  onDataLoaded()
-  enableSaveButton()
-
-setFieldValue = (field, value) ->
-  unless field.getAttribute("type") is "checkbox"
-    field.value = value
-    field.setAttribute "savedValue", value
-  else
-    field.checked = value
+  # Populate options.  The constructor adds each new object to "Option.all".
+  new type(name,enableSaveButton) for name, type of {
+    exclusionRules: ExclusionRulesOption
+    filterLinkHints: CheckBoxOption
+    hideHud: CheckBoxOption
+    keyMappings: TextOption
+    linkHintCharacters: NonEmptyTextOption
+    linkHintNumbers: NonEmptyTextOption
+    nextPatterns: NonEmptyTextOption
+    previousPatterns: NonEmptyTextOption
+    regexFindMode: CheckBoxOption
+    scrollStepSize: NumberOption
+    searchEngines: TextOption
+    searchUrl: NonEmptyTextOption
+    userDefinedLinkHintCss: TextOption
+  }
+
+  $("saveOptions").addEventListener "click", saveOptions
+  $("restoreSettings").addEventListener "click", restoreToDefaults
+  $("advancedOptionsLink").addEventListener "click", toggleAdvancedOptions
+  $("showCommands").addEventListener "click", activateHelpDialog
+  $("filterLinkHints").addEventListener "click", maintainLinkHintsView
+
+  maintainLinkHintsView()
+  window.onbeforeunload = -> "You have unsaved changes to options." unless $("saveOptions").disabled
 
-toggleAdvancedOptions = do (advancedMode=false) -> (event) ->
-  if advancedMode
-    $("advancedOptions").style.display = "none"
-    $("advancedOptionsLink").innerHTML = "Show advanced options&hellip;"
-  else
-    $("advancedOptions").style.display = "table-row-group"
-    $("advancedOptionsLink").innerHTML = "Hide advanced options"
-  advancedMode = !advancedMode
-  event.preventDefault()
diff --git a/pages/options.html b/pages/options.html
index b0ae8fd5..fb904316 100644
--- a/pages/options.html
+++ b/pages/options.html
@@ -6,7 +6,6 @@
     <script src="../lib/dom_utils.js"></script>
     <script src="../lib/handler_stack.js"></script>
     <script src="../lib/clipboard.js"></script>
-    <script src="../lib/exclusion_rule.js"></script>
     <script src="../content_scripts/link_hints.js"></script>
     <script src="../content_scripts/vomnibar.js"></script>
     <script src="../content_scripts/scroller.js"></script>
@@ -175,9 +174,10 @@
         border-top: 1px solid #eee;
       }
       /* Ids and classes for rendering exclusionRules */
-      #exclusionScroll {
+      #exclusionScrollBox {
         overflow: scroll;
         overflow-x: hidden;
+        overflow-y: auto;
         height: 225px;
         border: 1px solid #bfbfbf;
         border-radius: 2px;
@@ -193,6 +193,11 @@
       .passKeys {
         width: 120px;
       }
+      #exclusionAddButton {
+        float: right;
+        margin-top: 5px;
+        margin-right: 0px;
+      }
     </style>
   <link rel="stylesheet" type="text/css" href="../content_scripts/vimium.css" />
 
@@ -216,15 +221,25 @@
             <div class="help">
               <div class="example">
                 <p>
-                   The left column contains URL patterns.  Vimium will be wholly or partially disabled for URLs matching these patterns.  Patterns are Javascript regular expressions.  Additionally, the symbox "*" matches any zero or more characters.
+                   The left column contains URL patterns.  Vimium will be wholly or partially disabled for URLs matching these patterns.  Patterns are Javascript regular expressions.  Additionally, the character "*" matches any zero or more characters.
                 </p>
                 <p>
                    The right column contains keys which Vimium would would normally handle, but should instead be passed through to the underlying web page (for pages matching the corresponding pattern).  If empty, then Vimium is wholly disabled.
                 </p>
               </div>
             </div>
-            <div id="exclusionScroll">
-               <table id="exclusionRules"></table>
+            <div>
+               <div id="exclusionScrollBox">
+                  <table id="exclusionRules"></table>
+                  <template id="exclusionRuleTemplate">
+                    <tr>
+                       <td><input/ type="text" class="pattern" placeholder="URL pattern"></td>
+                       <td><input/ type="text" class="passKeys" placeholder="Exclude keys"></td>
+                       <td><input/ type="button" class="exclusionRemoveButton" value="&#x2716;"></td>
+                    </tr>
+                  </template>
+               </div>
+               <button id="exclusionAddButton">Add Rule</button>
             </div>
           </td>
         </tr>
diff --git a/pages/popup.coffee b/pages/popup.coffee
index 2f3cee2a..ecf683e5 100644
--- a/pages/popup.coffee
+++ b/pages/popup.coffee
@@ -24,31 +24,34 @@ onLoad = ->
     onChange()
 
 onChange = ->
+  # As the text in the popup's input elements is changed, update the the popup's buttons accordingly.
+  # Aditionally, enable and disable those buttons as appropriate.
   pattern = document.getElementById("popupPattern").value.trim()
   passKeys = document.getElementById("popupPassKeys").value.trim()
+  popupExclude = document.getElementById("popupExclude")
 
   document.getElementById("popupRemove").disabled =
     not (originalRule and pattern == originalPattern)
 
   if originalRule and pattern == originalPattern and passKeys == originalPassKeys
-    document.getElementById("popupExclude").disabled = true
-    document.getElementById("popupExclude").value = "Update Rule"
+    popupExclude.disabled = true
+    popupExclude.value = "Update Rule"
 
   else if originalRule and pattern == originalPattern
-    document.getElementById("popupExclude").disabled = false
-    document.getElementById("popupExclude").value = "Update Rule"
+    popupExclude.disabled = false
+    popupExclude.value = "Update Rule"
 
   else if originalRule
-    document.getElementById("popupExclude").disabled = false
-    document.getElementById("popupExclude").value = "Add Rule"
+    popupExclude.disabled = false
+    popupExclude.value = "Add Rule"
 
   else if pattern
-    document.getElementById("popupExclude").disabled = false
-    document.getElementById("popupExclude").value = "Add Rule"
+    popupExclude.disabled = false
+    popupExclude.value = "Add Rule"
 
   else
-    document.getElementById("popupExclude").disabled = true
-    document.getElementById("popupExclude").value = "Add Rule"
+    popupExclude.disabled = true
+    popupExclude.value = "Add Rule"
 
 showMessage = do ->
   timer = null
diff --git a/tests/unit_tests/exclusion_test.coffee b/tests/unit_tests/exclusion_test.coffee
index 7fb63df9..a24c3b67 100644
--- a/tests/unit_tests/exclusion_test.coffee
+++ b/tests/unit_tests/exclusion_test.coffee
@@ -15,7 +15,6 @@ root.Marks =
 extend(global, require "../../lib/utils.js")
 Utils.getCurrentVersion = -> '1.44'
 extend(global,require "../../background_scripts/sync.js")
-extend(global, require "../../lib/exclusion_rule.js")
 extend(global,require "../../background_scripts/settings.js")
 Sync.init()
 extend(global, require "../../background_scripts/exclusions.js")
@@ -34,9 +33,13 @@ context "Excluded URLs and pass keys",
     assert.isFalse rule.passKeys
 
   should "be enabled, but with pass keys", ->
-    rule = isEnabledForUrl({ url: 'https://mail.google.com/mail/u/0/#inbox' })
+    rule = isEnabledForUrl({ url: 'https://www.facebook.com/something' })
     assert.isTrue rule.isEnabledForUrl
-    assert.equal rule.passKeys, 'jk'
+    assert.isFalse rule.passKeys
+    addExclusionRule("http*://www.facebook.com/*","oO")
+    rule = isEnabledForUrl({ url: 'https://www.facebook.com/something' })
+    assert.isTrue rule.isEnabledForUrl
+    assert.equal rule.passKeys, 'oO'
 
   should "be enabled", ->
     rule = isEnabledForUrl({ url: 'http://www.twitter.com/pages' })
@@ -61,8 +64,8 @@ context "Excluded URLs and pass keys",
 
   should "update an existing excluded URL with passkeys", ->
     rule = isEnabledForUrl({ url: 'http://mail.google.com/page' })
-    assert.isTrue rule.isEnabledForUrl
-    assert.equal rule.passKeys, 'jk'
+    assert.isFalse rule.isEnabledForUrl
+    assert.isFalse rule.passKeys
     addExclusionRule("http*://mail.google.com/*","jknp")
     rule = isEnabledForUrl({ url: 'http://mail.google.com/page' })
     assert.isTrue rule.isEnabledForUrl
-- 
cgit v1.2.3


From 50ea5ac670e736cebcb9bc1300401fe7aa3835fd Mon Sep 17 00:00:00 2001
From: Stephen Blott
Date: Wed, 22 Oct 2014 06:50:35 +0100
Subject: Clearer reset-to-defaults confirmation message.

---
 pages/options.coffee | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pages/options.coffee b/pages/options.coffee
index 8bc3039e..7f374f5d 100644
--- a/pages/options.coffee
+++ b/pages/options.coffee
@@ -136,7 +136,7 @@ saveOptions = ->
   $("saveOptions").disabled = true
 
 restoreToDefaults = ->
-  return unless confirm "Are you sure you want to return Vimium's settings to their defaults?"
+  return unless confirm "Are you sure you want to permanently return all of Vimium's settings to their defaults?"
   Option.all.map (option) -> option.restoreToDefault()
   maintainLinkHintsView()
   $("saveOptions").disabled = true
-- 
cgit v1.2.3