Merge pull request #3336 from bsclifton/history-bugfix

Bookmarked history entries now show context menu in about:history

docs/windowActions.md

@@ -250,6 +250,12 @@ The new frame is expected to appear at the index it was last closed at
 
 
 
+### clearClosedFrames() 
+
+Dispatches a message to the store to clear closed frames
+
+
+
 ### setActiveFrame(frameProps) 
 
 Dispatches a message to the store to set a new frame as the active frame.

js/contextMenus.js

@@ -218,7 +218,7 @@ function siteDetailTemplateInit (siteDetail, activeFrame) {
   let deleteLabel
   let deleteTag
 
-  if (siteUtil.isBookmark(siteDetail)) {
+  if (siteUtil.isBookmark(siteDetail) && activeFrame) {
     deleteLabel = 'deleteBookmark'
     deleteTag = siteTags.BOOKMARK
   } else if (siteUtil.isFolder(siteDetail)) {
@@ -969,7 +969,8 @@ function onHamburgerMenu (location, e) {
 
 function onMainContextMenu (nodeProps, frame, contextMenuType) {
   if (contextMenuType === 'bookmark' || contextMenuType === 'bookmark-folder') {
-    onSiteDetailContextMenu(Immutable.fromJS(nodeProps), Immutable.fromJS({ location: '', title: '', partitionNumber: frame.get('partitionNumber') }))
+    const activeFrame = Immutable.fromJS({ location: '', title: '', partitionNumber: frame.get('partitionNumber') })
+    onSiteDetailContextMenu(Immutable.fromJS(nodeProps), activeFrame)
   } else if (contextMenuType === 'history') {
     onSiteDetailContextMenu(Immutable.fromJS(nodeProps))
   } else if (contextMenuType === 'download') {

js/state/siteUtil.js

@@ -155,12 +155,15 @@ module.exports.removeSite = function (sites, siteDetail, tag) {
     return sites
   }
   const tags = sites.getIn([index, 'tags'])
-  // If there are no tags and the removeSite call was called without a specific tag
-  // then remove the entry completely.
   if (tags.size === 0 && !tag) {
-    sites = sites.splice(index, 1)
-    return sites
-  }
+    // If called without tags and entry has no tags, remove the entry
+    return sites.splice(index, 1)
+  } else if (tags.size > 0 && !tag) {
+    // If called without tags BUT entry has tags, null out lastAccessedTime.
+    // This is a bookmark entry that we want to clear history for (but NOT delete/untag bookmark)
+    return sites.setIn([index, 'lastAccessedTime'], null)
+  }
+  // Else, remove the specified tag
   return sites
     .setIn([index, 'parentFolderId'], 0)
     .setIn([index, 'tags'], tags.toSet().remove(tag).toList())

test/unit/state/siteUtilTest.js

@@ -181,14 +181,27 @@ describe('siteUtil', function () {
       const expectedSites = sites.setIn([0, 'parentFolderId'], 0).setIn([0, 'tags'], Immutable.List([]))
       assert.deepEqual(processedSites, expectedSites)
     })
-    it('deletes a history entry (no tag specified)', function () {
-      const siteDetail = {
-        tags: [],
-        location: testUrl1
-      }
-      const sites = Immutable.fromJS([siteDetail])
-      const processedSites = siteUtil.removeSite(sites, Immutable.fromJS(siteDetail))
-      assert.deepEqual(processedSites, Immutable.fromJS([]))
+    describe('called with tag=null/undefined', function () {
+      it('deletes a history entry (has no tags)', function () {
+        const siteDetail = {
+          tags: [],
+          location: testUrl1
+        }
+        const sites = Immutable.fromJS([siteDetail])
+        const processedSites = siteUtil.removeSite(sites, Immutable.fromJS(siteDetail))
+        assert.deepEqual(processedSites, Immutable.fromJS([]))
+      })
+      it('nulls out the lastAccessedTime for a bookmarked entry (has tag)', function () {
+        const siteDetail = {
+          location: testUrl1,
+          tags: [siteTags.BOOKMARK],
+          lastAccessedTime: 123
+        }
+        const sites = Immutable.fromJS([siteDetail])
+        const processedSites = siteUtil.removeSite(sites, Immutable.fromJS(siteDetail))
+        const expectedSites = sites.setIn([0, 'lastAccessedTime'], null)
+        assert.deepEqual(processedSites, expectedSites)
+      })
     })
   })