Bug 468305 was about adding this callback to the bookmarks API, but history is missing it too. For history it can be worse for Weave to build an id->guid mapping table, since there are usually many more items than for bookmarks.

Requesting blocking because this really hurts weave/fennec integration.

What's the compatibility hit of adding this late? ETA to a patch?

(In reply to comment #2) > What's the compatibility hit of adding this late? ETA to a patch? I can do this just like bug 468305 - adding a special interface for branch, but modifying it on trunk. This will not break binary add-ons, and degrades gracefully for JS ones. ETA on patch is 48 hours for trunk, and another 24 hours after review for a branch patch.

(of course, this bug bumps up in priority if it becomes a blocker)

(which it has!)

I can't believe we only notify in RemovePage. This notification is almost not even useful to implementers... Probably need follow-up bugs (that won't make 1.9.1) for adding notifications in at least RemovePages, and maybe a few other places.

Not bad at all. Should be a pretty trivial branch patch too!

(In reply to comment #6) > Probably need follow-up bugs (that won't make 1.9.1) for adding notifications > in at least RemovePages, and maybe a few other places. The problem with those notifications was that they caused views to update on every removed page, while they should have not. Clearly i think we should implement on views a sort of notification cache, during a batch they should cache all received notifications, on end batch they should analyze the received notification and see if they need to do more than a simple refresh (or if they CAN do something faster than a full refresh).

(In reply to comment #8) > The problem with those notifications was that they caused views to update on > every removed page, while they should have not. Clearly i think we should > implement on views a sort of notification cache, during a batch they should > cache all received notifications, on end batch they should analyze the received > notification and see if they need to do more than a simple refresh (or if they > CAN do something faster than a full refresh). Our code is a mess in that regard, yeah. Don't even get me started about the last parameter to onVisit...

Comment on attachment 371957 [details] [diff] [review] v1.0 r=me

There actually isn't much change here, but I'd like dietrich to skim it over anyway.

Comment on attachment 372138 [details] [diff] [review] branch v1.0 r=me, thanks

Question: would it be okay for me to just document this method within nsINavBookmarkObserver even though in 1.9.1 it's in its own interface, since it'll be moved in later, and this is largely an implementation detail that doesn't impact most people? With a note mentioning the situation for those people that it does actually affect?

Except, you know, I meant nsINavHistoryObserver. :)

I would say that it's OK to document it there, and make a note on how to use it in 1.9.1.