onItemChanged() "cleartime" is sent when visits to a bookmark are removed, we could use an history notification instead. This depends on implementing the history notification in bug 1678618.

So, the matter here is that cleartime could be replaced with page-removed, the only downside of doing that is the consumer won't know if the page is bookmarked and likely cleartime consumers only care about bookmarks. It's basically a lower traffic page-removed notified only for bookmarked pages.

Actually there is only one consumer of cleartime, https://searchfox.org/mozilla-central/rev/3151f97de27730793c2e298716df760999423f26/toolkit/components/places/nsNavHistoryResult.cpp#3190 and the only thing it does is basically to clear the treeview columns visit_count and most_recent_visit.

I think there's just 2 solutions:

1. we introduce bookmark-visits-removed, that is pretty much the same. The downside is duplicating the page-removed notification, potentially creating more confusion.

2. We could check result->CanSkipHistoryDetailsNotifications(), and if it's false register the result for page-removed... we currently don't do that because bookmark views usually don't care about history
   https://searchfox.org/mozilla-central/rev/3151f97de27730793c2e298716df760999423f26/toolkit/components/places/nsNavHistoryResult.cpp#1854,1859,1863
   At that point folder nodes will have to handle page_removed, that is a potential perf problem because they'll handle every uris, not just bookmarked ones. If it's trivial we could add an isBookmarked property to page-removed, and then a bookmark result could easily skip notifying descendants, if the notification is not about a bookmarked page.

I have investigated this, have some questions.

First, I took a look at the behavior along with the following STR.
Especially, how NodeHistoryDetailsChanged is handled after catching cleartime event in nsNavHistoryResultNode.
https://searchfox.org/mozilla-central/rev/3151f97de27730793c2e298716df760999423f26/toolkit/components/places/nsNavHistoryResult.cpp#3190-3196

1. Open http://wikipedia.org/ .
2. Bookmark it on the urlbar.
3. Open library panel.
4. Copy the bookmark to another bookmark folder.
5. Select "today" of history.
6. Remove wikipedia page from it.

The places where implement NodeHistoryDetailsChanged is three, but two of them are empty.
https://searchfox.org/mozilla-central/search?q=NodeHistoryDetailsChanged&path=&case=false&regexp=false
The other one is in treeView.
https://searchfox.org/mozilla-central/rev/489e82dcc1e5afbe691ff3b1c982382914637e38/browser/components/places/content/treeView.js#952
I added a log there, but it seems that it was not called when the above STR.
Perhaps, the reason is that we register the history observer when selecting the one of the directories on the library panel such as "today". But, it seems that when removing item from the folder or the folder itself, as nsNavHistoryResult also will be remove, treeView does not listen?? (I am not sure 100% since I confirmed RemoveObserver was not called..)
And also, I threw a patch that commented out to stop NOTIFY_RESULT_OBSERVERS after catching "cleartime", it seems that there is no test failure.
try: https://treeherder.mozilla.org/jobs?repo=try&selectedTaskRun=YBWn5WHVRBG3cy1Ahiz5FA.0&revision=3ceebe9dce854045607f3937af1268b794751d0b
code: https://hg.mozilla.org/try/rev/f38caca20fca6ee7fb7607a2184f45d38f32a39e#l3.57

So, questions.
Perhaps, we may no longer need the event? Or do we have another STR?

(In reply to Daisuke Akatsuka (:daisuke) from comment #2)

The places where implement NodeHistoryDetailsChanged is three, but two of them are empty.
https://searchfox.org/mozilla-central/search?q=NodeHistoryDetailsChanged&path=&case=false&regexp=false
The other one is in treeView.
https://searchfox.org/mozilla-central/rev/489e82dcc1e5afbe691ff3b1c982382914637e38/browser/components/places/content/treeView.js#952

This seems to make sense, menus and toolbars don't really care about history details... potentially if a menu shows bookmarks sorted by some history defail it should care to update sorting, but the view should only be notified about sorting changed, it won't do anything with history details because it doesn't show them

I added a log there, but it seems that it was not called when the above STR.
Perhaps, the reason is that we register the history observer when selecting the one of the directories on the library panel such as "today". But, it seems that when removing item from the folder or the folder itself, as nsNavHistoryResult also will be remove, treeView does not listen?? (I am not sure 100% since I confirmed RemoveObserver was not called..)

I'm not sure I understand, is nsNavHistoryResult not receiving onItemChanged("cleartime") at all? That would mean the result is not registered as a bookmarks listener, that's unlikely since it's a bookmarks view. Or we are receiving cleartime but not forwarding it to the view?
What's the value of CanSkipHistoryDetailsNotifications()? which pane of the Library are you observing?

And also, I threw a patch that commented out to stop NOTIFY_RESULT_OBSERVERS after catching "cleartime", it seems that there is no test failure.

I'm not surprised, we only have a syntetic test at https://searchfox.org/mozilla-central/rev/489e82dcc1e5afbe691ff3b1c982382914637e38/toolkit/components/places/tests/bookmarks/test_async_observers.js#56-85 that is unlikely to properly test the UI reacting to this.
We may have to add tests.

Perhaps, we may no longer need the event? Or do we have another STR?

The STR should be more or less that, have a visited bookmark in a bookmarks treeview, remove its history.
We need some kind of notification because otherwise these cases may break:
a. A bookmarks view sorted by date or visit time, would not be resorted after visits removals
b. A bookmarks view showing visit related columns (like Most Recent Visit), would not be updated properly

While this is not specific STR, I hope it helps understanding what the notification is supposed to do.

(In reply to Marco Bonardo [:mak] from comment #3)

Perhaps, we may no longer need the event? Or do we have another STR?

The STR should be more or less that, have a visited bookmark in a bookmarks treeview, remove its history.
We need some kind of notification because otherwise these cases may break:
a. A bookmarks view sorted by date or visit time, would not be resorted after visits removals
b. A bookmarks view showing visit related columns (like Most Recent Visit), would not be updated properly

While this is not specific STR, I hope it helps understanding what the notification is supposed to do.

Thank you very much.
Thanks to your hints, I could find a way that receives cleartime, then reaches to nodeHistoryDetailsChanged of treeView from NOTIFY_RESULT_OBSERVERS. The STR is like below.

1. Open http://wikipedia.org/ .
2. Bookmark it on the urlbar.
3. Open library panel.
4. Select a directory that contains the bookmark.
5. Add Most Recent Visit column. (This was important! :)
6. Remove the history via Clear Recent History panel.

I will try to add a test with the above STR.

The following patch is waiting for review from an inactive reviewer:

:daisuke, could you please find another reviewer or abandon the patch if it is no longer relevant?

For more information, please visit auto_nag documentation.

https://hg.mozilla.org/mozilla-central/rev/d5674706401e