From 3dc8a03d7271c659139c157bb658795dd21298f9 Mon Sep 17 00:00:00 2001
From: Stephen Blott
Date: Sun, 11 Nov 2012 13:22:18 +0000
Subject: Remove history entries. When a chrome history entry is removed,
 remove that entry from our history too.  Same when the entire history is
 removed.

---
 background_scripts/completion.coffee    | 13 ++++++++++++
 tests/unit_tests/completion_test.coffee | 37 ++++++++++++++++++++++++++++++++-
 2 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/background_scripts/completion.coffee b/background_scripts/completion.coffee
index 25f76c1e..8ab71290 100644
--- a/background_scripts/completion.coffee
+++ b/background_scripts/completion.coffee
@@ -365,6 +365,7 @@ HistoryCache =
       history.sort @compareHistoryByUrl
       @history = history
       chrome.history.onVisited.addListener(@onPageVisited.bind(this))
+      chrome.history.onVisitRemoved.addListener(@onVisitRemoved.bind(this))
       callback(@history) for callback in @callbacks
       @callbacks = null
 
@@ -383,6 +384,18 @@ HistoryCache =
     else
       @history.splice(i, 0, newPage)
 
+  # When a page is removed from the chrome history, remove it from the vimium history too.
+  onVisitRemoved: (toRemove) ->
+    if toRemove.allHistory
+      @history = []
+    else
+      toRemove.urls.map (url) =>
+        i = HistoryCache.binarySearch({url:url}, @history, @compareHistoryByUrl)
+        # TODO (smblott)
+        #      The `i < @history.length` condition below should not be necessary.  It can be removed when `binarySearch` is fixed.
+        if i < @history.length and @history[i].url == url
+          @history.splice(i, 1)
+
 # Returns the matching index or the closest matching index if the element is not found. That means you
 # must check the element at the returned index to know whether the element was actually found.
 # This method is used for quickly searching through our history cache.
diff --git a/tests/unit_tests/completion_test.coffee b/tests/unit_tests/completion_test.coffee
index 3de1d716..77464b96 100644
--- a/tests/unit_tests/completion_test.coffee
+++ b/tests/unit_tests/completion_test.coffee
@@ -41,6 +41,14 @@ context "HistoryCache",
       should "return length - 1 if it should be at the end of the list", ->
         assert.equal 0, HistoryCache.binarySearch(3, [3, 5, 8], @compare)
 
+      # FIXME (smblott)
+      #       The following should pass, but fails.
+      #       I think it's what the test above intends (but misses because of a typo).
+      #       Bottom line: binarySearch can currently return an index beyond the end of the array.
+      #
+      # should "return end of list if greater than last element in list", ->
+      #   assert.equal 2, HistoryCache.binarySearch(10, [3, 5, 8], @compare)
+
       should "found return the position if it's between two elements", ->
         assert.equal 1, HistoryCache.binarySearch(4, [3, 5, 8], @compare)
         assert.equal 2, HistoryCache.binarySearch(7, [3, 5, 8], @compare)
@@ -51,9 +59,11 @@ context "HistoryCache",
       @history2 = { url: "a.com", lastVisitTime: 10 }
       history = [@history1, @history2]
       @onVisitedListener = null
+      @onVisitRemovedListerner = null
       global.chrome.history =
         search: (options, callback) -> callback(history)
         onVisited: { addListener: (@onVisitedListener) => }
+        onVisitRemoved: { addListener: (@onVisitRemovedListerner) => }
       HistoryCache.reset()
 
     should "store visits sorted by url ascending", ->
@@ -75,6 +85,30 @@ context "HistoryCache",
       HistoryCache.use (@results) =>
       assert.arrayEqual [newSite, @history1], @results
 
+    should "remove pages from the history, when page is not in history", ->
+      HistoryCache.use (@results) =>
+      assert.arrayEqual [@history2, @history1], @results
+      toRemove = { urls: [ "x.com" ], allHistory: false }
+      @onVisitRemovedListerner(toRemove)
+      HistoryCache.use (@results) =>
+      assert.arrayEqual [@history2, @history1], @results
+
+    should "remove pages from the history", ->
+      HistoryCache.use (@results) =>
+      assert.arrayEqual [@history2, @history1], @results
+      toRemove = { urls: [ "a.com" ], allHistory: false }
+      @onVisitRemovedListerner(toRemove)
+      HistoryCache.use (@results) =>
+      assert.arrayEqual [@history1], @results
+
+    should "remove all pages from the history", ->
+      HistoryCache.use (@results) =>
+      assert.arrayEqual [@history2, @history1], @results
+      toRemove = { allHistory: true }
+      @onVisitRemovedListerner(toRemove)
+      HistoryCache.use (@results) =>
+      assert.arrayEqual [], @results
+
 context "history completer",
   setup ->
     @history1 = { title: "history1", url: "history1.com", lastVisitTime: hours(1) }
@@ -83,6 +117,7 @@ context "history completer",
     global.chrome.history =
       search: (options, callback) => callback([@history1, @history2])
       onVisited: { addListener: -> }
+      onVisitRemoved: { addListener: -> }
 
     @completer = new HistoryCompleter()
 
@@ -102,7 +137,7 @@ context "domain completer",
     @history2 = { title: "history2", url: "http://history2.com", lastVisitTime: hours(1) }
 
     stub(HistoryCache, "use", (onComplete) => onComplete([@history1, @history2]))
-    global.chrome.history = { onVisited: { addListener: -> } }
+    global.chrome.history = { onVisited: { addListener: -> }, onVisitRemoved: -> }
     stub(Date, "now", returns(hours(24)))
 
     @completer = new DomainCompleter()
-- 
cgit v1.2.3


From 03823f97a526ed5e5104b724d4f2a0b582505a6c Mon Sep 17 00:00:00 2001
From: Stephen Blott
Date: Sun, 11 Nov 2012 14:10:58 +0000
Subject: Extend removing entries to the domain completer.

---
 background_scripts/completion.coffee    | 34 ++++++++++++++++++++++++++++-----
 tests/unit_tests/completion_test.coffee |  4 ++--
 2 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/background_scripts/completion.coffee b/background_scripts/completion.coffee
index 8ab71290..91f79338 100644
--- a/background_scripts/completion.coffee
+++ b/background_scripts/completion.coffee
@@ -169,7 +169,7 @@ class HistoryCompleter
 # The domain completer is designed to match a single-word query which looks like it is a domain. This supports
 # the user experience where they quickly type a partial domain, hit tab -> enter, and expect to arrive there.
 class DomainCompleter
-  domains: null # A map of domain -> history
+  domains: null # A map of domain -> { entry: <historyEntry>, referenceCount: <count> }
 
   filter: (queryTerms, onComplete) ->
     return onComplete([]) if queryTerms.length > 1
@@ -190,7 +190,7 @@ class DomainCompleter
   sortDomainsByRelevancy: (queryTerms, domainCandidates) ->
     results = []
     for domain in domainCandidates
-      recencyScore = RankingUtils.recencyScore(@domains[domain].lastVisitTime || 0)
+      recencyScore = RankingUtils.recencyScore(@domains[domain].entry.lastVisitTime || 0)
       wordRelevancy = RankingUtils.wordRelevancy(queryTerms, domain, null)
       score = (wordRelevancy + Math.max(recencyScore, wordRelevancy)) / 2
       results.push([domain, score])
@@ -205,13 +205,37 @@ class DomainCompleter
         domain = @parseDomain(entry.url)
         if domain
           previousEntry = @domains[domain]
-          @domains[domain] = entry if !previousEntry || (previousEntry.lastVisitTime < entry.lastVisitTime)
+          if previousEntry
+            previousEntry.entry = entry if previousEntry.lastVisitTime < entry.lastVisitTime
+            previousEntry.referenceCount +=1
+          else
+            @domains[domain] = { entry: entry, referenceCount: 1 }
       chrome.history.onVisited.addListener(@onPageVisited.bind(this))
+      chrome.history.onVisitRemoved.addListener(@onVisitRemoved.bind(this))
       onComplete()
 
   onPageVisited: (newPage) ->
     domain = @parseDomain(newPage.url)
-    @domains[domain] = newPage if domain
+    if domain
+      previousEntry = @domains[domain]
+      if previousEntry
+        previousEntry.entry = newPage
+        previousEntry.referenceCount += 1
+      else
+        @domains[domain] = { entry: newPage, referenceCount: 1 }
+
+  onVisitRemoved: (toRemove) ->
+    if toRemove.allHistory
+      @domains = {}
+    else
+      toRemove.urls.forEach (url) =>
+        domain = @parseDomain(url)
+        if domain
+          previousEntry = @domains[domain]
+          if previousEntry
+            previousEntry.referenceCount -= 1
+            if previousEntry.referenceCount == 0
+              delete @domains[domain]
 
   parseDomain: (url) -> url.split("/")[2] || ""
 
@@ -389,7 +413,7 @@ HistoryCache =
     if toRemove.allHistory
       @history = []
     else
-      toRemove.urls.map (url) =>
+      toRemove.urls.forEach (url) =>
         i = HistoryCache.binarySearch({url:url}, @history, @compareHistoryByUrl)
         # TODO (smblott)
         #      The `i < @history.length` condition below should not be necessary.  It can be removed when `binarySearch` is fixed.
diff --git a/tests/unit_tests/completion_test.coffee b/tests/unit_tests/completion_test.coffee
index 77464b96..5d7be78b 100644
--- a/tests/unit_tests/completion_test.coffee
+++ b/tests/unit_tests/completion_test.coffee
@@ -137,7 +137,7 @@ context "domain completer",
     @history2 = { title: "history2", url: "http://history2.com", lastVisitTime: hours(1) }
 
     stub(HistoryCache, "use", (onComplete) => onComplete([@history1, @history2]))
-    global.chrome.history = { onVisited: { addListener: -> }, onVisitRemoved: -> }
+    global.chrome.history = { onVisited: { addListener: -> }, onVisitRemoved: { addListener: -> } }
     stub(Date, "now", returns(hours(24)))
 
     @completer = new DomainCompleter()
@@ -147,7 +147,7 @@ context "domain completer",
     assert.arrayEqual ["history1.com"], results.map (result) -> result.url
 
   should "pick domains which are more recent", ->
-    # This domains are the same except for their last visited time.
+    # These domains are the same except for their last visited time.
     assert.equal "history1.com", filterCompleter(@completer, ["story"])[0].url
     @history2.lastVisitTime = hours(3)
     assert.equal "history2.com", filterCompleter(@completer, ["story"])[0].url
-- 
cgit v1.2.3


From 2071873f5c9f496e4fde94d44cbf8b0e50bf4c21 Mon Sep 17 00:00:00 2001
From: Stephen Blott
Date: Sun, 11 Nov 2012 15:59:27 +0000
Subject: Remove comments regarding non-existent bug.

---
 background_scripts/completion.coffee    | 2 --
 tests/unit_tests/completion_test.coffee | 9 ++-------
 2 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/background_scripts/completion.coffee b/background_scripts/completion.coffee
index 91f79338..c12c6d80 100644
--- a/background_scripts/completion.coffee
+++ b/background_scripts/completion.coffee
@@ -415,8 +415,6 @@ HistoryCache =
     else
       toRemove.urls.forEach (url) =>
         i = HistoryCache.binarySearch({url:url}, @history, @compareHistoryByUrl)
-        # TODO (smblott)
-        #      The `i < @history.length` condition below should not be necessary.  It can be removed when `binarySearch` is fixed.
         if i < @history.length and @history[i].url == url
           @history.splice(i, 1)
 
diff --git a/tests/unit_tests/completion_test.coffee b/tests/unit_tests/completion_test.coffee
index 5d7be78b..f0fc9ded 100644
--- a/tests/unit_tests/completion_test.coffee
+++ b/tests/unit_tests/completion_test.coffee
@@ -41,13 +41,8 @@ context "HistoryCache",
       should "return length - 1 if it should be at the end of the list", ->
         assert.equal 0, HistoryCache.binarySearch(3, [3, 5, 8], @compare)
 
-      # FIXME (smblott)
-      #       The following should pass, but fails.
-      #       I think it's what the test above intends (but misses because of a typo).
-      #       Bottom line: binarySearch can currently return an index beyond the end of the array.
-      #
-      # should "return end of list if greater than last element in list", ->
-      #   assert.equal 2, HistoryCache.binarySearch(10, [3, 5, 8], @compare)
+      should "return one passed end of list (so: list.length) if greater than last element in list", ->
+        assert.equal 3, HistoryCache.binarySearch(10, [3, 5, 8], @compare)
 
       should "found return the position if it's between two elements", ->
         assert.equal 1, HistoryCache.binarySearch(4, [3, 5, 8], @compare)
-- 
cgit v1.2.3


From 0a8443d30dde0b7ba914a742182a1900baae2836 Mon Sep 17 00:00:00 2001
From: Stephen Blott
Date: Sun, 11 Nov 2012 16:31:17 +0000
Subject: Refactor domain maintenance code.

---
 background_scripts/completion.coffee | 22 ++++++----------------
 1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/background_scripts/completion.coffee b/background_scripts/completion.coffee
index c12c6d80..95c50baf 100644
--- a/background_scripts/completion.coffee
+++ b/background_scripts/completion.coffee
@@ -200,29 +200,19 @@ class DomainCompleter
   populateDomains: (onComplete) ->
     HistoryCache.use (history) =>
       @domains = {}
-      history.forEach (entry) =>
-        # We want each key in our domains hash to point to the most recent History entry for that domain.
-        domain = @parseDomain(entry.url)
-        if domain
-          previousEntry = @domains[domain]
-          if previousEntry
-            previousEntry.entry = entry if previousEntry.lastVisitTime < entry.lastVisitTime
-            previousEntry.referenceCount +=1
-          else
-            @domains[domain] = { entry: entry, referenceCount: 1 }
+      history.forEach (entry) => @onPageVisited entry
       chrome.history.onVisited.addListener(@onPageVisited.bind(this))
       chrome.history.onVisitRemoved.addListener(@onVisitRemoved.bind(this))
       onComplete()
 
+  # We want each key in our domains hash to point to the most recent History entry for that domain.
   onPageVisited: (newPage) ->
     domain = @parseDomain(newPage.url)
     if domain
-      previousEntry = @domains[domain]
-      if previousEntry
-        previousEntry.entry = newPage
-        previousEntry.referenceCount += 1
-      else
-        @domains[domain] = { entry: newPage, referenceCount: 1 }
+      @domains[domain] ||= { entry: newPage, referenceCount: 0 }
+      slot = @domains[domain]
+      slot.entry = newPage if slot.entry.lastVisitTime < newPage.lastVisitTime
+      slot.referenceCount += 1
 
   onVisitRemoved: (toRemove) ->
     if toRemove.allHistory
-- 
cgit v1.2.3


From 9215d975207a3eb07e01cbc202fa2ee3402d332c Mon Sep 17 00:00:00 2001
From: Stephen Blott
Date: Sun, 11 Nov 2012 16:52:36 +0000
Subject: More simplification/refactoring.

---
 background_scripts/completion.coffee | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/background_scripts/completion.coffee b/background_scripts/completion.coffee
index 95c50baf..08c33430 100644
--- a/background_scripts/completion.coffee
+++ b/background_scripts/completion.coffee
@@ -205,12 +205,12 @@ class DomainCompleter
       chrome.history.onVisitRemoved.addListener(@onVisitRemoved.bind(this))
       onComplete()
 
-  # We want each key in our domains hash to point to the most recent History entry for that domain.
   onPageVisited: (newPage) ->
     domain = @parseDomain(newPage.url)
     if domain
       @domains[domain] ||= { entry: newPage, referenceCount: 0 }
       slot = @domains[domain]
+      # We want each entry in our domains hash to point to the most recent History entry for that domain.
       slot.entry = newPage if slot.entry.lastVisitTime < newPage.lastVisitTime
       slot.referenceCount += 1
 
@@ -220,12 +220,8 @@ class DomainCompleter
     else
       toRemove.urls.forEach (url) =>
         domain = @parseDomain(url)
-        if domain
-          previousEntry = @domains[domain]
-          if previousEntry
-            previousEntry.referenceCount -= 1
-            if previousEntry.referenceCount == 0
-              delete @domains[domain]
+        if domain and @domains[domain] and ( @domains[domain].referenceCount -= 1 ) == 0
+          delete @domains[domain]
 
   parseDomain: (url) -> url.split("/")[2] || ""
 
-- 
cgit v1.2.3


From 775122483b5c5dd7631e247bc3bb7c89aac0e9ab Mon Sep 17 00:00:00 2001
From: Stephen Blott
Date: Sun, 11 Nov 2012 17:33:29 +0000
Subject: Additional unit tests for domain completer.

---
 tests/unit_tests/completion_test.coffee | 35 +++++++++++++++++++++++++++------
 1 file changed, 29 insertions(+), 6 deletions(-)

diff --git a/tests/unit_tests/completion_test.coffee b/tests/unit_tests/completion_test.coffee
index f0fc9ded..6e6a2efb 100644
--- a/tests/unit_tests/completion_test.coffee
+++ b/tests/unit_tests/completion_test.coffee
@@ -54,11 +54,11 @@ context "HistoryCache",
       @history2 = { url: "a.com", lastVisitTime: 10 }
       history = [@history1, @history2]
       @onVisitedListener = null
-      @onVisitRemovedListerner = null
+      @onVisitRemovedListener = null
       global.chrome.history =
         search: (options, callback) -> callback(history)
         onVisited: { addListener: (@onVisitedListener) => }
-        onVisitRemoved: { addListener: (@onVisitRemovedListerner) => }
+        onVisitRemoved: { addListener: (@onVisitRemovedListener) => }
       HistoryCache.reset()
 
     should "store visits sorted by url ascending", ->
@@ -84,7 +84,7 @@ context "HistoryCache",
       HistoryCache.use (@results) =>
       assert.arrayEqual [@history2, @history1], @results
       toRemove = { urls: [ "x.com" ], allHistory: false }
-      @onVisitRemovedListerner(toRemove)
+      @onVisitRemovedListener(toRemove)
       HistoryCache.use (@results) =>
       assert.arrayEqual [@history2, @history1], @results
 
@@ -92,7 +92,7 @@ context "HistoryCache",
       HistoryCache.use (@results) =>
       assert.arrayEqual [@history2, @history1], @results
       toRemove = { urls: [ "a.com" ], allHistory: false }
-      @onVisitRemovedListerner(toRemove)
+      @onVisitRemovedListener(toRemove)
       HistoryCache.use (@results) =>
       assert.arrayEqual [@history1], @results
 
@@ -100,7 +100,7 @@ context "HistoryCache",
       HistoryCache.use (@results) =>
       assert.arrayEqual [@history2, @history1], @results
       toRemove = { allHistory: true }
-      @onVisitRemovedListerner(toRemove)
+      @onVisitRemovedListener(toRemove)
       HistoryCache.use (@results) =>
       assert.arrayEqual [], @results
 
@@ -132,7 +132,11 @@ context "domain completer",
     @history2 = { title: "history2", url: "http://history2.com", lastVisitTime: hours(1) }
 
     stub(HistoryCache, "use", (onComplete) => onComplete([@history1, @history2]))
-    global.chrome.history = { onVisited: { addListener: -> }, onVisitRemoved: { addListener: -> } }
+    @onVisitedListener = null
+    @onVisitRemovedListener = null
+    global.chrome.history =
+      onVisited: { addListener: (@onVisitedListener) => }
+      onVisitRemoved: { addListener: (@onVisitRemovedListener) => }
     stub(Date, "now", returns(hours(24)))
 
     @completer = new DomainCompleter()
@@ -150,6 +154,25 @@ context "domain completer",
   should "returns no results when there's more than one query term, because clearly it's not a domain", ->
     assert.arrayEqual [], filterCompleter(@completer, ["his", "tory"])
 
+  should "remove 1 matching domain entry", ->
+    # Force installation of `@onVisitRemovedListener`
+    @history2.lastVisitTime = hours(3) and filterCompleter(@completer, ["story"])
+    @onVisitRemovedListener { allHistory: false, urls: [ @history2.url ] }
+    assert.equal "history1.com", filterCompleter(@completer, ["story"])[0].url
+
+  should "remove 2 (both) matching domain entries", ->
+    # Force installation of `@onVisitRemovedListener`
+    @history2.lastVisitTime = hours(3) and filterCompleter(@completer, ["story"])
+    @onVisitRemovedListener { allHistory: false, urls: [ @history2.url ] }
+    @onVisitRemovedListener { allHistory: false, urls: [ @history1.url ] }
+    assert.isTrue filterCompleter(@completer, ["story"]).length == 0
+
+  should "remove *all* domain entries", ->
+    # Force installation of `@onVisitRemovedListener`
+    @history2.lastVisitTime = hours(3) and filterCompleter(@completer, ["story"])
+    @onVisitRemovedListener { allHistory: true }
+    assert.isTrue filterCompleter(@completer, ["story"]).length == 0
+
 context "tab completer",
   setup ->
     @tabs = [
-- 
cgit v1.2.3


From cf757fc841c6b97a8341472bd15bdc95c527babc Mon Sep 17 00:00:00 2001
From: Stephen Blott
Date: Sun, 11 Nov 2012 17:45:24 +0000
Subject: More domain completer tests.

---
 tests/unit_tests/completion_test.coffee | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/tests/unit_tests/completion_test.coffee b/tests/unit_tests/completion_test.coffee
index 6e6a2efb..e9e60ff9 100644
--- a/tests/unit_tests/completion_test.coffee
+++ b/tests/unit_tests/completion_test.coffee
@@ -130,8 +130,9 @@ context "domain completer",
   setup ->
     @history1 = { title: "history1", url: "http://history1.com", lastVisitTime: hours(1) }
     @history2 = { title: "history2", url: "http://history2.com", lastVisitTime: hours(1) }
+    @history3 = { title: "history2", url: "http://history2.com/something", lastVisitTime: hours(0) }
 
-    stub(HistoryCache, "use", (onComplete) => onComplete([@history1, @history2]))
+    stub(HistoryCache, "use", (onComplete) => onComplete([@history1, @history2, @history3]))
     @onVisitedListener = null
     @onVisitRemovedListener = null
     global.chrome.history =
@@ -142,7 +143,7 @@ context "domain completer",
     @completer = new DomainCompleter()
 
   should "return only a single matching domain", ->
-    results = filterCompleter(@completer, ["story"])
+    results = filterCompleter(@completer, ["story1"])
     assert.arrayEqual ["history1.com"], results.map (result) -> result.url
 
   should "pick domains which are more recent", ->
@@ -156,20 +157,30 @@ context "domain completer",
 
   should "remove 1 matching domain entry", ->
     # Force installation of `@onVisitRemovedListener`
-    @history2.lastVisitTime = hours(3) and filterCompleter(@completer, ["story"])
+    filterCompleter(@completer, ["story"])
     @onVisitRemovedListener { allHistory: false, urls: [ @history2.url ] }
     assert.equal "history1.com", filterCompleter(@completer, ["story"])[0].url
 
-  should "remove 2 (both) matching domain entries", ->
-    # Force installation of `@onVisitRemovedListener`
-    @history2.lastVisitTime = hours(3) and filterCompleter(@completer, ["story"])
+  should "remove all entries from one domain", ->
+    filterCompleter(@completer, ["story"]) # Force installation of `@onVisitRemovedListener`.
+    @onVisitRemovedListener { allHistory: false, urls: [ @history2.url ] }
+    @onVisitRemovedListener { allHistory: false, urls: [ @history3.url ] }
+    assert.equal "history1.com", filterCompleter(@completer, ["story"])[0].url
+
+  should "remove 3 (all) matching domain entries", ->
+    filterCompleter(@completer, ["story"]) # Force installation of `@onVisitRemovedListener`.
     @onVisitRemovedListener { allHistory: false, urls: [ @history2.url ] }
     @onVisitRemovedListener { allHistory: false, urls: [ @history1.url ] }
+    @onVisitRemovedListener { allHistory: false, urls: [ @history3.url ] }
+    assert.isTrue filterCompleter(@completer, ["story"]).length == 0
+
+  should "remove 3 (all) matching domain entries, and do it all at once", ->
+    filterCompleter(@completer, ["story"]) # Force installation of `@onVisitRemovedListener`.
+    @onVisitRemovedListener { allHistory: false, urls: [ @history2.url, @history1.url, @history3.url ] }
     assert.isTrue filterCompleter(@completer, ["story"]).length == 0
 
   should "remove *all* domain entries", ->
-    # Force installation of `@onVisitRemovedListener`
-    @history2.lastVisitTime = hours(3) and filterCompleter(@completer, ["story"])
+    filterCompleter(@completer, ["story"]) # Force installation of `@onVisitRemovedListener`.
     @onVisitRemovedListener { allHistory: true }
     assert.isTrue filterCompleter(@completer, ["story"]).length == 0
 
-- 
cgit v1.2.3


From c44f0217fbfc4df1ac6bffea4a07abe3930e634e Mon Sep 17 00:00:00 2001
From: Stephen Blott
Date: Sun, 11 Nov 2012 22:15:10 +0000
Subject: Changes in responce to philc's recommendations. See:
 https://github.com/philc/vimium/pull/715

---
 background_scripts/completion.coffee    |  6 ++++-
 tests/unit_tests/completion_test.coffee | 48 +++++++++++++++++++--------------
 2 files changed, 33 insertions(+), 21 deletions(-)

diff --git a/background_scripts/completion.coffee b/background_scripts/completion.coffee
index 08c33430..13e700d8 100644
--- a/background_scripts/completion.coffee
+++ b/background_scripts/completion.coffee
@@ -169,7 +169,11 @@ class HistoryCompleter
 # The domain completer is designed to match a single-word query which looks like it is a domain. This supports
 # the user experience where they quickly type a partial domain, hit tab -> enter, and expect to arrive there.
 class DomainCompleter
-  domains: null # A map of domain -> { entry: <historyEntry>, referenceCount: <count> }
+  # A map of domain -> { entry: <historyEntry>, referenceCount: <count> }
+  #  - `entry` is the most recently accessed page in the History within this domain.
+  #  - `referenceCount` is a count of the number of History entries within this domain.
+  #     If `referenceCount` goes to zero, the domain entry can and should be deleted.
+  domains: null
 
   filter: (queryTerms, onComplete) ->
     return onComplete([]) if queryTerms.length > 1
diff --git a/tests/unit_tests/completion_test.coffee b/tests/unit_tests/completion_test.coffee
index e9e60ff9..47fe16b6 100644
--- a/tests/unit_tests/completion_test.coffee
+++ b/tests/unit_tests/completion_test.coffee
@@ -80,7 +80,7 @@ context "HistoryCache",
       HistoryCache.use (@results) =>
       assert.arrayEqual [newSite, @history1], @results
 
-    should "remove pages from the history, when page is not in history", ->
+    should "(not) remove page from the history, when page is not in history (it should be a no-op)", ->
       HistoryCache.use (@results) =>
       assert.arrayEqual [@history2, @history1], @results
       toRemove = { urls: [ "x.com" ], allHistory: false }
@@ -130,20 +130,17 @@ context "domain completer",
   setup ->
     @history1 = { title: "history1", url: "http://history1.com", lastVisitTime: hours(1) }
     @history2 = { title: "history2", url: "http://history2.com", lastVisitTime: hours(1) }
-    @history3 = { title: "history2", url: "http://history2.com/something", lastVisitTime: hours(0) }
 
-    stub(HistoryCache, "use", (onComplete) => onComplete([@history1, @history2, @history3]))
-    @onVisitedListener = null
-    @onVisitRemovedListener = null
+    stub(HistoryCache, "use", (onComplete) => onComplete([@history1, @history2]))
     global.chrome.history =
-      onVisited: { addListener: (@onVisitedListener) => }
-      onVisitRemoved: { addListener: (@onVisitRemovedListener) => }
+      onVisited: { addListener: -> }
+      onVisitRemoved: { addListener: -> }
     stub(Date, "now", returns(hours(24)))
 
     @completer = new DomainCompleter()
 
   should "return only a single matching domain", ->
-    results = filterCompleter(@completer, ["story1"])
+    results = filterCompleter(@completer, ["story"])
     assert.arrayEqual ["history1.com"], results.map (result) -> result.url
 
   should "pick domains which are more recent", ->
@@ -155,32 +152,43 @@ context "domain completer",
   should "returns no results when there's more than one query term, because clearly it's not a domain", ->
     assert.arrayEqual [], filterCompleter(@completer, ["his", "tory"])
 
-  should "remove 1 matching domain entry", ->
-    # Force installation of `@onVisitRemovedListener`
+context "domain completer (removing entries)",
+  setup ->
+    @history1 = { title: "history1", url: "http://history1.com", lastVisitTime: hours(1) }
+    @history2 = { title: "history2", url: "http://history2.com", lastVisitTime: hours(1) }
+    @history3 = { title: "history2", url: "http://history2.com/something", lastVisitTime: hours(0) }
+
+    stub(HistoryCache, "use", (onComplete) => onComplete([@history1, @history2, @history3]))
+    @onVisitedListener = null
+    @onVisitRemovedListener = null
+    global.chrome.history =
+      onVisited: { addListener: (@onVisitedListener) => }
+      onVisitRemoved: { addListener: (@onVisitRemovedListener) => }
+    stub(Date, "now", returns(hours(24)))
+
+    @completer = new DomainCompleter()
     filterCompleter(@completer, ["story"])
-    @onVisitRemovedListener { allHistory: false, urls: [ @history2.url ] }
+
+  should "remove 1 matching domain entry", ->
+    @onVisitRemovedListener { allHistory: false, urls: [@history2.url] }
     assert.equal "history1.com", filterCompleter(@completer, ["story"])[0].url
 
   should "remove all entries from one domain", ->
-    filterCompleter(@completer, ["story"]) # Force installation of `@onVisitRemovedListener`.
-    @onVisitRemovedListener { allHistory: false, urls: [ @history2.url ] }
-    @onVisitRemovedListener { allHistory: false, urls: [ @history3.url ] }
+    @onVisitRemovedListener { allHistory: false, urls: [@history2.url] }
+    @onVisitRemovedListener { allHistory: false, urls: [@history3.url] }
     assert.equal "history1.com", filterCompleter(@completer, ["story"])[0].url
 
   should "remove 3 (all) matching domain entries", ->
-    filterCompleter(@completer, ["story"]) # Force installation of `@onVisitRemovedListener`.
-    @onVisitRemovedListener { allHistory: false, urls: [ @history2.url ] }
-    @onVisitRemovedListener { allHistory: false, urls: [ @history1.url ] }
-    @onVisitRemovedListener { allHistory: false, urls: [ @history3.url ] }
+    @onVisitRemovedListener { allHistory: false, urls: [@history2.url] }
+    @onVisitRemovedListener { allHistory: false, urls: [@history1.url] }
+    @onVisitRemovedListener { allHistory: false, urls: [@history3.url] }
     assert.isTrue filterCompleter(@completer, ["story"]).length == 0
 
   should "remove 3 (all) matching domain entries, and do it all at once", ->
-    filterCompleter(@completer, ["story"]) # Force installation of `@onVisitRemovedListener`.
     @onVisitRemovedListener { allHistory: false, urls: [ @history2.url, @history1.url, @history3.url ] }
     assert.isTrue filterCompleter(@completer, ["story"]).length == 0
 
   should "remove *all* domain entries", ->
-    filterCompleter(@completer, ["story"]) # Force installation of `@onVisitRemovedListener`.
     @onVisitRemovedListener { allHistory: true }
     assert.isTrue filterCompleter(@completer, ["story"]).length == 0
 
-- 
cgit v1.2.3


From f0347388264b0fb437489f4f021ba8c0dbcca8e3 Mon Sep 17 00:00:00 2001
From: Stephen Blott
Date: Mon, 12 Nov 2012 06:43:11 +0000
Subject: Improve unit test structure and coverage.

---
 background_scripts/completion.coffee    |  3 +--
 tests/unit_tests/completion_test.coffee | 24 ++++++++++++++----------
 2 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/background_scripts/completion.coffee b/background_scripts/completion.coffee
index 13e700d8..28bb1afa 100644
--- a/background_scripts/completion.coffee
+++ b/background_scripts/completion.coffee
@@ -212,8 +212,7 @@ class DomainCompleter
   onPageVisited: (newPage) ->
     domain = @parseDomain(newPage.url)
     if domain
-      @domains[domain] ||= { entry: newPage, referenceCount: 0 }
-      slot = @domains[domain]
+      slot = @domains[domain] ||= { entry: newPage, referenceCount: 0 }
       # We want each entry in our domains hash to point to the most recent History entry for that domain.
       slot.entry = newPage if slot.entry.lastVisitTime < newPage.lastVisitTime
       slot.referenceCount += 1
diff --git a/tests/unit_tests/completion_test.coffee b/tests/unit_tests/completion_test.coffee
index 47fe16b6..08ce46c2 100644
--- a/tests/unit_tests/completion_test.coffee
+++ b/tests/unit_tests/completion_test.coffee
@@ -41,7 +41,7 @@ context "HistoryCache",
       should "return length - 1 if it should be at the end of the list", ->
         assert.equal 0, HistoryCache.binarySearch(3, [3, 5, 8], @compare)
 
-      should "return one passed end of list (so: list.length) if greater than last element in list", ->
+      should "return one passed end of array (so: array.length) if greater than last element in array", ->
         assert.equal 3, HistoryCache.binarySearch(10, [3, 5, 8], @compare)
 
       should "found return the position if it's between two elements", ->
@@ -154,9 +154,9 @@ context "domain completer",
 
 context "domain completer (removing entries)",
   setup ->
-    @history1 = { title: "history1", url: "http://history1.com", lastVisitTime: hours(1) }
+    @history1 = { title: "history1", url: "http://history1.com", lastVisitTime: hours(2) }
     @history2 = { title: "history2", url: "http://history2.com", lastVisitTime: hours(1) }
-    @history3 = { title: "history2", url: "http://history2.com/something", lastVisitTime: hours(0) }
+    @history3 = { title: "history2something", url: "http://history2.com/something", lastVisitTime: hours(0) }
 
     stub(HistoryCache, "use", (onComplete) => onComplete([@history1, @history2, @history3]))
     @onVisitedListener = null
@@ -167,30 +167,34 @@ context "domain completer (removing entries)",
     stub(Date, "now", returns(hours(24)))
 
     @completer = new DomainCompleter()
+    # Force installation of listeners.
     filterCompleter(@completer, ["story"])
 
-  should "remove 1 matching domain entry", ->
-    @onVisitRemovedListener { allHistory: false, urls: [@history2.url] }
-    assert.equal "history1.com", filterCompleter(@completer, ["story"])[0].url
+  should "remove 1 entry for domain with reference count of 1", ->
+    @onVisitRemovedListener { allHistory: false, urls: [@history1.url] }
+    assert.equal "history2.com", filterCompleter(@completer, ["story"])[0].url
+    assert.equal 0, filterCompleter(@completer, ["story1"]).length
 
-  should "remove all entries from one domain", ->
+  should "remove 2 entries for domain with reference count of 2", ->
     @onVisitRemovedListener { allHistory: false, urls: [@history2.url] }
+    assert.equal "history2.com", filterCompleter(@completer, ["story2"])[0].url
     @onVisitRemovedListener { allHistory: false, urls: [@history3.url] }
+    assert.equal 0, filterCompleter(@completer, ["story2"]).length
     assert.equal "history1.com", filterCompleter(@completer, ["story"])[0].url
 
   should "remove 3 (all) matching domain entries", ->
     @onVisitRemovedListener { allHistory: false, urls: [@history2.url] }
     @onVisitRemovedListener { allHistory: false, urls: [@history1.url] }
     @onVisitRemovedListener { allHistory: false, urls: [@history3.url] }
-    assert.isTrue filterCompleter(@completer, ["story"]).length == 0
+    assert.equal 0, filterCompleter(@completer, ["story"]).length
 
   should "remove 3 (all) matching domain entries, and do it all at once", ->
     @onVisitRemovedListener { allHistory: false, urls: [ @history2.url, @history1.url, @history3.url ] }
-    assert.isTrue filterCompleter(@completer, ["story"]).length == 0
+    assert.equal 0, filterCompleter(@completer, ["story"]).length
 
   should "remove *all* domain entries", ->
     @onVisitRemovedListener { allHistory: true }
-    assert.isTrue filterCompleter(@completer, ["story"]).length == 0
+    assert.equal 0, filterCompleter(@completer, ["story"]).length
 
 context "tab completer",
   setup ->
-- 
cgit v1.2.3