Closed Bug 303567 Opened 19 years ago Closed 13 years ago

In live bookmarks, entries not having a <link> element should use the <link> child of root

Categories

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

enhancement

Tracking

()

VERIFIED FIXED
Firefox 7

People

(Reporter: frnchfrgg, Assigned: mak)

References

()

Details

(Keywords: dataloss, Whiteboard: [fixed-in-places])

Attachments

(2 files, 3 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.7.8) Gecko/20050517 Firefox/1.0.4 (Debian package 1.0.4-2)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.7.8) Gecko/20050517 Firefox/1.0.4 (Debian package 1.0.4-2)

In a live bookmark folder, when an entry of the RSS has no <link> (which is
permitted by the spec), Firefox should use the (mandatory) <link> at root level.

This would allow for a "default target" in RSS, which is especially useful when
several news are related to the same page (but not all, since bookmarks wouldn't
be meaningful then). For example, when a RSS feed lists all changes to a site,
some news refer to a specific page, but several aren't bound to a specific
location, and thus are to be linked with the home page.

Currently, entries lacking a <link> aren't displayed, and if none has a <link>
the message "the feed failed to load" is displayed.

Reproducible: Always

Steps to Reproduce:
Add "http://www.komite.net/pl/hidden/avec.rss" and
"http://www.komite.net/pl/hidden/sans.rss" to the live bookmarks.
The first has five entries, each with a <link>.
The second has those same five entries, but two of them lacking their <link>.

Go and try to open the corresponding folders.
Actual Results:  
The second live bookmark has only three visible items, and the first has five.

Expected Results:  
The second live bookmark should have had five visible items, two of them
pointing towards the default <link> in the RSS.

The current behaviour has been seen on Linux Mozilla 1.0.4, Windows (1.0.6), and
Linux (Deerpark alpha 2).
Sorry if it is a duplicate, I did dig a lot, but I am prone to error.
*** Bug 303602 has been marked as a duplicate of this bug. ***
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b4) Gecko/20050805
Firefox/1.0+ ID:2005080510

confirmed ->New
Status: UNCONFIRMED → NEW
Ever confirmed: true
*** Bug 304667 has been marked as a duplicate of this bug. ***
I tried to look at the code at ttp://lxr.mozilla.org/mozilla/source/browser/components/bookmarks/src/nsBookmarksFeedHandler.cpp 

If in HandleRDFItem (line 505), instead of returning when either title or link isn't a litteral (line 527), the code just requires title to be a litteral and fetches the global link from the arg 'aLinkResource' (if I am not mistaken, it is the RSS_link elt passed to HandleRDFItem from TryParseAsRDF, which seems to be the global link elt), would it work (for the RDF case, that is) ?

For the non RDF case, it seems that the <link> elt is blindly skipped by the code that positions itself at the right place to start the processing of the <item>s, but provided some parsing is added, just using the global link when linkStr is empty at line 726 should do the trick... ?

Am I awfully off-track ?
First, I was totally off-track concerning the RDF part of my last post.

Second, in the Atom specification, the author is mandatory for the feed element if and only if there exists a child not having any author, but no similar system is available for the link.

Nevertheless, Firefox could do the following :
a) try to use the entry's link
b) revert if necessary to root link, if it exists
c) return the same error as now if a & b fail (b shouldn't ever fail in RSS2)
As I see it, the only fool proof solution would be to integrate some full-scale aggregator features to the browser instead of mere bookmarks. As it stands now, people will continue to create and subscribe to feeds that don't link to individual entries (which is perfectly acceptable), and then get confused when all they get is a vague error message.

If Firefox was smart, it could offer the user to download an extension, say Sage, should this occur. Since Firefox can't show the actual content of the feed (without dereferencing links), it can't really handle the feed. Using the "root link" gives unpredictable results, since its exact meaning isn't defined in the specifications. At least the users should be presented with a useful notice instead of the "failed to load" error.
*** Bug 354390 has been marked as a duplicate of this bug. ***
since the previous links posted for reproduction seem not to exist any more, here's a file that has several RSS items without links and one that has a link element.  If you use Live Bookmarks to subscribe to this feed you get just one item, not the 30 that exist in the feed.
The feeds in Steps to Reproduce don't exist any more - I attached a file that displays both with link and without link behavior
I agree with this idea. I think we should link to the host of the feed if there's no link element in the feed itself (even though it is required).
Assignee: nobody → sayrer
Keywords: dataloss
Target Milestone: --- → Firefox 3
I don't think it is required to have a link in every item of the feed at least for RSS2; AFAICT such feeds validate.
Component: Bookmarks → Places
QA Contact: bookmarks → places
Attached patch fix (obsolete) — Splinter Review
Assignee: sayrer → dietrich
Status: NEW → ASSIGNED
Attachment #319920 - Flags: review?(mak77)
Comment on attachment 319920 [details] [diff] [review]
fix

? toolkit/components/places/src/.nsLivemarkService.js.swp
Index: toolkit/components/places/src/nsLivemarkService.js
===================================================================
+/**
+ *
+ * Update Behavior in Firefox:
+ *
+ * Five seconds after the browser starts up,

the browser UI starts up


+ * is started via nsILivemarkService.start(). The update timer fires immediately
+ * and then every 15 minutes thereafter (gExpiration/4).

15 minutes is true only with default options, instead i would say:

The update timer fires immediately and by default every 15 minutes (is dynamically calculated as gExpiration/4), with a maximum limit of 1 hour.

 
     for (var i = 0; i < feed.items.length; ++i) {
       let entry = feed.items.queryElementAt(i, Ci.nsIFeedEntry);
       let href = entry.link;
-      if (!href)
-        continue;
+      if (!href) {
+        if (!siteURI)
+          continue;
+        else
+          href = siteURI;
+      }

this should work and imo is more readable

let href = entry.link || siteURI;
if (!href)
  continue;

this could probably need a minitest with an example local feed.
hwv r=me
Attachment #319920 - Flags: review?(mak77) → review+
Given our history with screwing up loops, a test that makes sure a feed with three entries, first and third with links and second without, continues to only use the siteURI for the second, would be a very good idea.
Attached patch fix v2 (obsolete) — Splinter Review
- comments fixed
- tests added (including philor's comment)
Attachment #319920 - Attachment is obsolete: true
Attachment #320218 - Flags: review?(mak77)
Hardware: PC → All
Target Milestone: Firefox 3 → ---
Priority: -- → P3
Target Milestone: --- → Firefox 3.1
Attached patch unbitrot (obsolete) — Splinter Review
sorry for late (damn late!), i've unbitrotted this for you, so forgive me :)

+
+/**
+ *
+ * Update Behavior in Firefox:
+ *
+ * Five seconds after the browser UI starts up, the livemark service's update
+ * timer is started via nsILivemarkService.start(). The update timer fires
+ * immediately and by default every 15 minutes (is dynamically calculated as
+ * gExpiration/4), with a maximum limit of 1 hour.
+ *
+ * When the update timer fires, it iterates over the list of livemarks, and
will
+ * refresh a livemark *only* if it's expired.
+ *
+ * The expiration time for a livemark is determined by using information
+ * provided by the server when the feed was requested, specifically
+ * nsICacheEntryInfo.expirationTime. If no information was provided by the 
+ * server, the default expiration time is 1 hour.
+ *
+ * Users can modify the default expiration time via the 
+ * browser.bookmarks.livemark_refresh_seconds preference.
+ *
+ */

Since we are about changing this i'd remove the description for now

fixe lines over 80 chars where possible
check for exceptions when instantiating services

apart that, r=me
Attachment #320218 - Attachment is obsolete: true
Attachment #337674 - Flags: review+
Attachment #320218 - Flags: review?(mak77)
Dietrich, do you have any time slot to fix this for 3.1? should only need an unbitrot and a cleanup.
Target Milestone: Firefox 3.1 → Firefox 3.2a1
Target Milestone: Firefox 3.6a1 → ---
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
Assignee: dietrich → nobody
Attached patch patch v1.0Splinter Review
Applied on top of the patch in bug 661445 (this bug adds a useful test to increase coverage for that bug)
Assignee: nobody → mak77
Attachment #337674 - Attachment is obsolete: true
Attachment #538412 - Flags: review?(dietrich)
Depends on: 661445
Flags: in-testsuite?
Comment on attachment 538412 [details] [diff] [review]
patch v1.0

r=me, thanks for fixing this!
Attachment #538412 - Flags: review?(dietrich) → review+
http://hg.mozilla.org/projects/places/rev/d7a1485f467d
Whiteboard: [fixed-in-places]
The test is randomly failing, I suspect it's the same issue seen in bug 561007 (so a GC running at a random time and collecting some related stuff), I can't reproduce neither on Win, nor on Linux64. Will try on Mac and on Tryserver, otherwise I'll disable test and file a follow-up to fix it :(
Depends on: 664986
the random timeouts have been handled in bug 664986.

http://hg.mozilla.org/mozilla-central/rev/d7a1485f467d
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 7
Mozilla/5.0 (Windows NT 5.1; rv:9.0a1) Gecko/20110822 Firefox/9.0a1
Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0

Verified issue on Win XP, Win 7, Ubuntu 11.04 and Mac OS X 10.6 using the RSS feed attached in Comment 9 - entries with no link are now displayed.

Setting resolution to VERIFIED FIXED.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: