Closed Bug 487511 Opened 13 years ago Closed 13 years ago

nsINavHistoryObserver has no "onBeforeDeleteURI" callback

Categories

(Toolkit :: Places, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: hello, Assigned: sdwilsh)

References

Details

(Keywords: dev-doc-complete, fixed1.9.1)

Attachments

(2 files)

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.
Assignee: nobody → sdwilsh
Status: NEW → ASSIGNED
tracking-fennec: --- → ?
Flags: blocking1.9.1?
Target Milestone: --- → mozilla1.9.2a1
Version: unspecified → Trunk
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!)
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Summary: nsINavHistoryObserver has no "onBeforeRemove" callback → nsINavHistoryObserver has no "onBeforeDelete" callback
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.
Attached patch v1.0Splinter Review
Not bad at all.  Should be a pretty trivial branch patch too!
Attachment #371957 - Flags: review?
Attachment #371957 - Flags: review? → review?(dietrich)
Whiteboard: [needs review dietrich]
(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
Attachment #371957 - Flags: review?(dietrich) → review+
Summary: nsINavHistoryObserver has no "onBeforeDelete" callback → nsINavHistoryObserver has no "onBeforeDeleteURI" callback
http://hg.mozilla.org/mozilla-central/rev/b0c75eac0898
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [needs review dietrich]
Attached patch branch v1.0Splinter Review
There actually isn't much change here, but I'd like dietrich to skim it over anyway.
Attachment #372138 - Flags: review?
Attachment #372138 - Flags: review? → review?(dietrich)
Blocks: 415372
Comment on attachment 372138 [details] [diff] [review]
branch v1.0

r=me, thanks
Attachment #372138 - Flags: review?(dietrich) → review+
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.
Depends on: 513466
Blocks: 507666
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.