455315 - when removing a bookmark we recalculate frecency with wrong isBookmark

Status: VERIFIED FIXED

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

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

spin-off from bug 418643, in a couple places when removing a bookmark we are doing

rv = History()->UpdateFrecency(placeId, PR_FALSE /* isBookmark */);

but we don't know if the uri is bookmarked in some other place, so we should check if a bookmark to that placeId exists somewhere else (paying attention to perf)

Comment 1

[Shawn Wilsher :sdwilsh]

Bug 449406 adds a nice method on nsNavBookmarks (IsRealBookmark) that has a fast path that checks if it's in the hashtable first that can probably be useful here.

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

same issue in removefolderChildren and removeItem.

Comment 3

[Dietrich Ayala (:dietrich)]

would like to get this in 3.1, once bug 449406 is fixed.

Comment 4

[Drew Willcoxon :adw]

It sounds like this is the same problem Bug 412132 addressed.

Comment 5

[Drew Willcoxon :adw]

Attachment: v1.0

Changes are pretty straight-forward using sdwilsh's IsRealBookmark function (comment 1).  Includes a test case.

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 360114 [details] [diff] [review]
v1.0

some fly-by drive comment, i didn't looked through the all the tests

>+    rv = History()->UpdateFrecency(placeId, IsRealBookmark(placeId));

could be a good idea to add a comment saying why we need to do this, since the previous assumption was wrong.
So the fact we check if it's a bookmark because we could have more than one bookmark for this address.

>diff --git a/toolkit/components/places/tests/unit/test_update_frecency_after_delete.js b/toolkit/components/places/tests/unit/test_update_frecency_after_delete.js

>+function createLivemark(lmItemURI) {
>
>+  let lmId = lmServ.createLivemarkFolderOnly(bmServ.unfiledBookmarksFolder,
>
>+                                             "livemark title",
>
>+                                             uri("http://example.com/"),
>
>+                                             uri("http://example.com/rdf"),
>
>+                                             -1);
>
>+  let lmItemId = bmServ.insertBookmark(lmId,
>
>+                                       lmItemURI,
>
>+                                       bmServ.DEFAULT_INDEX,
>
>+                                       "livemark item title");
>
>+  return lmItemId;
>
>+}

using lmItemId and lmChildItemId would be maybe more readable

>+function getFrecency(url) {
>
>+  let sql = "SELECT frecency FROM moz_places_view WHERE url = ?1";
>
>+  let stmt = dbConn.createStatement(sql);
>
>+  stmt.bindUTF8StringParameter(0, url);
>
>+  do_check_true(stmt.executeStep());
>
>+  let frecency = stmt.getInt32(0);
>

nothing bad with this, but notice you can use the storage wrapper in JS, that's more readable (use :url in the query and then stmt.params.url = url)

>
>+function prepTest(testIndex, testName) {
>
>+  print("Test " + testIndex + ": " + testName);
>
>+  histServ.QueryInterface(Ci.nsIBrowserHistory).removeAllPages();
>
>+  dbConn.executeSimpleSQL("DELETE FROM moz_places_view");

this looks bad, won't this also remove the places for the bookmarks you're not removing below?

>+
>
>+function visit(uri) {
>
>+  histServ.addVisit(uri,
>
>+                    Date.now() * 1000,
>
>+                    null,
>
>+                    histServ.TRANSITION_BOOKMARK,
>
>+                    false,
>
>+                    0);
>
>+}

checking for visitId > 0 is a good sanity check
Notice that IIRC transition_bookmark has a particular frecency since it double counts the bookmark visit bonus, hwv this is probably covered by Shawn's tests.


>+function run_test() {
>
>+  dbConn =
>
>+    Cc["@mozilla.org/browser/nav-history-service;1"].
>
>+    getService(Ci.nsPIPlacesDatabase).
>
>+    DBConnection;

any reason to not get this above when you define dbConn?

>
>+
>
>+  let stmt = dbConn.createStatement("SELECT MAX(id) FROM moz_bookmarks");
>
>+  stmt.executeStep();
>
>+  defaultBookmarksMaxId = stmt.getInt32(0);
>

ids are usually int64

Comment 7

[Drew Willcoxon :adw]

Attachment: v2.0

Changes re: comment 6, thanks.

(In reply to comment #6)
> >+function prepTest(testIndex, testName) {
> >
> >+  print("Test " + testIndex + ": " + testName);
> >
> >+  histServ.QueryInterface(Ci.nsIBrowserHistory).removeAllPages();
> >
> >+  dbConn.executeSimpleSQL("DELETE FROM moz_places_view");
> 
> this looks bad, won't this also remove the places for the bookmarks you're not
> removing below?

It's just emptying the places tables and all the non-built-in bookmarks (i.e., all bookmark items but the toolbar folder, tag folder, etc.).

I should note that this test case is very similar to the one I wrote for bug 412132, which was approved by Dietrich.

Comment 8

[Drew Willcoxon :adw]

Attachment: v3.0

Changes per conversation with MaK77 on IRC.  Copied function remove_all_bookmarks and its helper check_no_bookmarks from tests/bookmarks/head_bookmarks.js to tests/unit/head_bookmarks.js.  The test case now uses this func instead of direct SQL to clear the database.

Comment 9

[Dietrich Ayala (:dietrich)]

Comment on attachment 360341 [details] [diff] [review]
v3.0

r=me thanks!

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/2cc43e087677

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 360341 [details] [diff] [review]
v3.0

asking approval, a nice-to-have follow-up to a blocker, comes with unit tests.

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 360341 [details] [diff] [review]
v3.0

a191=beltzner

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9546fd07cf43

Comment 15

[Tracy Walker [:tracy]]

verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090415 Shiretoko/3.5b4pre

Comment 16

[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