after changing a bookmark's location, need to update the frecency of the "old" uri. we already update the frecency of the "new" uri for the bookmark. From the current patch in bug #394038, see nsNavBookmarks::ChangeBookmarkURI() // upon changing the uri for a bookmark, update the frecency for the new place // no need to check if this is a livemark, because... rv = History()->UpdateFrecency(placeId, PR_TRUE /* isBookmark */); NS_ENSURE_SUCCESS(rv, rv); #if 0 // upon changing the uri for a bookmark, update the frecency for the old place // XXX todo, we need to get the oldPlaceId (fk) before changing it above // and then here, we need to determine if that oldPlaceId is still a bookmark (and not a livemark) rv = History()->UpdateFrecency(oldPlaceId, PR_FALSE /* isBookmark */); NS_ENSURE_SUCCESS(rv, rv); #endif notes: 1) Q: "no need to check if this is a livemark, because..."? A: the reasons we check if a bookmark is real child of a livemark item is that those should not appear in autocomplete (so frecency should be = 0) if they are not visited. When changing a bookmark's uri, the new url is a legit bookmark, so we want it to appear in ac, even if not visited. 2) if we don't fix this, here's an how the user would see it: say I have a legit bookmark to foo.com/story1.html and that is also a livemark item for food.com/feed.xml. foo.com/story1.html is not visited, but since it is a legit bookmark, and not just an item of a livemark feed, it should show up in the ac results. now, say I change the uri for the legit foo.com/story1.html bookmark to be foo.com/story2.html. the current code will call UpdateFrecency() on foo.com/story2.html, and pass in isBookmark = true, so this will now be visible in ac even if not visited. but, the disabled code will not call UpdateFrecency() on foo.com/story1.html, so it will appear in ac results even though it is only an unvisitied livemark item.

Comment on attachment 355870 [details] [diff] [review] Patches method nsNavBookmarks::ChangeBookmarkURI I'm going to have dietrich do this review since I'm not 100% familiar with this code. I do have a few comments though: >+ // We need the bookmark's current URI and corresponding places ID below, so get >+ // them now before we change them. >+ nsIURI *oldURI; >+ PRInt64 oldPlaceId; >+ rv = GetBookmarkURI(aBookmarkId, &oldURI); Anything that is ever derived from nsISupports needs to be in an nsCOMPtr: nsCOMPtr<nsIURI> oldURI; ... rv = GetBookmarkURI(aBookmarkId, getter_AddRefs(oldURI)); >+ // Upon changing the uri for a bookmark, update the frecency for the new place. >+ // No need to check if this is a livemark, because changing a bookmark's URI >+ // => new URI is legit bookmark. This comment didn't really make sense to me

Comment on attachment 355871 [details] [diff] [review] Test case Test looks fine to me, but since dietrich is looking at the code, he should probably look at this too.

Comment on attachment 355892 [details] [diff] [review] Second try re: comment 3 >+ >+ // For each bookmark ID associated with oldURI, get its parent ID. If any >+ // such parent is not a livemark, then oldURI is still bookmarked. >+ for (PRUint32 i = 0; i < bookmarkIds.Length(); i++) { >+ PRInt64 parentId; >+ rv = GetFolderIdForItem(bookmarkIds[i], &parentId); >+ NS_ENSURE_SUCCESS(rv, rv); >+ PRBool parentIsLivemark; >+ rv = lms->IsLivemark(parentId, &parentIsLivemark); >+ NS_ENSURE_SUCCESS(rv, rv); >+ if (!parentIsLivemark) { >+ isBookmarked = PR_TRUE; >+ break; >+ } >+ } >+ } this approach will execute two queries per bookmark found (isLivemark() -> annosvc.itemHasAnnotation()), as well as causing xpconnect crossings. it'd be far faster to just query the db directly once, for bookmarks w/ that place id, and whose parents do not have the livemark annotation. we've found that the performance cost of abstracting the livemarks implementation in the back-end is too high, so here it's ok to use raw SQL.

Added 3 tests.

Comment on attachment 356110 [details] [diff] [review] patch updated re: comment 6 >+ "SELECT children.id " >+ "FROM moz_bookmarks AS children, moz_bookmarks AS parents " >+ "WHERE children.fk = ?1 AND " >+ "children.parent = parents.id AND " >+ "parents.id NOT IN (" >+ "SELECT moz_items_annos.item_id " >+ "FROM moz_items_annos, moz_anno_attributes " >+ "WHERE moz_items_annos.anno_attribute_id = moz_anno_attributes.id AND " >+ "moz_anno_attributes.name = '" >+ LMANNO_FEEDURI >+ "'" >+ ")"), defensively, should add a check for item type = bookmark. also, since children.parent is a moz_bookmarks row id, could you do it w/ a single table? eg: children.parent NOT IN (...) otherwise, r=me.

Comment on attachment 356110 [details] [diff] [review] patch updated re: comment 6 a fly-by review comment: >+ nsCOMPtr<mozIStorageStatement> isBookmarkedStmt; >+ rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING( >+ "SELECT children.id " >+ "FROM moz_bookmarks AS children, moz_bookmarks AS parents " >+ "WHERE children.fk = ?1 AND " >+ "children.parent = parents.id AND " >+ "parents.id NOT IN (" >+ "SELECT moz_items_annos.item_id " >+ "FROM moz_items_annos, moz_anno_attributes " >+ "WHERE moz_items_annos.anno_attribute_id = moz_anno_attributes.id AND " >+ "moz_anno_attributes.name = '" >+ LMANNO_FEEDURI >+ "'" >+ ")"), could you make this a JOIN please, that'd be more readable and most likely faster, you can take a look at the second part of the query at http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsNavHistory.cpp#2506 for an example (Clearly that query is selecting livemarks while you need to do the opposite). Also i would bind the attribute name rather than inlining it (we use to).

Comment on attachment 356118 [details] [diff] [review] Test case looks ok, r=me. thanks!

re: comment 9 Changes made, thanks. re: comment 10 The join went away. (See comment 9 -- silly mistake by me. Hopefully bugs like this will beef up my SQL-fu :) Attribute name now bound.

Added test: "Changing the URI of a nonexistent bookmark should fail."

As discussed in #places today, the patch to this bug slightly changes the semantics of nsNavBookmarks::ChangeBookmarkURI: it's no longer legal to pass in a bad item ID. Unit test toolkit/components/places/tests/unit/test_expiration.js does just that (for reasons I can't discern), so I've patched it here so that the test now passes with the new bug patch. It might not be a big deal, but I don't know this test code at all, so I've flagged it for review.

Previous patches bundled together for checkin.

+ "SELECT moz_items_annos.item_id " use aliases please, you should not refer directly to the table name, it could sound correct but in some complex case that could force a complete table lookup (slow) instead of a lookup on the current results table. So take a look around at common aliases we use for every table (moz_items_annos is usually aliased as a so "SELECT FROM moz_items_annos a" and refer to columns as a.column). + "FROM moz_items_annos, moz_anno_attributes " + "WHERE moz_items_annos.anno_attribute_id = moz_anno_attributes.id AND " this is a JOIN really, sqlite optimizer is probably already doing the right thing, but is better to explicitely do that by ourselves. FROM moz_items_annos a JOIN moz_anno_attributes n ON a.anno_attribute_id = n.id WHERE ...

Changes re: comment 16

i have this in a mercurial queue ready to land, so i'll push it with my other patches, probably tomorrow if tree is going green enough.

Comment on attachment 356557 [details] [diff] [review] for checkin patch asking approval for 1.9.1, medium risk but comes with tests.

verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090127 Shiretoko/3.1b3pre

sorry CnP error before. reverified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090202 Shiretoko/3.1b3pre

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