306640 - Add generic annotation service for associating data with web pages

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, enhancement)

Description

[Brett Wilson]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.9) Gecko/20050711 Firefox/1.0.5
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.9) Gecko/20050711 Firefox/1.0.5

There should be some kind of service for associating data with web pages. This
could be used by extensions to provide interesting services like associating
notes with a web page. This will also be used to write richer bookmarks and
history components, and to as a replacement for current things like storing form
data and favorite icons.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Brett Wilson]

Attachment: Proposed interface

Proposed interface for the annotation service. This does not yet implement the
expiration of annotations. That is better saved until after the new history
system is ready.

Comment 2

[Brett Wilson]

Anybody have comments on whether the interface should take a nsIURI or a string URI?

The current implementation uses nsIURI, but it might be easier for some callers
to just take a string, especially if you get a URL from history and want see
what it has: you have to create an object which parses the URI, all for naught.

Comment 3

[timeless]

Comment on attachment 194570 [details]
Proposed interface

please don't use //, use
/**
 * ...
 */
even for documenting constants. the parsers we have will do useful things for
such comments.

i'm not particularly fond of expire_never = 5.
i'd rather -1....

don't use wstring.

why are you using nsIVariant instead of nsISupports or nsISerializable ?

document what happens if i call getAnnotationInfo for something that has no
such annotation.

I'd rather removePageAnnotations take a second parameter that's some sort of
string or regexp or whatever.

bill should be able to kill all bill annotations easily w/o blowing away fred
annotations.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #3)
> why are you using nsIVariant instead of nsISupports or nsISerializable ?

nsISupports/nsISerializable suck horribly when used from JS.  nsIVariant lets
the JS API be sane through xpconnect.  (Do we get a SupportsString or whatever
if you pass in a string to a nsISupports param through xpconnect?)

Comment 5

[Brett Wilson]

The purpose I had in mind for removePageAnnotations would be when we are purging
the page from the DB and we need to remove everything associated with it.

If you want to do something fancier, you can always just get the annotations and
delete the ones you don't want. This seems almost as easy and more flexible than
a regexp. I think if there is going to be a lot of contention about this
function, we should just remove it.

Comment 6

[Brett Wilson]

See also the Wiki:

http://wiki.mozilla.org/Annotations

Comment 7

[Brett Wilson]

Attachment: Preliminary annotation service interface and implementation

This adds the annotation service to the "places" component (not built by default). There are some missing implementations and commented out interfaces: this is just a first pass, we can add some more helpful functions to get lists of annotation snd expire annotations later.

Comment 8

[O. Atsushi (Torisugari)]

(In reply to comment #7)
> Created an attachment (id=202424)

> Index: components/places/public/nsIAnnotationService.idl
> Index: components/places/src/nsAnnotationService.cpp
> Index: components/places/src/nsAnnotationService.h

Then these are "browser" components?
I agree "places" should be one of browser components (bug 266174), but
annotation service might be useful for other products, Thunderbird and
thrid-party apps with xulrunner.

Comment 9

[O. Atsushi (Torisugari)]

(In reply to comment #7)
> expire annotations later.

IMHO, setting/getting the expiration date would be more solid spec than
setting/getting the life time days/weeks/months.

Comment 10

[Brett Wilson]

(In reply to comment #8)
> Then these are "browser" components?

This service is very tied to history/bookmarks. Implementation of a similar system for other consumers is trivial using mozStorage.

(In reply to comment #9)
> IMHO, setting/getting the expiration date would be more solid spec than
> setting/getting the life time days/weeks/months.

Expiration is unimplemented and the policy still needs to be flushed out.

Comment 11

[Brian Ryner (not reading)]

Comment on attachment 202424 [details] [diff] [review]
Preliminary annotation service interface and implementation

>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ components/places/public/nsIAnnotationService.idl	9 Nov 2005 22:11:51 -0000
>@@ -0,0 +1,208 @@
>+#include "nsISupports.idl"
>+#include "nsIURI.idl"
>+#include "nsIVariant.idl"

nsIURI and nsIVariant should just be forward-declared.

>+    const long EXPIRE_MONTHS = 3;
>+
>+      // For small, user-entered data like notes that should never expire.
>+    const long EXPIRE_NEVER = 5;

Is 4 bad luck?

This interface uses an odd mish-mash of |long| and |PRInt32| to refer to 32-bit signed ints.  Pick one.

>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ components/places/src/nsAnnotationService.cpp	9 Nov 2005 22:11:51 -0000
>+const int nsAnnotationService::kAnnoIndex_ID = 0;
>+const int nsAnnotationService::kAnnoIndex_Page = 1;
>+const int nsAnnotationService::kAnnoIndex_Name = 2;
>+const int nsAnnotationService::kAnnoIndex_MimeType = 3;
>+const int nsAnnotationService::kAnnoIndex_Content = 4;
>+const int nsAnnotationService::kAnnoIndex_Flags = 5;
>+const int nsAnnotationService::kAnnoIndex_Expiration = 6;

PRInt32

>+nsAnnotationService::SetAnnotation(nsIURI* aURI,
>+                                   const nsACString& aName,
>+                                   nsIVariant *aValue,
>+                                   PRInt32 aFlags, PRInt32 aExpiration)
>+{
>+  if (! aValue)
>+    return NS_ERROR_INVALID_ARG;
>+  nsString stringValue;

nsAutoString

>+NS_IMETHODIMP
>+nsAnnotationService::SetAnnotationString(nsIURI* aURI,
>+                                         const nsACString& aName,
>+                                         const nsAString& aValue,
>+                                         PRInt32 aFlags, PRInt32 aExpiration)
>+{
>+  mozStorageTransaction transaction(mDBConn, PR_FALSE);
>+  mozIStorageStatement* statement; // class var, not owned by this function
>+  nsresult rv = StartSetAnnotation(aURI, aName, aFlags, aExpiration, &statement);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  rv = statement->BindStringParameter(kAnnoIndex_Content, aValue);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  rv = statement->BindNullParameter(kAnnoIndex_MimeType);
>+  NS_ENSURE_SUCCESS(rv, rv);

The statement will be left open if either of these fail.  Likewise in the other SetAnnotation* methods.

>+NS_IMETHODIMP
>+nsAnnotationService::GetAnnotation(nsIURI* aURI,
>+                                   const nsACString& aName,
>+                                   nsIVariant** _retval)
>+{
>+  nsString stringValue;

nsAutoString

>+  nsresult rv = GetAnnotationString(aURI, aName, stringValue);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<nsIWritableVariant> var = do_CreateInstance("@mozilla.org/variant;1");
>+  if (! var)
>+    return NS_ERROR_FAILURE;

You might want to pass &rv to do_CreateInstance and return that error code.

>+  rv = var->SetWritable(PR_TRUE);
>+  NS_ENSURE_SUCCESS(rv, rv);

I don't think this is necessary.

>+  rv = var->SetAsAString(stringValue);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  *_retval = var;
>+  (*_retval)->AddRef();

NS_ADDREF.

>+// nsAnnotationService::GetAnnotationInt32
>+
>+NS_IMETHODIMP
>+nsAnnotationService::GetAnnotationInt32(nsIURI* aURI,
>+                                        const nsACString& aName,
>+                                        PRInt32 *_retval)
>+{
>+  nsresult rv = StartGetAnnotationFromURI(aURI, aName);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  rv = mDBGetAnnotationFromURI->GetAsInt32(kAnnoIndex_Content, _retval);
>+  mDBGetAnnotationFromURI->Reset();
>+  return rv;
>+}

It's more or less implied by the API that GetAsInt32 won't fail, given the presence of AsInt32().  So, maybe just:

*_retval = mDBGetAnnotationFromURI->GetAsInt32(kAnnoIndex_Content);
mDBGetAnnotationFromURI->Reset();
return NS_OK;

>+NS_IMETHODIMP
>+nsAnnotationService::GetAnnotationInfo(nsIURI* aURI,
>+                                       const nsACString& aName,
>+                                       PRInt32 *aFlags, PRInt32 *aExpiration,
>+                                       nsACString& aMimeType,
>+                                       PRInt32 *aStorageType)
>+{
>+  nsresult rv = StartGetAnnotationFromURI(aURI, aName);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  mozStorageStatementScoper resetter(mDBGetAnnotationFromURI);
>+
>+  rv = mDBGetAnnotationFromURI->GetAsInt32(kAnnoIndex_Flags, aFlags);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  rv = mDBGetAnnotationFromURI->GetAsInt32(kAnnoIndex_Expiration, aExpiration);
>+  NS_ENSURE_SUCCESS(rv, rv);

Same here, use AsInt32().

>+// nsAnnotationService::StartSetAnnotation
>+//
>+//    This does most of the work of setting an annotation, except for setting
>+//    the actual value and MIME type and executing the statement. It will
>+//    create a URL entry if necessary. It will either update an existing
>+//    annotation or insert a new one, and aStatement will be set to either
>+//    mDBAddAnnotation or mDBSetAnnotation. The aStatement RESULT IS NOT
>+//    ADDREFED. This is just one of the class vars, which control it's scope.

s/it's/its/

>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ components/places/src/nsAnnotationService.h	9 Nov 2005 22:11:51 -0000
>+class nsAnnotationService : public nsIAnnotationService
>+{
>+public:
>+  nsAnnotationService();
>+  virtual ~nsAnnotationService();

doesn't need to be virtual or public.

>--- components/places/src/nsNavHistory.cpp	9 Nov 2005 19:19:33 -0000	1.2
>+++ components/places/src/nsNavHistory.cpp	9 Nov 2005 22:11:51 -0000
> nsNavHistory::~nsNavHistory()
> {
>   gObserverService->RemoveObserver(this, gQuitApplicationMessage);
>+
>+  // remove the static reference to the service. Check to make sure its us
>+  // in case somebody creates an extra instance of the service.
>+  if (gHistoryService == this)
>+    gHistoryService = nsnull;

This should just assert, like the constructor.

>@@ -423,16 +426,59 @@ nsNavHistory::InitMemDB()
>   }
>   transaction.Commit();
> 
>   printf("DONE initializing history in-memory DB\n");
>   return NS_OK;
> }
> 
> 
>+// nsNavHistory::GetUrlIdFor
>+//
>+//    Called by the bookmarks and annotation servii, this function returns the

Hm, let's just go with "services."

>--- components/places/src/nsNavHistory.h	9 Nov 2005 19:19:33 -0000	1.2
>+++ components/places/src/nsNavHistory.h	9 Nov 2005 22:11:51 -0000
>@@ -55,16 +55,17 @@
> #include "nsIPrefService.h"
> #include "nsIPrefBranch.h"
> #include "nsIObserver.h"
> #include "nsIObserverService.h"
> #include "nsIStringBundle.h"
> #include "nsITimer.h"
> #include "nsITreeSelection.h"
> #include "nsITreeView.h"
>+#include "nsServiceManagerUtils.h"
> #include "nsString.h"
> #include "nsVoidArray.h"
> #include "nsWeakReference.h"
> 
> // Number of prefixes used in the autocomplete sort comparison function
> #define AUTOCOMPLETE_PREFIX_LIST_COUNT 6
> // Size of visit count boost to give to urls which are sites or paths
> #define AUTOCOMPLETE_NONPAGE_VISIT_COUNT_BOOST 5
>@@ -263,45 +264,86 @@ protected:
>       nsNavHistoryResultNode* a, nsNavHistoryResultNode* b, void* closure);
>   PR_STATIC_CALLBACK(int) SortComparison_VisitCountLess(
>       nsNavHistoryResultNode* a, nsNavHistoryResultNode* b, void* closure);
>   PR_STATIC_CALLBACK(int) SortComparison_VisitCountGreater(
>       nsNavHistoryResultNode* a, nsNavHistoryResultNode* b, void* closure);
> };
> 
> 
>+class AutoCompleteIntermediateResultSet;
> 
> // nsNavHistory
> 
> class nsNavHistory : nsSupportsWeakReference,
>                      public nsINavHistory,
>                      public nsIObserver,
>                      public nsIBrowserHistory,
>                      public nsIAutoCompleteSearch
> {
>-  friend struct AutoCompleteIntermediateResultSet;
>+  friend class AutoCompleteIntermediateResultSet;
> public:
>   nsNavHistory();
> 
>   NS_DECL_ISUPPORTS
> 
>   NS_DECL_NSINAVHISTORY
>   NS_DECL_NSIGLOBALHISTORY2
>   NS_DECL_NSIBROWSERHISTORY
>   NS_DECL_NSIOBSERVER
>   NS_DECL_NSIAUTOCOMPLETESEARCH
> 
>   nsresult Init();
> 
>+  /**
>+   * Used by other components in the places directory such as the annotation
>+   * service to get a reference to this history object. Returns a pointer to
>+   * the service if it exists. Otherwise creates one. Returns NULL on error.
>+   */
>+  static nsNavHistory* GetHistoryService()
>+  {
>+    if (gHistoryService)
>+      return gHistoryService;

I'd do this the other way, I think:

if (!gHistoryService) {
  do_GetService();
}
return gHistoryService;

It's less code and I think it agrees better with the way branch prediction works on most cpus.

sr=me with those changes.

Comment 12

[Brett Wilson]

Attachment: Add annotation observers and moz-anno URI generator

This also includes some cleanup of the interface: the integer type has been standardized on PRInt32.

Comment 13

[Brett Wilson]

Basic implementation is on trunk in the places system (off by default). It needs many improvements: I'll make those separate bugs.

Comment 14

[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