Merge pull request philc#715 from smblott-github/history--remove-entries

Remove entries from History/Domain completers when they're removed on chrome.

Author: philc

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 -> history
+  # 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
@@ -190,7 +194,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])
@@ -200,18 +204,27 @@ 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]
-          @domains[domain] = entry if !previousEntry || (previousEntry.lastVisitTime < entry.lastVisitTime)
+      history.forEach (entry) => @onPageVisited entry
       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
+      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
+
+  onVisitRemoved: (toRemove) ->
+    if toRemove.allHistory
+      @domains = {}
+    else
+      toRemove.urls.forEach (url) =>
+        domain = @parseDomain(url)
+        if domain and @domains[domain] and ( @domains[domain].referenceCount -= 1 ) == 0
+          delete @domains[domain]
 
   parseDomain: (url) -> url.split("/")[2] || ""
 
@@ -365,6 +378,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 +397,16 @@ 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.forEach (url) =>
+        i = HistoryCache.binarySearch({url:url}, @history, @compareHistoryByUrl)
+        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.

tests/unit_tests/completion_test.coffee

@@ -41,6 +41,9 @@ 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 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", ->
         assert.equal 1, HistoryCache.binarySearch(4, [3, 5, 8], @compare)
         assert.equal 2, HistoryCache.binarySearch(7, [3, 5, 8], @compare)
@@ -51,9 +54,11 @@ context "HistoryCache",
       @history2 = { url: "a.com", lastVisitTime: 10 }
       history = [@history1, @history2]
       @onVisitedListener = null
+      @onVisitRemovedListener = null
       global.chrome.history =
         search: (options, callback) -> callback(history)
         onVisited: { addListener: (@onVisitedListener) => }
+        onVisitRemoved: { addListener: (@onVisitRemovedListener) => }
       HistoryCache.reset()
 
     should "store visits sorted by url ascending", ->
@@ -75,6 +80,30 @@ context "HistoryCache",
       HistoryCache.use (@results) =>
       assert.arrayEqual [newSite, @history1], @results
 
+    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 }
+      @onVisitRemovedListener(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 }
+      @onVisitRemovedListener(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 }
+      @onVisitRemovedListener(toRemove)
+      HistoryCache.use (@results) =>
+      assert.arrayEqual [], @results
+
 context "history completer",
   setup ->
     @history1 = { title: "history1", url: "history1.com", lastVisitTime: hours(1) }
@@ -83,6 +112,7 @@ context "history completer",
     global.chrome.history =
       search: (options, callback) => callback([@history1, @history2])
       onVisited: { addListener: -> }
+      onVisitRemoved: { addListener: -> }
 
     @completer = new HistoryCompleter()
 
@@ -102,7 +132,9 @@ 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: { addListener: -> }
     stub(Date, "now", returns(hours(24)))
 
     @completer = new DomainCompleter()
@@ -112,14 +144,58 @@ 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
 
   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"])
 
+context "domain completer (removing entries)",
+  setup ->
+    @history1 = { title: "history1", url: "http://history1.com", lastVisitTime: hours(2) }
+    @history2 = { title: "history2", url: "http://history2.com", lastVisitTime: hours(1) }
+    @history3 = { title: "history2something", 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()
+    # Force installation of listeners.
+    filterCompleter(@completer, ["story"])
+
+  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 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.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.equal 0, filterCompleter(@completer, ["story"]).length
+
+  should "remove *all* domain entries", ->
+    @onVisitRemovedListener { allHistory: true }
+    assert.equal 0, filterCompleter(@completer, ["story"]).length
+
 context "tab completer",
   setup ->
     @tabs = [