Keep bookmarked entries on history

[cezaraugusto]

• Submitted a ticket for my issue if one did not already exist.
• Used Github auto-closing keywords in the commit message.
• Ran git rebase -i to squash commits (if needed).

Auditor: @bsclifton, @darkdh

Fix #6160

Test plan:

Automated tests must pass

npm run test -- --grep="returns newSiteDetail value for lastAccessedTime when oldSite value is undefined"

npm run test -- --grep="returns oldSiteDetail value for lastAccessedTime when newSite value is undefined"

Manual tests:

1. Visit a site
2. Go to about:history and check entry is there
3. Bookmark that site
4. Entry should still be visible on about:history
5. Bookmark about:history
6. History should be bookmarked (but not visible on about:history as we don't store about pages)

[darkdh]

We should add this case into

siteUtil
    addSite
      for new entries (oldSite is null)
        when adding history

[bsclifton]

Great fix, @cezaraugusto 😄 Only thing I can add is a +1 for @darkdh's comment for adding a test

[bsclifton]

@cezaraugusto does this also fix #5413?

[cezaraugusto]

@bsclifton not really, that one happens due to our site's counter. There's an inconsistency between the moment we add a bookmark, but fix is on the way (next).

[darkdh]

I guess you might forgot to call addSite for this siteDetail and you may need to leverage previous processedSites as first argument.

[bsclifton]

@cezaraugusto the above mention by @darkdh is correct; we need to actually call addSite (which then calls mergeSiteDetails, where the change was made). When you call addSite, you'll want to make a test case for each value. Where it returns:

• lastAccessedTime = newSiteDetail.get('lastAccessedTime')
• lastAccessedTime = oldSiteDetail.get('lastAccessedTime')
• lastAccessedTime = 0

[cezaraugusto]

ok thanks, added one for oldSites and one for newSites. Case for lastAccessedTime = 0 was already covered by @darkdh, since it's the same case AFAIU, I left it as-is.

[darkdh]

Test looks good but I think we should remove oldSiteDetail's tags to make it closer to our use case "Bookmark a site after it is added to history"

[darkdh]

lgtm

[bsclifton]

Looks great 😄 Thanks for the tests!

[bsclifton]

CI had 2 errors; I re-ran both of those locally and they both passed. Merging 😄