1330934 - New tab page tiles do not get removed immediately when new removing all history visits for a particular site

Status: NEW

(Firefox :: New Tab Page, defect, P3)

Description

[:Gijs (he/him)]

+++ This bug was initially created as a clone of Bug #1330384 +++

This used to affect automigration undo, but that specific case was worked around in bug 1330384.

This still affects e.g. "forget about this site", or manual clearing of history in the library or history sidebar. It does not affect 'clear recent history', because the new tab page listens for this separately.

It seems right now the builtin onDeleteURI handlers do nothing, whereas they should really ensure that tiles get removed correctly, or use the onDeleteVisits handler to do this (though that doesn't seem to pass on a 'current visit count', so I'm not sure that works).

Doing so seems to be tricky because IME the sitemap/linkmap that the new tab page keeps (AIUI, to unify things like "foo.com/bar" and "foo.com/baz" or whatever) decrements/increments per URI, and for some reason I'm seeing the counter getting out of whack. There's also latent bugs in some of the observer calling where only the first parameter gets passed, and for deletions you need 2 more parameters (so this implementation needs updating: https://dxr.mozilla.org/mozilla-central/rev/b1c31c4a0a678194931779e0f13fba7b508eb109/toolkit/modules/NewTabUtils.jsm#788-798 ). Also, it seems there are two places history observers, namely here:

https://dxr.mozilla.org/mozilla-central/rev/b1c31c4a0a678194931779e0f13fba7b508eb109/browser/components/newtab/PlacesProvider.jsm#106

and here:

https://dxr.mozilla.org/mozilla-central/rev/b1c31c4a0a678194931779e0f13fba7b508eb109/toolkit/modules/NewTabUtils.jsm#628

and it's not clear to me why there are two instead of one. This code is... very messy.

I don't know that this should be particularly high-priority given that it doesn't affect clear recent history (and therefore, I assume, probably doesn't affect the forget button either).

Comment 1

[Marco Bonardo [:mak]]

(In reply to :Gijs from comment #0)
> It seems right now the builtin onDeleteURI handlers do nothing, whereas they
> should really ensure that tiles get removed correctly, or use the
> onDeleteVisits handler to do this (though that doesn't seem to pass on a
> 'current visit count', so I'm not sure that works).

To me looks like both would be needed, onDeleteURI is invoked when a page has no more visits nor bookmarks, while onDeleteVisits (WHEN aVisitTime == 0) is invoked when a page has no more visits, but still has bookmarks.
The query for the new tab page seems to only care about the fact the page has visits.

Comment 2

[Tim Spurway [:tspurway]]

Nan, is this still an issue under Activity Stream?

Comment 3

[Nan Jiang [:nanj]]

re: onDeleteURI, Activity Stream correctly responds to all those history ops, such as "forget about this site", manually delete the history, and clear recent history.

It listens to onDeleteURI here,

http://searchfox.org/mozilla-central/source/browser/extensions/activity-stream/lib/PlacesFeed.jsm#45

re: onDeleteVisits, we're currently not doing anything special to onDeleteVisits,

http://searchfox.org/mozilla-central/source/browser/extensions/activity-stream/lib/PlacesFeed.jsm#67

That being said, a bookmarked page will remain in the topsites, even if the user chooses to "forget about this site" or delete it manually from history. Note that since AS also shows recent bookmarks in the Highlights section, this affects highlights as well. I recall this is the desired behavior for now, but we could "need design feedback" to confirm it.