Closed Bug 259682 Opened 20 years ago Closed 20 years ago

Live Bookmarks / livemarks do not automatically refresh

Categories

(Firefox :: Bookmarks & History, defect)

Product:
Firefox
Component:
Bookmarks & History
Type:
defect
Priority:
Not set
Severity:
minor

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: zorin, Assigned: vlad)

References

Details

(Keywords: fixed-aviary1.0)

Attachments

(1 file, 1 obsolete file)

259682-livemark-refresh-fix-1.patch
12.42 KB, patch
shaver
: review+
shaver
: approval-aviary+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; rv:1.7.3) Gecko/20040913 Firefox/0.10
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; rv:1.7.3) Gecko/20040913 Firefox/0.10

Live bookmarks don't seem to refresh automatically, which diminishes their
usefulness. I couldn't find any documentation on whether they are supposed to or
not, nor could I find any related prefs, so I'm filing it as a bug.

Reproducible: Always
Steps to Reproduce:
1.Create a live bookmark of an RSS feed, say Slashdot.
2.Check it.
3.Check it again several hours later. No change.
4.Refresh it manually
5.Check it again; new content.

Actual Results:  
The Live bookmark does not update.

Expected Results:  
Live bookmarks should update automatically, preferably with a user configurable
interval.
Hardware: Macintosh → All
Firefox should support automatic updates on Live Bookmarks, in my opinion. The
term 'Live' in suggests that these things are automatically updated, because if
they're not then they can hardly be considered live.

I'd suggest that the refresh rate for bookmarks (if implemented) needs to have a
default time. I'd suggest 3 hours, but I've no good reasoning to back that up so
if someone can think up a better default and give some good reasoning then go
ahead. There also needs to be a user interface setting to change the default
refresh time, both globally and on a per bookmark basis.

It's also important that Firefox honours the TTL property of feeds (see
http://blogs.law.harvard.edu/tech/rss#ltttlgtSubelementOfLtchannelgt) if it is
lower than the user setting - and that it also handles the http headers for
feeds in the correct manner (current behaviour is unknown to me).
Attached patch 259682-livemark-refresh-fix-0 (obsolete) — — Splinter Review 
Finally tracked this down.  The template builder was caching content, and was
never hitting the data source again -- so we were never asked to fetch the data
again.	Here we do it as part of the bookmarks timeout.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
>     if (NS_FAILED(rv)) {
>-        // if we failed, try again in 5 minutes, to avoid trying
>+        // if we failed, try again in 10 minutes, to avoid trying
>         // to load the livemark over and over.
>-        ttl = 300;          
>+        ttl = 600;          

That worries me. I'll buy the earlier "couldn't fetch the data, try in 5
minutes", but I don't think that a parser-breaking error in my feed is an
invitation to reparse it every 10 minutes: that's insult to injury, if the try
again is automatic, not forced by an impatient user. 
(In reply to comment #3)
> >     if (NS_FAILED(rv)) {
> >-        // if we failed, try again in 5 minutes, to avoid trying
> >+        // if we failed, try again in 10 minutes, to avoid trying
> >         // to load the livemark over and over.
> >-        ttl = 300;          
> >+        ttl = 600;          
> 
> That worries me. I'll buy the earlier "couldn't fetch the data, try in 5
> minutes", but I don't think that a parser-breaking error in my feed is an
> invitation to reparse it every 10 minutes: that's insult to injury, if the try
> again is automatic, not forced by an impatient user. 

Good point; though the previous "couldn't fetch the data" was also a "couldn't
parse the data".  I'll make it a real "couldn't fetch the data" check.  Also, as
was pointed out to me, the patch in bug 261109 should be incorporated in this
one -- the next time we load the feed should be the max of the cache expiry time
and the ttl (if the cache expiry time is > the ttl, then we'll just end up
reparsing from the cache, which doesn't do us any good).

(In reply to comment #4)
> Good point; though the previous "couldn't fetch the data" was also a "couldn't
> parse the data".  I'll make it a real "couldn't fetch the data" check.

Don't you have one? I thought that was
http://lxr.mozilla.org/mozilla/source/browser/components/bookmarks/src/nsBookmarksFeedHandler.cpp#207
Comment on attachment 160703 [details] [diff] [review]
259682-livemark-refresh-fix-0

There's no way that bmks->UpdateLivemarkChildren() can cause livemarkEnumerator
to end up confused, right?  Other than that mild concern, it looks fine.

Do we have test cases for these that verify that we do the right thing in terms
of timer operation and refetching for the different |user-ttl OP cache-expiry|
cases?
Attachment #160703 - Flags: review+
Attachment #160703 - Flags: approval-aviary?
(In reply to comment #6)
> (From update of attachment 160703 [details] [diff] [review])
> There's no way that bmks->UpdateLivemarkChildren() can cause livemarkEnumerator
> to end up confused, right?  Other than that mild concern, it looks fine.

Nope; UpdateLivemarkChildren doesn't touch the things that livemarkEnumerator is
iterating.

> Do we have test cases for these that verify that we do the right thing in terms
> of timer operation and refetching for the different |user-ttl OP cache-expiry|
> cases?

No, though this patch doesn't do that yet.. which is why I cancelled the review
and was going to do another one tomorrow to incorporate that :)

I'm not sure about a testing protocol, but if it helps I made a little more
flexible version of Ted's test feed from bug 261109 -
philringnalda.com/mtests/feedingtime.php/n will return a feed with the fetch
time in the single item title, and Expires and Cache-control set for n minutes
(or 5, if you leave it off).
(In reply to comment #8)
> I'm not sure about a testing protocol, but if it helps I made a little more
> flexible version of Ted's test feed from bug 261109 -
> philringnalda.com/mtests/feedingtime.php/n will return a feed with the fetch
> time in the single item title, and Expires and Cache-control set for n minutes
> (or 5, if you leave it off).

Ah, cool.. I'll use it tomorrow to test this stuff somoe more.

New patch, incorporating cache expiration time check from bug 261109.
Attachment #160703 - Attachment is obsolete: true
*** Bug 261109 has been marked as a duplicate of this bug. ***
This patch seems to fix bug 252412 (livemark in bookmarks folder causes the
folder to collapse the 1st time it's opened).
Attachment #160788 - Flags: review?(shaver)
Attachment #160788 - Flags: review+
Attachment #160788 - Flags: approval-aviary+
In on aviary.. Timo, can you check bug 252412 with the nightly tomorrow?  I
can't imagine why it would fix it, but hey, i'll take a freebie... :)
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.3) Gecko/20041003
Firefox/0.10

Confirming that refresh works fine (modded speed to test)

keyword fixed-aviary-1.0 missing, Vlad
after some further testing I notice 2 things:
1) the computer is gradually slowing down
2) I sometimes loose contextmenu

I set live bookmarks refresh at 600 secs and have about 10 of them in my
bookmarks folder.

Investigating relation with this patch
(In reply to comment #12)
> This patch seems to fix bug 252412 (livemark in bookmarks folder causes the
> folder to collapse the 1st time it's opened).
Confirming, I see the same, the collapse appears to have vanished.
(maybe Live BM's loaded when browser is started ?)

Can you add the new "livemark_refresh_seconds" pref to firefox.js, so that it
shows up in about:config, please?
after some reboots it seems that:
1) bug 252412 isn't gone
2) the refresh doesn't work (tested extensivly with Phil Ringnalda, both RDF as RSS)
3) a livemark_refresh_seconds of 300 and 10 Live bookmarks in use causes drag
and unresponsive browser behaviour
4) frequent loss of contextmenus
   no response when typing text
   when some refreshprocess takes place, but doesn't refresh the livebookmarks.

Using an AMD850/512 MB
(In reply to comment #18)
> after some reboots it seems that:
> 1) bug 252412 isn't gone

I figured it wasn't.  Unfortunately, the bug that's causing it isn't a bookmarks
bug at all, but something to do with our menu code.. which is very scary.

> 2) the refresh doesn't work (tested extensivly with Phil Ringnalda, both RDF
as RSS)

How did you test?  Note that the refresh timeout (livemark_refresh_seconds that
is) only has an effect if the page's cache expiration is lower than that.  If
the page cache expiration time is greater than livemark_refresh_seconds, then
the page cache expiration time is used instead.

> 3) a livemark_refresh_seconds of 300 and 10 Live bookmarks in use causes drag
> and unresponsive browser behaviour

Hmmm.  The network fetching is asynchronous, the parsing is sync, but it should
happen pretty quickly.  It was pretty snappy for me with about a dozen live
bookmarks, but I'll give it another try.

> 4) frequent loss of contextmenus

This is the same cause as #1; something is forcing menus to get rolled up.

>    no response when typing text
>    when some refreshprocess takes place, but doesn't refresh the livebookmarks.

Not sure what you mean here?  It doesn't refresh the menu items?  Or something else?

Going to reopen this; Thanks for the feedback!


    - Vlad
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to comment #19)

> > 4) frequent loss of contextmenus
> 
> This is the same cause as #1; something is forcing menus to get rolled up.
> 
> >    no response when typing text
> >    when some refreshprocess takes place, but doesn't refresh the livebookmarks.
> 
> Not sure what you mean here?  It doesn't refresh the menu items?  Or something
else?
> 
> Going to reopen this; Thanks for the feedback!
> 
> 
>     - Vlad

It doesn't refresh the live bookmarks at all.
Allthough I don't know the settings exactly I'm sure Phil had the expiration
setting set to minimal at the end, at the first tests he used 5 minutes and I a
refrehrate of 300 to start, decreasing to 120 at the end. It made no difference,
it just wouldn't update.
http://philringnalda.com/mtests/feedingtime.php/4 (<rdf:>)


>How did you test?  Note that the refresh timeout (livemark_refresh_seconds that
>is) only has an effect if the page's cache expiration is lower than that.  If
>the page cache expiration time is greater than livemark_refresh_seconds, then
>the page cache expiration time is used instead.

Shouldn't this be the other way around? If the user wants Firefox to check the
RSS feed more often than the expiration the page wants, they should be able to
do so.

Slashdot has an expiration time of one hour, I believe, but the RSS feed updates
in real time. This is a case where a lower user-specified refresh time would be
helpful.

-Z
ok, with the proper pref
browser.bookmarks.livemark_refresh_seconds
the updating does work fine.

Investgating the other issues

(changed OS to ALL, was still Mac)
OS: MacOS X → All
ok, here we go

1- bug 252412 is still there, as you predicted/expected
2- after setting the proper prev the update works
3- changing the prev value in about:config works and changes the refreshrate correct
4- the drag has vanished, however, the fact that the use of a wrong prev
(livemark_refresh_seconds) caused this is a bit strange (needs investigating
?).It happened in 4 different consecutive sessions with reboots in between so
this may be something nasty.

I think it's fair to set FIXED

the problem with .4 needs another bug, if someone can confirm it that is.



How about UI? This should be a separate bug?
maybe it wouldn't be a bad idea to take scheduling bookmarks(currently hidden
UI) and scheduling live bookmarks in one (no UI yet). Another bug and definitly
another time (not before 1.0)
Status: REOPENED → RESOLVED
Closed: 20 years ago → 20 years ago
Keywords: fixed-aviary1.0
Resolution: --- → FIXED
*** Bug 264216 has been marked as a duplicate of this bug. ***
*** Bug 264218 has been marked as a duplicate of this bug. ***
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.

Attachment

General

Creator:
Created:
Updated:
Size: