Closed
Bug 244078
(livemark)
Opened 20 years ago
Closed 20 years ago
Live Bookmarks/Livemark (RSS feed integration)
Categories
(Firefox :: Bookmarks & History, defect, P3)
Firefox
Bookmarks & History
Tracking
()
RESOLVED
FIXED
Firefox1.0
People
(Reporter: bugs, Assigned: vlad)
Details
(Keywords: fixed-aviary1.0, relnote)
Attachments
(9 files, 6 obsolete files)
682 bytes,
application/octet-stream
|
Details | |
4.39 KB,
patch
|
jst
:
review+
shaver
:
superreview+
|
Details | Diff | Splinter Review |
101.46 KB,
patch
|
bugs
:
review+
|
Details | Diff | Splinter Review |
1.07 KB,
text/html
|
Details | |
6.68 KB,
patch
|
shaver
:
review+
|
Details | Diff | Splinter Review |
1.66 KB,
text/plain
|
Details | |
993 bytes,
patch
|
shaver
:
review+
|
Details | Diff | Splinter Review |
2.87 KB,
patch
|
shaver
:
review+
|
Details | Diff | Splinter Review |
6.38 KB,
patch
|
shaver
:
review+
|
Details | Diff | Splinter Review |
.
Reporter | ||
Updated•20 years ago
|
Status: NEW → ASSIGNED
Flags: blocking1.0+
Priority: -- → P3
Target Milestone: --- → Firefox1.0
Reporter | ||
Comment 1•20 years ago
|
||
RSS feed integration into Firefox... specifically: - when a page is encountered that has the <link rel="alternate" type="application/rss+xml" title="RSS" href="http://www.foo.com/index.xml" /> link tag in the <head> display an icon in the status bar that opens an Add Bookmark dialog to add the feed as a bookmark. - RSS Feed bookmarks behave like folders in that they can be opened, showing the posts as bookmarks underneath. They should be immutable folders however (cannot cut, delete from them, cannot insert into them, drag operations blocked). - the major RSS formats should be supported (1.0 RDF, 2.0 XML etc) A suggested approach is to decorate such bookmarks with a flag, e.g. LIVE_BOOKMARK="true" and when the bookmarks datasource is asked for children of that container, it can see that it's a live bookmark and fetch the content. Caching of results can be implemented if there are update problems. As a side note Live Bookmarks are the perfect use case of Scheduled Update Notifications... they are files that change often and there's a real value in having the icon change subtly or something similar when there's a new post. This should not be seen as a pre-requisite for the former however. I'm not likely to get to this for 1.0 so I'm looking for help to implement... this would be a great project for someone to get their feet wet in RDF/Bookmarks code.
Keywords: helpwanted
Comment 2•20 years ago
|
||
If we're going to do this, we should probably add Atom 0.3 XML support too. It's widely used amongst blogging communities.
Comment 3•20 years ago
|
||
See also bug 240393.
Summary: Live Bookmarks → Live Bookmarks (RSS feed integration)
To be honest, I'd say this is extension material, like an enhanced version of RSS Reader Panel http://fls.moo.jp/moz/rssreader.html
Reporter | ||
Comment 5•20 years ago
|
||
-> vlad, who has been working on a patch.
Assignee: bugs → vladimir
Status: ASSIGNED → NEW
Alias: livemark
Assignee | ||
Comment 6•20 years ago
|
||
Proposed patch is attached. Bookmarks menu might (will) behave strangely unless patch for bug 246908 is also applied -- I'd love to get rid of the explicit rdf:type Livemark template in the bookmarks menu, but I can't get the template builder to switch which template to apply once the rdf container stops being empty (nsXULTemplateBuilder::Retract() "write me" comment :)
Assignee | ||
Comment 7•20 years ago
|
||
Assignee | ||
Comment 8•20 years ago
|
||
My "clean" tree I was diffing against wasn't so clean. New patch attached.
Assignee | ||
Updated•20 years ago
|
Attachment #150844 -
Attachment is obsolete: true
Assignee | ||
Comment 9•20 years ago
|
||
added ability to reload livemarks (right-click, also bookmarks manager edit menu). also fixed minor icon issue in the bookmarks manager with new theme (lost a diff to the .css earlier)
Attachment #150872 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•20 years ago
|
Attachment #151015 -
Flags: review?(bugs)
Assignee | ||
Comment 10•20 years ago
|
||
Was just pointed to thunderbird bug 225158 -- there's probably a decent amount of code that can be shared (in creating a common RSS parsing/notification/etc. service).
Comment 11•20 years ago
|
||
vlad, is there any experimental build to test this feature?
Comment 12•20 years ago
|
||
(In reply to comment #11) > vlad, > is there any experimental build to test this feature? Trunk?
Comment 13•20 years ago
|
||
I don't see this feature in the trunk build. Anyway, here's an experimental build for win32 http://forums.mozillazine.org/viewtopic.php?t=89512 However it seems I got RSS auto discovery bugs while switching tabs... And some RDF could not be parsed in this build...
Assignee | ||
Comment 14•20 years ago
|
||
Thanks for the build! I'm behind a very slow net atm but I'll make a linux build next week as well. I'm finishing up an updated patch that fixes some of the tab switching issues; however, can you point me to some RSS that doesn't parse? There hasn't been extensive testing of the parsers and I'd appreciate some examples of where it breaks.
Comment 15•20 years ago
|
||
(In reply to comment #14) > Thanks for the build! I'm behind a very slow net atm but I'll make a linux > build next week as well. I'm finishing up an updated patch that fixes some of > the tab switching issues; however, can you point me to some RSS that doesn't > parse? There hasn't been extensive testing of the parsers and I'd appreciate > some examples of where it breaks. I'd tried to group all the feed issues I've encountered. The first one is the RSS 0.9 format feed. e.g. http://www.mozilla.org/news.rdf THe second one is the RSS 1.0 format feed with UTF-8 encoding and Unicode signature(BOM) on the front. The third one is the non iso-8859-1 / UTF-8 format feeds. The encodings I've tried, like Shift-JIS, Big5, doesn't work. e.g. http://white.sakura.ne.jp/~piro/xul/xul.rdf And some of the sites are parsed incorrectly. The encoding of these feeds is UTF-8 but without BOM. ref. http://blog.elixus.org/jedi/index.rdf Here's all the issues I've faced. And I'd like to ask for enhancement about... 1. Report if the feed is mal-formed. So far the livemarks just opened no items and it's hard to persuade end-users that it's not a bug IMO. 2. Load the auto-discovery livemarks icon when the page loads. (like Alt stylesheet switcher on the left of statusbar) If we visit some of the sites like http://weblogs.mozillazine.org/asa/ , the Alt stylesheet switcher would show before the page is fully loaded. But it's not in the livemarks icon. I think it can be done with the similar mechanism from Alt stylesheet switcher to show the icon before it's loaded. 3. considering recognize some of the common feeds linked from HTML. e.g. http://www.mozilla.org/news.rdf It links the feed(.xml/.rss/.rdf) with MIME application/xml. e.g. http://weblogs.mozillazine.org/asa/ It links the atom feed with MIME application/x.atom+xml(this one is strange though) The current implementation does not recognize the first one which is no quite useful IMO. I hope the auto-discovery features would support it since it's already widely used. I hope my comments would be considered. Thanks, vlad. Livemarks spport is really a great work!
Comment 16•20 years ago
|
||
just to add to this, the RDF parser just drops xml parsing errors alltogether right now
Assignee | ||
Comment 17•20 years ago
|
||
(In reply to comment #15) > I'd tried to group all the feed issues I've encountered. > > The first one is the RSS 0.9 format feed. > e.g. http://www.mozilla.org/news.rdf This one should be fixed in this patch. The other two I'll take a look at now -- most likely some utf8/unicode issue that i'm not handling correctly. > Here's all the issues I've faced. > And I'd like to ask for enhancement about... > > 1. Report if the feed is mal-formed. So far the livemarks just opened no items > and it's hard to persuade end-users that it's not a bug IMO. I agree. I guess I can create a fake bookmark entry in this case with a descriptive title, with an invalid URL (or a URL pointing to a mozilla.org "This is a livemark that failed to load." page or something). The real fix is related to bug 248015 and bug 192624. > 2. Load the auto-discovery livemarks icon when the page loads. (like Alt > stylesheet switcher on the left of statusbar) > If we visit some of the sites like http://weblogs.mozillazine.org/asa/ , the Alt > stylesheet switcher would show before the page is fully loaded. But it's not in > the livemarks icon. I think it can be done with the similar mechanism from Alt > stylesheet switcher to show the icon before it's loaded. I noticed this problem as well.. in theory, I think I'm using the same mechanism as the stylesheet switcher, but you're right, the behaviour is different. Need to figure out why... > 3. considering recognize some of the common feeds linked from HTML. > e.g. http://www.mozilla.org/news.rdf > It links the feed(.xml/.rss/.rdf) with MIME application/xml. Should be done now (can't accept application/xml, but we can look at title="RSS") > e.g. http://weblogs.mozillazine.org/asa/ > It links the atom feed with MIME application/x.atom+xml(this one is strange though) Added application/x.atom+xml as well. > The current implementation does not recognize the first one which is no quite > useful IMO. I hope the auto-discovery features would support it since it's > already widely used. Yep, the only real way to get this working is to get a bunch of existing use cases and then handle those; the lack of any real standard (or even a small set of standards) for this stuff sucks in this case. > I hope my comments would be considered. Thanks, vlad. > Livemarks support is really a great work! Thanks, and thanks for the comments!
Assignee | ||
Updated•20 years ago
|
Attachment #151015 -
Attachment is obsolete: true
Assignee | ||
Updated•20 years ago
|
Attachment #151015 -
Flags: review?(bugs)
Comment 18•20 years ago
|
||
(In reply to comment #17) > Should be done now (can't accept application/xml, but we can look at title="RSS") Does it mean the auto-discovery icon only appeared when it has exactly the string title="RSS" with MIME application/xml? What about if the title named "RSS0.91 Feed" or "RSS 2.0 Feed" or the like? > Added application/x.atom+xml as well. Ah, I just noticed that application/x.atom+xml is the new MIME from the draft of Atom API. ref. http://www.atomenabled.org/developers/api/atom-api-spec.php > Thanks, and thanks for the comments! You're welcome. I just hope the feature would do better and better. :)
Comment 19•20 years ago
|
||
Vladimir, I just wanted to tell you (and other RSS interested people) that there is an RSS extension for Firefox called Sage http://sage.mozdev.org/ They also have a wiki http://wiki.mozdev.org:8080/cgi-bin/mozdev-wiki.pl?ProjectSage with a few feature requests and comments. Perhaps you can use some of them in your Live Bookmarks. I'm looking forward to test the new patch.
Assignee | ||
Comment 20•20 years ago
|
||
This patch fixes the encoding parsing issues; should be able to parse xml files in any valid encoding that mozilla recognizes. (Whether you see the right characters in the menus is up to your UI font, however.) This patch requires the additional livemark-parsestring patch.
Attachment #151691 -
Attachment is obsolete: true
Assignee | ||
Comment 21•20 years ago
|
||
Changes to RDFXMLParser's parseString (pre-patch it wanted UTF8 in a wide nsString). Also adds a UTF8 parsing interface to nsIDOMParser, but I think I can do without this. (This patch should probably be its own bug.)
Assignee | ||
Comment 22•20 years ago
|
||
It's not fun to realize just as you're about to drift off to sleep at 2am that your last proposed patch broke the bookmarks toolbar. (The bookmarks-template can't be split up; the parent="hbox", as fragile as it is, needs to stay so that the same template can be used to recrusively build submenu content.)
Attachment #151716 -
Attachment is obsolete: true
Reporter | ||
Comment 23•20 years ago
|
||
Comment on attachment 151786 [details] [diff] [review] livemark-6.patch >+ var head = _content.document.getElementsByTagName("head"); >+ if (head == null || head.size == 0) do you mean |.length|? >+ livemarkLinks.push([links[i].href, links[i].type, links[i].title]); why not do: livemarkLinks.push({ href: links[i].href, type: links[i].type, title: links[i].title }); so that later you can do: menuItem.setAttribute("label", markinfo.title || markinfo.href); instead of: >+ menuItem.setAttribute("label", markinfo[2] ? markinfo[2] : markinfo[0]); ... makes the code easier to follow. Learned that lesson with the arguments parameter of the addBookmark dialog, which as you saw is somewhat unwieldy and which some day will be rewritten to be a js obj with named parameters. >+ if (livemarkLinks.length > 0) { >+ _content.livemarkLinks = livemarkLinks; ... >+ var livemarkLinks = _content.livemarkLinks; ... etc. _content is the web page's js obj, right? Seems a little risky to stash this variable there... why not set it on the <browser> tag and not interfere with the page's "namespace"? >+<!ENTITY livemarkIcon.tooltip "Create a livemark for this page"> This might be the right name to expose this feature to the user as, if it is, it's a word we made up so it should be capitalized. >+ case "Livemark": >+ commands = ["bm_newfolder", "bm_separator", >+ "cut", "copy", "paste", "bm_separator", >+ "delete", "bm_separator", >+ "bm_refreshlivemark", "bm_separator", >+ "bm_properties"]; >+ break; >+ case "LivemarkBookmark": >+ commands = ["bm_open", "bm_openinnewwindow", "bm_openinnewtab", "bm_separator", From this, I infer that Livemark = the actual editable bookmark item, and that LivemarkBookmark = an entry in the immutable child list. > case "cmd_bm_properties": > case "cmd_bm_rename": >- return length == 1; >+ if (length != 1 || BookmarksUtils.resolveType(aSelection.parent[0]) == 'Livemark') Match quote style, please ("") >+ if (length != 1 || type0 == 'Livemark') ditto >+ var ptype = BookmarksUtils.resolveType(parent); >+ if (ptype == 'Livemark') ditto >+ <menuitem label="&command.refreshLivemark.label;" >+ command="cmd_bm_refreshlivemark"/> >+ <menuseparator/> So I think these items should not be visible for non-livemark bookmarks, to avoid cluttering the UI - can you add code to _hide_ these menuitems in that case? >+ RDF.GetResource(NC_NS+"RSSURL")]; Shouldn't this property be called "FeedURL" since you now support more formats than RSS? >+ document.getElementById("rssurlrow").hidden = true; terminology... >+ <row id="rssurlrow" align="center"> >+ <label value="&bookmarks.rssurl.label;" control="rssurl"/> >+ <textbox id="rssurl" /> > </row> ... terminology ... >+<!ENTITY menuitem.newLivemark.label "New Livemark..."> >+<!ENTITY menuitem.newLivemark.accesskey "L"> > <!ENTITY menuitem.newFolder.label "New Folder..."> > <!ENTITY menuitem.newFolder.accesskey "F"> "While you're here" can you change "F" for New Folder's access key to "o" or anything else that's available? It'll fix another bug for me ;-) >+<!ENTITY bookmarks.rssurl.label "RSS Location:"> ... terminology ... >+nsIRDFResource *kNC_RSSURL; ... terminology ... More comments later.
Assignee | ||
Comment 24•20 years ago
|
||
I added some dummy items here that appear while the Livemark is loading, and also if the Livemark failed to load. They point to "about:livemark-loading" (or similar) for now, which doesn't exist. These items would best be disabled, but I'm not sure how best to do this without adding in a disabled rdf arc on the bookmark, which seems excessive just for this. This patch also fixes tab handling business; the livemark icon will get switched correctly with tab switches, etc. Also fixes RSS 0.9 handling (mozilla.org's news.rdf). (In reply to comment #23) > do you mean |.length|? Yes. Yes I do. > >+ livemarkLinks.push([links[i].href, links[i].type, links[i].title]); > why not do: > [...] done. > _content is the web page's js obj, right? Seems a little risky to stash this > variable there... why not set it on the <browser> tag and not interfere with > the page's "namespace"? I misunderstood what _content was; it's now on gBrowser.mCurrentBrowser. Is that the right place for it? > >+<!ENTITY livemarkIcon.tooltip "Create a livemark for this page"> > > This might be the right name to expose this feature to the user as, if it is, > it's a word we made up so it should be capitalized. done. > >+ case "Livemark": > >+ commands = ["bm_newfolder", "bm_separator", > >+ "cut", "copy", "paste", "bm_separator", > >+ "delete", "bm_separator", > >+ "bm_refreshlivemark", "bm_separator", > >+ "bm_properties"]; > >+ break; > >+ case "LivemarkBookmark": > >+ commands = ["bm_open", "bm_openinnewwindow", "bm_openinnewtab", "bm_separator", > > From this, I infer that Livemark = the actual editable bookmark item, and that > LivemarkBookmark = an entry in the immutable child list. It is, though LivemarkBookmark only exists within the context of this function. Everywhere else it's treated as a normal bookmark -- I added this special case here because otherwise you'd get a normal bookmarks menu with half the entries disabled. > Match quote style, please ("") whoops, done. > >+ <menuitem label="&command.refreshLivemark.label;" > >+ command="cmd_bm_refreshlivemark"/> > >+ <menuseparator/> > > So I think these items should not be visible for non-livemark bookmarks, to > avoid cluttering the UI - can you add code to _hide_ these menuitems in that > case? Refresh appears in two places -- in the right-click menu for livemarks, and in the bookmarks manager Edit menu. The right-click context menu bit is hidden for non-livemarks. I can hide the Edit menu entry (with potentially some pain, I think), but I figured that hiding/showing items in menus in the menu bar instead of disabling them was bad UI practice? > >+ RDF.GetResource(NC_NS+"RSSURL")]; > > Shouldn't this property be called "FeedURL" since you now support more formats > than RSS? done (everywhere). > >+<!ENTITY menuitem.newLivemark.label "New Livemark..."> > >+<!ENTITY menuitem.newLivemark.accesskey "L"> > > <!ENTITY menuitem.newFolder.label "New Folder..."> > > <!ENTITY menuitem.newFolder.accesskey "F"> > > "While you're here" can you change "F" for New Folder's access key to "o" or > anything else that's available? It'll fix another bug for me ;-) done :)
Assignee | ||
Updated•20 years ago
|
Attachment #151786 -
Attachment is obsolete: true
Comment 25•20 years ago
|
||
(In reply to comment #13) > I don't see this feature in the trunk build. > > Anyway, here's an experimental build for win32 > http://forums.mozillazine.org/viewtopic.php?t=89512 > > However it seems I got RSS auto discovery bugs while switching tabs... > And some RDF could not be parsed in this build... I was thinking it should just be added to the trunk, and then it will be tested there. Is that not what trunk is for?
Comment 26•20 years ago
|
||
(In reply to comment #25) > I was thinking it should just be added to the trunk, and then it will be tested > there. Is that not what trunk is for? I second that. Since the trunk is no longer considered as stable as the aviary branch. Perhaps some of the major features before 1.0 could be checked in the trunk in order to have a test for everyone. After the "Feature Complete" Firefox 0.9, it's the most impressive feature I've seen. And it needs localization that I think it should be in before Firefox 1.0 beta is out. I really hope it could be checked in as soon as possible and test it since I can't build. Now that Opera has newfeed support, Safari RSS has a well built-in feed pages support, Firefox should do so. vlad, is it possible to check it in the trunk first so that more people could test this feature?
Assignee | ||
Comment 27•20 years ago
|
||
(In reply to comment #26) > vlad, is it possible to check it in the trunk first so that more people could > test this feature? In theory it should be; I'm not sure how far trunk and aviary have diverged, and I know ben has been working on getting stuff back in to trunk from the branch. I need to create a new patch for the locale shuffle that just happened as well. However, where it gets committed and when is up to ben.
Comment 28•20 years ago
|
||
Heh. Nobody messes with any tree here. If you wanna test stuff, create a branch. The trunk has tree rules, and no aviary or something is changing that. Please read the stuff beneath http://www.mozilla.org/hacking/ before suggesting stuff like this.
Comment 29•20 years ago
|
||
well, if it wasn't depending on changes outside of /browser to work, we could do whatever we want. The tree rules on /browser /toolkit and /chrome are different, remember.
Reporter | ||
Updated•20 years ago
|
Flags: blocking-aviary1.0RC1+
Assignee | ||
Comment 30•20 years ago
|
||
I've got things updated in my local tree for the locale reorg, but i'll wait for a new patch to make any review changes necessary. Asa pointed out: http://blogs.lns.kicks-ass.net/moonjihad/moonjihad-browser.xul which is a blog-specific feed reader sidebar -- we could do something like that and add a dropdown menu at the top to let you select amongst already bookmarked feeds and display them in the sidebar, though that's moving into the realm of a generic feed reader, and I don't think we want to do that yet. Should we let people bookmark things as livemarks from a right-click bookmark menu on a link? (For people who have links to their rss feeds... but I assume that those people will also have <link> entries.) It would also make sense to share livemarks with tbird and the feedzilla stuff..
Reporter | ||
Comment 31•20 years ago
|
||
Comment on attachment 151851 [details] [diff] [review] livemark-7.patch OK - I really only have a nit - can I get you to use prefixes for your function parameters consistently (e.g. aFoo) - it makes your code much easier to follow. Aside from that I think we're ready now to start seeing testing! r=ben@mozilla.org
Attachment #151851 -
Flags: review+
Comment on attachment 151766 [details] [diff] [review] livemark-parsestring-0.patch This looks fine, and like something we need. Let's get jst with the r=, and we can go for it.
Attachment #151766 -
Flags: superreview+
Attachment #151766 -
Flags: review?(jst)
Comment 33•20 years ago
|
||
Comment on attachment 151766 [details] [diff] [review] livemark-parsestring-0.patch - In extensions/xmlextras/base/public/nsIDOMParser.idl: + * The UTF8 string passed in is parsed into a DOM document. + * + * @param str The UTF8 string to be parsed + * @param contentType The content type of the string - either text/xml + * or text/html + * @returns The DOM document created as a result of parsing the + * string + */ + nsIDOMDocument parseFromUTF8String(in string str, in string contentType); Any call to this method through XPConnect will get the high-bit stripped off for every character in the conversion from JS strings to char* data, and that's not cool here. Make this take AUTF8String arguments, for both str and contentType (to avoid unicode strings stripped down to ASCII from accidentally matching the wrong content type etc). - In nsDOMParser::ParseFromUTF8String(): + char *charBuf = nsCRT::strdup(str); + + // The new stream takes ownership of the buffer + nsresult rv = NS_NewByteArrayInputStream(getter_AddRefs(baiStream), charBuf, charLength); + if (NS_FAILED(rv)) { + *_retval = nsnull; + return rv; Won't we leak charBuf here? Creation of the new stream failed, so noone took ownership of the data. Free it here. r=jst with those changes.
Attachment #151766 -
Flags: review?(jst) → review+
Assignee | ||
Comment 34•20 years ago
|
||
in on aviary, go forth and find ye bugs... (other than opening shaver's blog in a separate tab, for some reason the livemark icon doesn't appear until you switch to another tab and back -- other sites work fine)
Comment 35•20 years ago
|
||
You also checked in the favicon changes. :( If Firefox doesn't crash right upon startup, it freezes sooner or later after I open a folder on the bookmarks sidebar. It tries to load all the favicons, but some fail and display no icon at all. If I then exit Firefox, its process is still running. I have to kill it in the task manager.
Assignee | ||
Comment 36•20 years ago
|
||
Which favicon changes? I don't see anything favicon-related in my checkins in lxr, and I'm pretty sure I sanitized the patch before committing (it went in via a separate clean tree).
Assignee | ||
Comment 37•20 years ago
|
||
There's a few other folks experiencing bookmarks-related crashes, though I can't reproduce them locally... can someone who's crashing get a stack trace, or a detailed set of steps to reproduce? FWIW, I'm doing my local testing on FC2/gtk2, though if need be I can also build on win32.
Comment 38•20 years ago
|
||
favicon changes: If you open a folder on the bookmarks sidebar, all the favicons are being loaded. That's new since your checkin. But that's not the reason for the crashes. I don't have any problems without livemarks in bookmarks. To get rid of the feedurls, I simply need to start an older build and close it. It "cleans up" the bookmarks.html. I'm working on a testcase (bookmarks.html).
Comment 39•20 years ago
|
||
1. Create a new profile, exit Firefox. 2. Copy this bookmarks.html over the default one. 3. Start Firefox. --> Sometimes Firefox crashes here. 4. Make the bookmarks sidebar visible, expand one of the livemark folders. --> Sometimes Firefox crashes here, the only child visible is "Livemark loading..." 5. If there's still no crash and the livemarks load, click on one of them. --> Network operations slow down immensly, the site never finishes loading. Open a bunch of them in a new tab and switch tabs. Tab switching takes ages (a couple of seconds).
Comment 40•20 years ago
|
||
Using WinXP here: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040708 Firefox/0.9.0+ (Steffen)
Assignee | ||
Comment 41•20 years ago
|
||
Thanks for the bookmarks.html.. was a problem in my parsing of simple rss feeds, specifically Atom feeds. This should fix both the crash and atom feed parsing.
Comment on attachment 152575 [details] [diff] [review] livemark-fix-0.patch >+// find the first node below aParentNode that is a TEXT_NODE, >+// and return it >+// >+nsresult >+nsFeedLoadListener::FindTextNode (nsIDOMNode *aParentNode, nsIDOMNode **aTextNode) >+{ >+ NS_ENSURE_ARG(aParentNode); >+ NS_ENSURE_ARG(aTextNode); >+ >+ nsresult rv; >+ >+ *aTextNode = nsnull; >+ >+ nsCOMPtr<nsIDOMNode> childNode; >+ rv = aParentNode->GetFirstChild(getter_AddRefs(childNode)); >+ if (!childNode || NS_FAILED(rv)) return NS_ERROR_FAILURE; This will replace a possibly-useful error with NS_ERROR_COULD_BE_ANYTHING. Poor form, IMO. For !childNode, do you really want to signal failure? You can return NS_OK from this function below if you don't find a text node anywhere; seems inconsistent. >+ PRUint16 nodeType = 0; >+ while (childNode) { >+ rv = childNode->GetNodeType(&nodeType); >+ if (nodeType == nsIDOMNode::TEXT_NODE) >+ break; >+ >+ rv = childNode->GetNextSibling(getter_AddRefs(childNode)); >+ if (NS_FAILED(rv)) return rv; >+ } do { ... } while(); is preferable where you know that condition will be true the first time. If you just bail on failure above, though, then this will work well (and make the handling of non-found consistent). The BM code looks to be a place where NS_ENSURE_SUCCESS(rv, rv) is preferred over the single-line if (to which it expands). I'm not a fan, but when-in-Rome trumps even my superior taste. >+ if (nodeType == nsIDOMNode::TEXT_NODE) { >+ *aTextNode = childNode.get(); >+ NS_IF_ADDREF(*aTextNode); No need for _IF_ here -- you made it non-null above. >- if (NS_FAILED(rv)) return rv; >+ if (!node || NS_FAILED(rv)) return NS_ERROR_FAILURE; if (!node) return NS_ERROR_UNEXPECTED; NS_ENSURE_SUCCESS(rv, rv); With that stuff fixed up, r=shaver.
Attachment #152575 -
Flags: review+
Assignee | ||
Comment 43•20 years ago
|
||
Fix in on aviary. Steffen, can you verify that it fixes your crash and other problems? The favicon/network wierdess still sounds fishy.
Comment 44•20 years ago
|
||
Thanks, neither my testcase nor any other feed I watch crashes or freezes Firefox anymore. A few thoughts: I'd like to be able to open the web site the feed syndicates. Maybe you can add a context menu entry for the livemark folder? The livemark properties dialog should then contain the web site address in addition to the feed address. Some sites, the most prominent being MozillaZine, don't offer the link to their feeds in the right way, so that the "flash" icon does not appear. If I want to add such a feed I have to right-click, select Copy Link Location, click "New Livemark" in the bookmars manager, enter a name and paste the url. It would be easier if I could drag'n'drop the feed link onto the "New Livemark" button. It should open the dialog, having the url already filled in. Maybe it could also take the name for the new livemark from the title of the page containing the link. See bug 232399 for my proposal to make the bookmarks toolbarbutton a drop target.
Comment 45•20 years ago
|
||
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040708 Firefox/0.9.0+ When I try to livemark (is that a verb?) the Atom feed at http://weblogs.mozillazine.org/asa/ I get a crash (the build is from *after* the fix patch was checked in). Also, if you hold down ctrl (as you would with a real folder) to drag a livemark folder around the personal toolbar folder it is actually copied instead. p.s. I'm also seeing the favicons thing, it didn't happen with yesterday's build, but if you open a bookmark folder in today's build Firefox starts fetching /favicon.ico from the bookmarked sites.
Comment 46•20 years ago
|
||
One further bug, it is possible to move a bookmark into a Livemark folder (after which it can't be moved out again) using the Move command in Bookmarks Manager.
Comment 47•20 years ago
|
||
Firefox crashes when I add the following RSS feed http://linuxfr.org/backend.rss After the first crash, with the feed in the bookmark.html, it crashes when I try to open the bookmark window (Bookmarks|Manage Bookmarks...) Talkback: TB255436Y TB255432X TB255413X TB255406G TB255405K (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040708 Firefox/0.9.0+)
Assignee | ||
Comment 48•20 years ago
|
||
Gah. This patch fixes the issue in comment #46. As for the crashes... I can reproduce the crash with the nightly build from http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2004-07-08-11-0.9/ but I can't reproduce it using my local build (from a clean tree, both debug and non-debug/optimized variants, also from my local dev tree). I went through and added every feed from mozillazine as a livemark -- http://rig.vlad1.com/~vladimir/junk/feed_bookmarks.html for those interested -- and wasn't able to get a crash even through several quit/restart cycles with the sidebar open. Can you guys who were crashing try with tonight's nightly, just in case the patch didn't make it into yesterday's? If you're building from source, "grep FindTextNode mozilla/browser/components/bookmarks/src/nsBookmarksFeedHandler.cpp" will return no hits if the patch didn't make it. There are a few feeds that fail to load/parse (blizzard's, a few others), and there's also an issue with dragging a livemark to a different folder that I need to track down; also, the livemark icon not appearing until you switched tabs got very old, so that'll get squashed soon.
Comment 49•20 years ago
|
||
(In reply to comment #48) > If you're building > from source, "grep FindTextNode > mozilla/browser/components/bookmarks/src/nsBookmarksFeedHandler.cpp" will > return no hits if the patch didn't make it. Results of grep FindTextNode mozilla/browser/components/bookmarks/src/nsBookmarksFeedHandler.cpp: NS_METHOD FindTextNode (nsIDOMNode *aParentNode, nsIDOMNode **aTextNode); nsFeedLoadListener::FindTextNode (nsIDOMNode *aParentNode, nsIDOMNode **aTextNode) rv = FindTextNode (childNode, getter_AddRefs(textNode)); rv = FindTextNode (childNode, getter_AddRefs(textNode));
Assignee | ||
Comment 50•20 years ago
|
||
Okay, sanity check (mine, mainly, I think): http://rig.vlad1.com/~vladimir/firefox-20040709-i686-linux-gtk2+xft-vlad.tar.bz2 is a dist from my clean cvs build, optimized. Could someone who's able to reproduce the crash on linux try that build, and see if they crash? I've built on two FC2 systems straight from CVS and can't reproduce it, and it's driving me a little crazy :)
Comment 51•20 years ago
|
||
With today's build, it doesn't crash when adding the following livemark but I can't exit Firefox. The windows all close but the process stays in memory. However, when I restart Firefox then, opening the "Manage Bookmark" window crashes Firefox. If the livemark is at the root of the bookmark tree, Firefox crashes when opening the Bookmark menu. Opening the bookmark bar doesn't crash but I can't exit Firefox (same as when adding the livemark) Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040709 Firefox/0.9.0+ (from http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2004-07-09-09-0.9/) http://linuxfr.org/backend.rss Talkback: TB261154Z, TB261108Q
Comment 52•20 years ago
|
||
*** Bug 240393 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 53•20 years ago
|
||
Finally was able to track down and reproduce the crash bug. This should fix it (was caused by bad nsCOMPtr usage that only became visible with a certain set of optimization flags =/).
Comment on attachment 152737 [details] [diff] [review] livemark-fix-2.patch Much better; sorry I didn't catch those on initial review. r=shaver for immediate aviary insertion.
Attachment #152737 -
Flags: review+
Comment on attachment 152644 [details] [diff] [review] livemark-fix-1.patch r=shaver.
Attachment #152644 -
Flags: review+
I encourage you to mark this FIXED at your earliest convenience, and encourage bug reporters to open new bugs for crashes, parsing infelicities, or otherwise-unexplained rashes and hives.
Assignee | ||
Comment 57•20 years ago
|
||
Crash fix is in on aviary. This patch fixes the link handling for displayling the livemark button in the status bar; it should be much more robust. If these issues are verified fixed after tomorrow's nightly, we should probably resolve this bug and start taking individual bugs against livemarks.
Comment on attachment 152744 [details] [diff] [review] livemark-fix-3.patch Goforit. r=shaver
Attachment #152744 -
Flags: review+
Comment 59•20 years ago
|
||
(In reply to comment #20) > Created an attachment (id=151716) > livemark-5.patch > > This patch fixes the encoding parsing issues; should be able to parse xml files > in any valid encoding that mozilla recognizes. (Whether you see the right > characters in the menus is up to your UI font, however.) > > This patch requires the additional livemark-parsestring patch. What do you mean by UI font? I've tested the win32 branch build in these few days, but I found the title of the feed items did not displayed correctly in non UTF-8 feeds.
Assignee | ||
Comment 60•20 years ago
|
||
In, and in. Resolving, please verify if the crashing issues are fixed with tomorrow's (er, today's) builds. Re comment #59: can you file a separate bug for the non-utf8 issue, and Cc vladimir@pobox.com on it? I'll take a look and see what's going on.
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Comment 61•20 years ago
|
||
I confirm that the crash for "http://linuxfr.org/backend.rss" is fixed with "Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040710 Firefox/0.9.0+"
Updated•20 years ago
|
Keywords: helpwanted
Summary: Live Bookmarks (RSS feed integration) → Live Bookmarks/Livemark (RSS feed integration)
Comment 62•20 years ago
|
||
(In reply to comment #27) > (In reply to comment #26) > > vlad, is it possible to check it in the trunk first so that more people could > > test this feature? > > In theory it should be; I'm not sure how far trunk and aviary have diverged, and > I know ben has been working on getting stuff back in to trunk from the branch. > I need to create a new patch for the locale shuffle that just happened as well. > However, where it gets committed and when is up to ben. Boy, it was a while since I looked at this bug. A lot of nightly branch crashes could have been avoided by putting into trunk, no?
Comment 63•20 years ago
|
||
brendan, shaver: Are toolkit apps expected to not abide by bug 18352?
Do you have a specific dependency concern? Citing a specific issue, rather than tossing out generalizations, is likely to be a more productive way of resolving whatever concrete problem is plaguing you. In some cases things that were previously optional become core, and in others we interpose different layering to make a too-hard dependency softer. And in some cases we don't sweat either issue as we get close to a release, where the cost of tree-juggling outweighs the gain from better modularity purity.
Comment 65•20 years ago
|
||
(In reply to comment #64) > Do you have a specific dependency concern? Citing a specific issue, rather than > tossing out generalizations, is likely to be a more productive way of resolving > whatever concrete problem is plaguing you. /bookmarks/src/nsBookmarksFeedHandler.cpp uses nsIDOMParser which adds an xmlextras dependency to bookmarks.
Assignee | ||
Comment 66•20 years ago
|
||
(In reply to comment #65) > > /bookmarks/src/nsBookmarksFeedHandler.cpp uses nsIDOMParser which adds an > xmlextras dependency to bookmarks. The alternative was to reimplement (or more likely copy/paste most of) nsDOMParser. This didn't seem sane. Going forward it would make sense to create a rss module shared by firefox and thunderbird. Such a thing would itself depend on nsDOMParser, so if that's a big issue, then perhaps nsDOMParser and nsDOMSerializer should be moved into a more core module.
As I understand it, xmlextras are going to become non-optional for toolkit apps, or indeed are already, since we use them in the extension manager; there's no way to build firefox -- the only bookmarks-using toolkit app I know of -- without xmlextras anyway, so I'm not going to get worked up about it. (And if you've read the bug you cite, you know that I can get _quite_ worked up about extension dependencies.) I don't know what the appropriately pure solution is for something that's an extension to seamonkey, but a core component of toolkit apps. If CVS didn't make moving stuff around such a colossal pain in the ass, I might approve of toolkit/xmlextras. As seamonkey recedes into tier-1.5 status, that might be OK. If there's some use case I'm missing, in which disabling xmlextras is desirable for an app that uses the bookmarks service, I'd be interested to hear what solution you suggest. Duplicating nsDOMParser does indeed sound like optimizing for the wrong case, though the case is only hypothetical thus far, but maybe we move the interface definition into somewhere more core. Is this a runtime dep, or only a build-time one?
Assignee | ||
Comment 68•20 years ago
|
||
(In reply to comment #67) > Is this a runtime dep, or only a build-time one? Build-time only; the feed types that depend on DOMParser will just turn into feeds that can't be parsed if I can't create a domparser instance.
Comment 69•20 years ago
|
||
Any chance we can get a right click menu option, "bookmark this link" when we click on the live bookmark's children? I know it's an easy workaround to just load the page and then bookmark it, but it would be nice to have a bookmark option!
Comment 70•20 years ago
|
||
setting fixed-aviary1.0 for bugfixes checked into branch, for searching purposes. sorry for bugspam.
Keywords: fixed-aviary1.0
Comment 71•18 years ago
|
||
sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change
QA Contact: mconnor → bookmarks
You need to log in
before you can comment on or make changes to this bug.
Description
•