Closed Bug 380639 Opened 13 years ago Closed 2 years ago
dynamic removal of feeds/<link> elements (DOMLink
Removed) don't cause updates to the feed discovery menu (live bookmarks)
Component: Bookmarks → RSS Discovery and Preview
OS: Linux → All
QA Contact: bookmarks → rss.preview
Hardware: PC → All
This is exactly what I am looking for!
For instructions on moving forward with the patch, see: http://developer.mozilla.org/en/docs/Getting_your_patch_in_the_tree
Can somebody else submit Mark's fix, or does it have to be Mark himself?
Doesn't matter, but fwiw, I don't think that patch would be accepted as is, because it introduces quite a bit of duplicated code.
More significant than the duplicated code is that it just doesn't work: Array.pop() takes off the last element of the array, it doesn't take arguments and search for them. An actual working patch would need to do that searching, would need to decide what constitutes a match if it isn't exactly "all three match" (if you add a feed with one title, change the title, then remove the link element, does it remove the feed with a matching href and type, or not?), and now that we have a single listener for DOMLinkAdded for feeds, icons, and search, would probably need to handle removing all three, and would need automated tests along the lines of the tests for adding in http://mxr.mozilla.org/seamonkey/source/browser/base/content/test/
Status: UNCONFIRMED → NEW
Ever confirmed: true
Thanks for the info. I've never submitted anything myself before, but it gives me some direction (and some hint of what I might be up against) if I wanted to try to do a patch myself.
Is anyone actively working on the patch? If not I may give it a shot.
William, by all means feel free to work on it (it does not appear anyone is currently)... Do realize that currently Firefox 3 is nearing its "Beta 4" milestone which means that any code changes are strictly looked at, so no-one can guarantee that your change will actually be in Firefox 3 (yet). But if its done fast enough, and gets needed reviews, hopefully it can make it!
See bug 471535 comment 6 and subsequent comments for some thoughts about how to fix this. Claudio and Matteo also mention an issue with DOMLinkRemoved in that bug, but there aren't really any details about the problem - Claudio, can you elaborate?
Summary: missing DOMLinkRemoved handler for dynamic removal of live bookmarks → dynamic removal feeds/<link> elements (DOMLinkRemoved) don't cause updates to the feed discovery menu (live bookmarks)
Summary: dynamic removal feeds/<link> elements (DOMLinkRemoved) don't cause updates to the feed discovery menu (live bookmarks) → dynamic removal of feeds/<link> elements (DOMLinkRemoved) don't cause updates to the feed discovery menu (live bookmarks)
Alternatively, we could just update the menu each time it's opened (which would also fix bug 396056).
The issue with DOMLinkRemoved is that this event is never fired. The workaround by Matteo fires a DOMLinkRemoved event when the DOMNodeRemoved event has a LINK element as original target. That is the purpose of the _FixDOMEvents function added as event handler to catch DOMNodeRemoved events right after the DOMContentLoaded event.
I would rather fix this in Gecko with DOMLinkRemoved correctly than have us incur a perf penalty with the DOMNodeRemoved listener[s] on the document. Is there any case where DOMLinkRemoved *is* fired? I thought there was a few at least!
Is there anyone on this bug who would be kind enough to mentor it?
(In reply to Anthony Ricaud (:rik) (out until June 2nd) from comment #18) > Is there anyone on this bug who would be kind enough to mentor it? Sure, although solving this with the approach in comment 13 is trickier now with multiple processes which makes me think we should get DOMLinkRemoved working and just listen for that. I just tested and it seems to work for some removals while not others but I haven't investigated why. To start, I would recommend making a test case showing that DOMLinkRemoved doesn't always work and attaching it in a Core dependency bug. With that bug fixed, this should be straightforward. The relevant code has moved a bit due to E10S: * https://mxr.mozilla.org/mozilla-central/search?string=Link%3AAddFeed * https://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-feeds.js
Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → INACTIVE
You need to log in before you can comment on or make changes to this bug.