319910 - Add bookmark function to check bookmarks for redirects

Status: RESOLVED FIXED

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

Description

[Brett Wilson]

Bookmark and annotation consumers will want to know which pages have ever redirected to a given page. See:

http://wiki.mozilla.org/Browser_History:Redirects

Comment 1

[Brett Wilson]

This will be very visible for the bookmarks button, so marking as a higher priority.

Comment 2

[Brett Wilson]

Attachment: Patch

Comment 3

[Brian Ryner (not reading)]

Comment on attachment 212926 [details] [diff] [review]
Patch

>--- nsNavBookmarks.cpp	16 Feb 2006 00:42:46 -0000	1.47
>+++ nsNavBookmarks.cpp	23 Feb 2006 19:41:17 -0000
>@@ -415,16 +444,205 @@
>+nsresult
>+nsNavBookmarks::FillBookmarksHash()
>+{
>+  nsresult rv;
>+  PRBool hasMore;
>+
>+  // first init the hashtable
>+  mBookmarksHash.Init(512);

Most of the hashtable operations are checked for out of memory, but not this one.

>+    mBookmarksHash.Put(toSpec, fromSpec);

or this one.

>+
>+    // handle redirects deeper than one level
>+    RecursiveAddBookmarkHash(fromSpec, toSpec, 0);

(and the result from this method isn't used for anything)

>+nsresult
>+nsNavBookmarks::RecursiveAddBookmarkHash(const nsCString& aBookmark,
>+                                         const nsCString& aCurrentSource,
>+                                         PRTime aMinTime)
>+{

should this all be wrapped in a transaction?

>+nsresult
>+nsNavBookmarks::UpdateBookmarkHashOnRemove(nsIURI* aURI)
>+{
>+  // note we have to use the DB version here since the hashtable may be
>+  // out-of-date
>+  PRBool inDB;
>+  nsresult rv = IsBookmarkedInDatabase(aURI, &inDB);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (inDB)
>+    return NS_OK; // bookmark still exists, don't need to update hashtable
>+
>+  // remove it
>+  nsCString spec;
>+  rv = aURI->GetSpec(spec);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  mBookmarksHash.Enumerate(RemoveBookmarkHashCallback,
>+                           NS_REINTERPRET_CAST(void*, &spec));

Generally it's not necessary to cast to void* - any pointer type should convert implicitly.

Looks ok otherwise.

Comment 4

[Joe Hughes]

So, what's the most efficient way to use this if I want to figure out if the given page (or a redirect-equivalent) is bookmarked, then keep track of the bookmarked page?  Is it:
bookmarkedURI = bms.getBookmarkedURIFor(currentURI);

Comment 5

[Brett Wilson]

> Is it: bookmarkedURI = bms.getBookmarkedURIFor(currentURI);

Yes.

Comment 6

[Brett Wilson]

Attachment: Better patch

This patch fixes a bug in the previous one where it would hang if you have a redirect cycle (actually surprisingly common for some access-protected pages). This also changes everything to use page IDs instead of URL specs which should make its footprint much smaller and much faster. It also saves us a lot of joins, making the DB statements faster.

Comment 7

[Brian Ryner (not reading)]

Comment on attachment 213062 [details] [diff] [review]
Better patch

>--- src/nsNavBookmarks.cpp	23 Feb 2006 22:18:34 -0000	1.48
>+++ src/nsNavBookmarks.cpp	24 Feb 2006 18:10:29 -0000
>@@ -250,16 +249,33 @@
>+  // must be last: This may cause bookmarks to be imported, which will exercise
>+  // most of the bookmark system
>   // keywords

nit: bring "keywords" up onto the previous line

>@@ -2368,40 +2391,55 @@
> //    (like referring visit ID and typed/bookmarked state).
> //
> //    This function walks up the referring chain and recursively calls itself,
> //    each time calling InternalAdd to create a new history entry. (When we
> //    get notified of redirects, we don't actually add any history entries, just
> //    save them in mRecentRedirects. This function will add all of them for a
> //    given destination page when that page is actually visited.)
> //    See GetRedirectFor for more information about how redirects work.
>+//
>+//    aRedirectBookmark should be empty when this function is first called. If
>+//    there are any redirects that are bookmarks the specs will be placed in
>+//    this buffer. The caller can then determine if any bookmarked items were
>+//    visited so it knows whether to update the bookmark service's redirect
>+//    hashtable.

This comment needs to be updated to reflect using the id instead of the url spec.

Looks great otherwise, I don't see any problems.

Comment 8

[Brett Wilson]

On branch and trunk.

Comment 9

[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