Closed Bug 412132 Opened 12 years ago Closed 11 years ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 3.6a1

People

(Reporter: sspitzer, Assigned: adw)

References

Details

(Keywords: fixed1.9.1)

Attachments

(1 file, 9 obsolete files)

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.
Priority: -- → P4
Whiteboard: [good first bug]
Assignee: nobody → adw
Attached patch Test case (obsolete) — Splinter Review
Attachment #355870 - Flags: review?(dietrich)
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
Attachment #355871 - Flags: review?(dietrich)
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.
Attached patch Second try re: comment 3 (obsolete) — Splinter Review
Attachment #355870 - Attachment is obsolete: true
Attachment #355892 - Flags: review?(dietrich)
Attachment #355870 - Flags: review?(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.
Attachment #355892 - Flags: review?(dietrich) → review-
Attached patch patch updated re: comment 6 (obsolete) — Splinter Review
Attachment #355892 - Attachment is obsolete: true
Attachment #356110 - Flags: review?(dietrich)
Attached patch Test case (obsolete) — Splinter Review
Added 3 tests.
Attachment #355871 - Attachment is obsolete: true
Attachment #356118 - Flags: review?(dietrich)
Attachment #355871 - Flags: review?(dietrich)
Attachment #356110 - Flags: review?(dietrich) → review+
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!
Attachment #356118 - Flags: review?(dietrich) → review+
Attached patch patch updated re: comment 9 (obsolete) — Splinter Review
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.
Attachment #356110 - Attachment is obsolete: true
Attachment #356296 - Flags: review+
Attached patch Test case (obsolete) — Splinter Review
Added test: "Changing the URI of a nonexistent bookmark should fail."
Attachment #356118 - Attachment is obsolete: true
Attachment #356297 - Flags: review+
Attached patch test_expiration.js patch (obsolete) — Splinter Review
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.
Attachment #356299 - Flags: review?(dietrich)
Attachment #356299 - Flags: review?(dietrich) → review+
Attached patch for checkin patch (obsolete) — Splinter Review
Previous patches bundled together for checkin.
Attachment #356296 - Attachment is obsolete: true
Attachment #356297 - Attachment is obsolete: true
Attachment #356299 - Attachment is obsolete: true
Attachment #356544 - Flags: review+
+              "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
Attachment #356544 - Attachment is obsolete: true
Attachment #356557 - Flags: review+
Keywords: checkin-needed
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.
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/6ddb1b2244aa
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug]
Target Milestone: --- → Firefox 3.2a1
Attachment #356557 - Flags: approval1.9.1?
Comment on attachment 356557 [details] [diff] [review]
for checkin patch

asking approval for 1.9.1, medium risk but comes with tests.
Flags: in-testsuite+
Blocks: 449406
verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090127 Shiretoko/3.1b3pre
Status: RESOLVED → VERIFIED
Attachment #356557 - Flags: approval1.9.1? → approval1.9.1+
Keywords: fixed1.9.1
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.