Closed Bug 244078 (livemark) Opened 20 years ago Closed 20 years ago

Live Bookmarks/Livemark (RSS feed integration)

Categories

(Firefox :: Bookmarks & History, defect, P3)

defect

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
Status: NEW → ASSIGNED
Flags: blocking1.0+
Priority: -- → P3
Target Milestone: --- → Firefox1.0
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
If we're going to do this, we should probably add Atom 0.3 XML support too. It's
widely used amongst blogging communities.
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 
-> vlad, who has been working on a patch. 
Assignee: bugs → vladimir
Status: ASSIGNED → NEW
Attached patch proposed patch (obsolete) — Splinter Review
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 :)
Attached patch proposed patch (obsolete) — Splinter Review
My "clean" tree I was diffing against wasn't so clean.	New patch attached.
Attached patch proposed patch (obsolete) — Splinter Review
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
Status: NEW → ASSIGNED
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).
vlad,
is there any experimental build to test this feature?
(In reply to comment #11)
> vlad,
> is there any experimental build to test this feature?

Trunk?
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...
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.
(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!
just to add to this, the RDF parser just drops xml parsing errors alltogether
right now
Attached patch livemark-4.patch (obsolete) — Splinter Review
(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!
(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. :)
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.
Attached patch livemark-5.patch (obsolete) — Splinter Review
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
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.)
Attached patch livemark-6.patch (obsolete) — Splinter Review
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
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.
Attached patch livemark-7.patchSplinter Review
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 :)
(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?
(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?
(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.
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.
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.
Flags: blocking-aviary1.0RC1+
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..
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 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+
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)
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.
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).
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.
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).
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).
Using WinXP here: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7)
Gecko/20040708 Firefox/0.9.0+ (Steffen)
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+
Fix in on aviary.  Steffen, can you verify that it fixes your crash and other
problems?  The favicon/network wierdess still sounds fishy.
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.
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.
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.
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+)
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.
(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));
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 :)
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
*** Bug 240393 has been marked as a duplicate of this bug. ***
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+
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.
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+
(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.
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
Keywords: relnote
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+"
Keywords: helpwanted
Summary: Live Bookmarks (RSS feed integration) → Live Bookmarks/Livemark (RSS feed integration)
(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?
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.
(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.
(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?
(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.
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!
setting fixed-aviary1.0 for bugfixes checked into branch, for searching purposes.
sorry for bugspam.
Keywords: fixed-aviary1.0
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.