nsINavHistoryObserver has no "onBeforeDeleteURI" callback

RESOLVED FIXED in mozilla1.9.2a1

Status

()

defect
P2
normal
RESOLVED FIXED
10 years ago
5 years ago

People

(Reporter: hello, Assigned: sdwilsh)

Tracking

({dev-doc-complete, fixed1.9.1})

Trunk
mozilla1.9.2a1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-testsuite +
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

10 years ago
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.
(Assignee)

Comment 1

10 years ago
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?
(Assignee)

Comment 3

10 years ago
(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.
(Assignee)

Comment 4

10 years ago
(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
(Assignee)

Updated

10 years ago
Summary: nsINavHistoryObserver has no "onBeforeRemove" callback → nsINavHistoryObserver has no "onBeforeDelete" callback
(Assignee)

Comment 6

10 years ago
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.
(Assignee)

Comment 7

10 years ago
Posted patch v1.0Splinter Review
Not bad at all.  Should be a pretty trivial branch patch too!
Attachment #371957 - Flags: review?
(Assignee)

Updated

10 years ago
Attachment #371957 - Flags: review? → review?(dietrich)
(Assignee)

Updated

10 years ago
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).
(Assignee)

Comment 9

10 years ago
(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+
(Assignee)

Updated

10 years ago
Summary: nsINavHistoryObserver has no "onBeforeDelete" callback → nsINavHistoryObserver has no "onBeforeDeleteURI" callback
(Assignee)

Comment 11

10 years ago
http://hg.mozilla.org/mozilla-central/rev/b0c75eac0898
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [needs review dietrich]
(Assignee)

Comment 12

10 years ago
Posted 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?
(Assignee)

Updated

10 years ago
Attachment #372138 - Flags: review? → review?(dietrich)

Updated

10 years ago
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. :)
(Assignee)

Comment 17

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

Updated

10 years ago
Depends on: 513466

Updated

10 years ago
Blocks: 507666
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.