412132 - after changing a bookmark's location, need to update the frecency of the "old" uri

Status: VERIFIED FIXED

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

Description

[Seth Spitzer]

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 1

[Drew Willcoxon :adw]

Attachment: Patches method nsNavBookmarks::ChangeBookmarkURI

Comment 2

[Drew Willcoxon :adw]

Attachment: Test case

Comment 3

[Shawn Wilsher :sdwilsh]

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 4

[Shawn Wilsher :sdwilsh]

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 5

[Drew Willcoxon :adw]

Attachment: Second try re: comment 3

Comment 6

[Dietrich Ayala (:dietrich)]

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.

Comment 7

[Drew Willcoxon :adw]

Attachment: patch updated re: comment 6

Comment 8

[Drew Willcoxon :adw]

Attachment: Test case

Added 3 tests.

Comment 9

[Dietrich Ayala (:dietrich)]

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 10

[Marco Bonardo [:mak]]

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 11

[Dietrich Ayala (:dietrich)]

Comment on attachment 356118 [details] [diff] [review]
Test case

looks ok, r=me. thanks!

Comment 12

[Drew Willcoxon :adw]

Attachment: patch updated re: comment 9

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.

Comment 13

[Drew Willcoxon :adw]

Attachment: Test case

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

Comment 14

[Drew Willcoxon :adw]

Attachment: test_expiration.js patch

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.

Comment 15

[Drew Willcoxon :adw]

Attachment: for checkin patch

Previous patches bundled together for checkin.

Comment 16

[Marco Bonardo [:mak]]

+              "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 ...

Comment 17

[Drew Willcoxon :adw]

Attachment: for checkin patch

Changes re: comment 16

Comment 18

[Marco Bonardo [:mak]]

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 19

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/6ddb1b2244aa

Comment 20

[Marco Bonardo [:mak]]

Comment on attachment 356557 [details] [diff] [review]
for checkin patch

asking approval for 1.9.1, medium risk but comes with tests.

Comment 21

[Tracy Walker [:tracy]]

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

Comment 22

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/63f8a612899a

Comment 23

[Tracy Walker [:tracy]]

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

Comment 24

[Gervase Markham [:gerv]]

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