Actually our timing on livemark update is: browser start -> 5s -> livemark start -> 15s -> check -> 15s -> check -> ... so we have 2 issues: - livemarks are updated after 20s, that could be a problem for some user that still see the old livemarks - we are checking too often (why check every 15s?) imho we should change to a situation where we do: browser start -> 5s -> livemark start -> check -> 5m -> check -> 5m -> check -> ...

Do we need to do anything about the way that browser.bookmarks.livemark_refresh_seconds has a minimum of 1m? Every time I've thought about doing this, I've gotten tangled up in my own thoughts about how that relates to an increased LIVEMARK_TIMEOUT.

mmh, LIVEMARK_TIMEOUT fires the timer for the update, actual update checks for anno expiration that is based upon the max between browser.bookmarks.livemark_refresh_seconds and 60s. in the worst case of a user with a 60s pref he could expect that the livemarks will be updated every 60s, while we will update them every 5 minutes... i don't get why a user would want to have the browser busy every minute in updating livemarks though... maybe we should increase the pref min, or instead of using a fixed constant we could use a time based on user pref, for example 1/4 of the pref (so with 60s try refresh every 15s, with default of 1hour do a refresh every 15m and so on...)

Comment on attachment 305999 [details] [diff] [review] patch clearing the review request waiting for a decision

check time has been made: MIN( FLOOR(expiration / 4), 1h); so we should check 4 times in the expiration time, if the user sets an expiration time > 4h we will still check every hour (this will allow us to catch feeds that failed in previous checks) includes fix for Bug 390505 (pageAnnos have been changed to itemAnnos) so even if we have more feeds with the same url we will update them (previously we were updating only the first found) i've tried to cleanup variable names to make them consistent between functions, removed a useless hasAnnotation check (we can catch the exception really) and changed the fireTimer function name to a more realistic checkAllLivemarks. Since there are other patches waiting for this service (Bug 407468, Bug 417729, Bug 420078) this will unbitrot soon, still i think we should take functionality patches before perf and cleanup ones.

s/includes fix for Bug 390505/includes fix for Bug 388716/ sorry, wrong bug number!

Comment on attachment 306719 [details] [diff] [review] patch >@@ -240,27 +249,28 @@ LivemarkService.prototype = { > > var livemark = this._livemarks[index]; > livemark.locked = true; > try { > // Check the TTL/expiration on this. If there isn't one, > // then we assume it's never been loaded. We perform this > // check even when the update is being forced, in case the > // livemark has somehow never been loaded. >- var exprTime = this._ans.getPageAnnotation(livemark.feedURI, >+ var expireTime = this._ans.getItemAnnotation(livemark.folderId, > LMANNO_EXPIRATION); nit: fix indent >- getSiteURI: function LS_getSiteURI(container) { >- this._ensureLivemark(container); >+ getSiteURI: function LS_getSiteURI(folderId) { >+ this._ensureLivemark(folderId); > >- if (this._ans.itemHasAnnotation(container, LMANNO_SITEURI)) { >+ if (this._ans.itemHasAnnotation(folderId, LMANNO_SITEURI)) { > var siteURIString = >- this._ans.getItemAnnotation(container, LMANNO_SITEURI); >+ this._ans.getItemAnnotation(folderId, LMANNO_SITEURI); > > return gIoService.newURI(siteURIString, null, null); > } > return null; > }, > please use try/catch instead of itemHasAnnotation, like you did in getFeedURI. > _setResourceTTL: function LLL__setResourceTTL(milliseconds) { >- var exptime = Date.now() + milliseconds; >- this._ans.setPageAnnotation(this._livemark.feedURI, LMANNO_EXPIRATION, >- exptime, 0, >+ var expireTime = Date.now() + milliseconds; >+ this._ans.setItemAnnotation(this._livemark.folderId, LMANNO_EXPIRATION, >+ expireTime, 0, > Ci.nsIAnnotationService.EXPIRE_NEVER); yes, much better. these will survive backup/restore now. r=me with these fixed

Danger, danger, antipattern alert! Where is the profiling that shows itemHasAnnotation being horribly expensive? The change in getFeedURI means that not only will you be catching the expected error when the annotation isn't there, you'll also be swallowing any other errors in getItemAnnotation, and swallowing the error when someone changes the function and misses this caller, and swallowing the error when someone changes the name of the annotation const and misses this caller, and swallowing any errors in newURI (and making life even more miserable for anyone trying to break on exceptions in Venkman). Given no other choice, sure, try/catch, test the caught error against the single error you expected and rethrow if it's not that, but here you have another choice.

(In reply to comment #8) > Danger, danger, antipattern alert! > > Where is the profiling that shows itemHasAnnotation being horribly expensive? it's likely not *horrible*, but does ensure 2 database queries instead of one. that said, this is not an oft-called method (relative to something like isLivemark), so I'm probably being over-miserly. > The change in getFeedURI means that not only will you be catching the expected > error when the annotation isn't there, you'll also be swallowing any other > errors in getItemAnnotation, and swallowing the error when someone changes the > function and misses this caller, and swallowing the error when someone changes > the name of the annotation const and misses this caller, and swallowing any > errors in newURI (and making life even more miserable for anyone trying to > break on exceptions in Venkman). > > Given no other choice, sure, try/catch, test the caught error against the > single error you expected and rethrow if it's not that, but here you have > another choice. > Yes, you're right. I'm blinded by perf concerns.

Comment on attachment 306719 [details] [diff] [review] patch see comment #8. shouldn't use try/catch to get round a non-performant pattern. we should probably fix the annotation service to not throw in a non-error state, but that's certainly for a different bug :)

thanks phil for the hint, you're certainly right, i only thought to current impl of that functions that is quite linear. i'll revert back that change and we'll fix that thing elsewhere

reverted changes to getFeedURI, more variable name cleanup, added a check in createLivemarkFolderOnly (the same done in createLivemark, to avoid adding livemarks to livemarks)

changed aFolderId to aParentId in methods that create a livemark folder, since after var renaming to be consistent we have "folderId = bms.createFolder(aFolderId", that could be confusing. changed in createLivemarkFolderOnly: - this._pushLivemark(livemarkID, feedURI); - var livemarkIndex = this._getLivemarkIndex(livemarkID); to: + var livemarkIndex = this._pushLivemark(folderId, aFeedURI) - 1; since _pushLivemark returns the new length of the array (we were already doing the same in createLivemark, and that avoids a full looping through all livemarks)

Comment on attachment 307022 [details] [diff] [review] patch >@@ -581,19 +592,18 @@ LivemarkLoadListener.prototype = { > this._processor = null; > this._livemark.locked = false; > } > }, > > deleteLivemarkChildren: LivemarkService.prototype.deleteLivemarkChildren, > > insertLivemarkChild: >- function LS_insertLivemarkChild(folderId, uri, title) { >- var id = this._bms.insertBookmark(folderId, uri, this._bms.DEFAULT_INDEX, >- title); >+ function LS_insertLivemarkChild(aFolderId, uri, title) { >+ this._bms.insertBookmark(aFolderId, uri, this._bms.DEFAULT_INDEX, title); nit: if you're going to prefix parameters with "a", please do it consistently. r=me otherwise. please ping ondrej, or note on bug 420078 that this patch rots it.

changed input vars consistently as requested, i've re-read this about 1 thousands times (will never be enough) in previous patch i had forgotten the changes to nsNavHistory, those were however already contained in previously reviewed patch.

thanks for the fixed up patch. another thought: we should probably fix browser.bookmarks.livemark_refresh_seconds to be places.* or something not browser-specific. and should add a default to /browser/app/profile/firefox.js. please file a followup for this? (don't want to continue to overload this bug)

(In reply to comment #17) > please file a followup for this? (don't want to continue to overload this bug) filled Bug 420748

drivers, this is needed by blocker Bug 390505, plus var cleanup will make the code less error-prone

Comment on attachment 307216 [details] [diff] [review] unbitrot damn this was polluted by a patch i was working on in nsnavhistory, need a cleaned one

Checking in toolkit/components/places/src/nsLivemarkService.js; /cvsroot/mozilla/toolkit/components/places/src/nsLivemarkService.js,v <-- nsLivemarkService.js new revision: 1.40; previous revision: 1.39 done Checking in toolkit/components/places/src/nsNavHistory.cpp; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v <-- nsNavHistory.cpp new revision: 1.268; previous revision: 1.267 done

trying to verify this. But I'm unable to sort out what the final FIX was.

you should check that livemark check happens after about 5s from ui startup and then every 5 minutes...

Feeds like http://del.icio.us/recent do not update shortly after startup. I waited 10-15 seconds. This feed is constantly getting new posts so it is a reliable test case feed. The feed doesn't appear to auto-update at all. Waiting 5 minutes makes no difference. Feedback for feeds on the toolbar like a slightly pulsing icon would be nice. reopening as I cannot verify this is fixed on nightly builds on Mac or Windows.

should look at bug 251842 for feed-specific refresh policies instead of arbitrary update times.

This is now blocking RC1; do we have a regression range?

I just grabbed a build on Windows (20080308) for the day after the fix went in. (20080307). Feeds are not updating with that build as described in comment# 25. Note: it's broken in BM menu and BM toolbar.

at gavin's request: filed bug 432690

(In reply to comment #25) > you should check that livemark check happens after about 5s from ui startup and > then every 5 minutes... NOTE: this is my fault. actual timing is: UI START -> 5s -> update -> 15min -> update -> so on...

verified with: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko/2008050704 Minefield/3.0pre

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