331654 - Provide a way to handle annotations values in a type-agnostic way in JavaScript

Status: VERIFIED FIXED

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

Description

[Joe Hughes]

Right now, there's no way to deal with an annotation in a type-agnostic way, and therefore there's no way for me to "back up" all of a URI's annotations from JavaScript (for later restoring).  I need this ability in order to write a proper "undo" function for nsINavBookmarksService::changeBookmarkURI().

Comment 1

[Joe Hughes]

Ah, we used to have this, but Brett removed it in this change:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsAnnotationService.cpp&branch=MOZILLA_1_8_BRANCH&root=/cvsroot&subdir=mozilla/browser/components/places/src&command=DIFF_FRAMESET&rev1=1.7.2.2&rev2=1.7.2.3

I'm going to add this back in so that we can cache sets of annotations for undo.

Comment 2

[Asaf Romano (gone)]

Attachment: patch

next: 1) make int64 getter work for int32 annotation 2) make double getter work for int64 annotations.

Comment 3

[Dietrich Ayala (:dietrich)]

Comment on attachment 269794 [details] [diff] [review]
patch

Index: toolkit/components/places/public/nsIAnnotationService.idl
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/components/places/public/nsIAnnotationService.idl,v
>retrieving revision 1.14
>diff -u -p -8 -r1.14 nsIAnnotationService.idl
>--- toolkit/components/places/public/nsIAnnotationService.idl	10 May 2007 08:05:19 -0000	1.14
>+++ toolkit/components/places/public/nsIAnnotationService.idl	26 Jun 2007 03:36:50 -0000
>@@ -109,69 +109,81 @@ interface nsIAnnotationService : nsISupp
>      *
>      * Do not use characters that are not valid in URLs such as spaces, ":",
>      * commas, or most other symbols. You should stick to ASCII letters and
>      * numbers plus "_", "-", and "/".
>      *
>      * aExpiration is one of EXPIRE_* above. aFlags should be 0 for now, some
>      * flags will be defined in the future.
>      *
>-     * NOTE: ALL ANNOTATIONS WILL GET DELETED WHEN THE PAGE IS REMOVED FROM
>-     * HISTORY, regardless of expiration date. This means that if you create an
>+     * NOTE (NOTE: NOT REALLY!),: ALL ANNOTATIONS WILL GET DELETED WHEN THE
>+     * PAGE IS REMOVED FROM HISTORY,
>+     * regardless of expiration date. This means that if you create an

i'd prefer a useful comment that points towards the expiration bug, and explains that it's not fixed yet, or no change at all.

>@@ -180,81 +192,81 @@ interface nsIAnnotationService : nsISupp
>     void setItemAnnotationBinary(in long long aItemId, in AUTF8String aName,
>                                  [const,array,size_is(aDataLen)] in octet aData,
>                                  in unsigned long aDataLen,
>                                  in AUTF8String aMimeType,
>                                  in long aFlags,
>                                  in unsigned short aExpiration);
> 
>     /**
>-     * Retrieves the string value of a given annotation. Throws an error if the
>-     * annotation does not exist. If the annotation was set as a different
>-     * type than you are retrieving it as, the value will be converted as best
>-     * as we can. You aren't always guaranteed a good conversion, however,
>-     * and errors will not be thrown in this case. (For example, doubles will
>-     * lose precision when stringified.)
>+     * Retrieves the value of a given annotation. Throws an error if the
>+     * annotation does not exist. Throws for binary annotations, for which
>+     * getPageAnnotationBinary/getItemAnnotationBinary should be used. C++
>+     * consumers may use the type-specific methods.

nit: s/may/should/ ?

>-    void getPageAnnotationInfo(in nsIURI aURI, in AUTF8String aName,
>-                           out PRInt32 aFlags, out unsigned short aExpiration,
>-                           out AUTF8String aMimeType,
>-                           out unsigned short aType);
>+    void getPageAnnotationInfo(in nsIURI aURI,
>+                                          in AUTF8String aName,
>+                                          out PRInt32 aFlags,
>+                                          out unsigned short aExpiration,
>+                                          out AUTF8String aMimeType,
>+                                          out unsigned short aType);

nit: indent issue

>+++ toolkit/components/places/src/nsAnnotationService.cpp	26 Jun 2007 03:36:56 -0000
>@@ -44,16 +44,17 @@
> #include "mozIStorageValueArray.h"
> #include "mozIStorageStatement.h"
> #include "mozIStorageFunction.h"
> #include "mozStorageHelper.h"
> #include "nsIServiceManager.h"
> #include "nsIVariant.h"
> #include "nsString.h"
> #include "nsVariant.h"
>+#include "nsIVariant.h"
> #include "nsNavBookmarks.h"

first, nsIVariant.h is included twice. second, do you need to both the interface and the implementation header files?

> 
>+NS_IMETHODIMP
>+nsAnnotationService::GetPageAnnotation(nsIURI* aURI,
>+                                       const nsACString& aName,
>+                                       nsIVariant** _retval)
>+{
>+  nsresult rv = StartGetAnnotationFromURI(aURI, aName);
>+  if (NS_FAILED(rv))
>+    return rv;
>+
>+  nsCOMPtr<nsIWritableVariant> value = new nsVariant();
>+  PRInt32 type = mDBGetAnnotationFromURI->AsInt32(kAnnoIndex_Type);
>+  switch (type) {
>+    case nsIAnnotationService::TYPE_INT32:
>+    case nsIAnnotationService::TYPE_INT64:
>+    case nsIAnnotationService::TYPE_DOUBLE: {
>+      rv = value->SetAsDouble(mDBGetAnnotationFromURI->AsDouble(kAnnoIndex_Content));
>+      break;
>+    }
>+    case nsIAnnotationService::TYPE_STRING: {
>+      printf("\nstring anno return 1\n");
>+      nsAutoString valueString;
>+      rv = mDBGetAnnotationFromURI->GetString(kAnnoIndex_Content, valueString);
>+      if (NS_SUCCEEDED(rv)) {
>+        printf("\nstring anno return 2\n");
>+        printf("%s\n", NS_LossyConvertUTF16toASCII(valueString).get());
>+        rv = value->SetAsAString(valueString);
>+      }
>+      break;
>+    }
>+    case nsIAnnotationService::TYPE_BINARY: {
>+      rv = NS_ERROR_INVALID_ARG;
>+      break;
>+    }
>+    default: {
>+      rv = NS_ERROR_UNEXPECTED;
>+      break;
>+    }
>+  }
>+
>+  if (NS_SUCCEEDED(rv)) {
>+    NS_ADDREF(*_retval = value);
>+    printf("\nstring anno return 3\n");
>+  }
>+  else
>+    *_retval = nsnull;
>+
>+  mDBGetAnnotationFromURI->Reset();
>+  return rv;
>+}

please remove the printf() lines.

>@@ -980,16 +980,17 @@ PRInt32 PR_CALLBACK nsNavHistoryContaine
>         NS_ENSURE_SUCCESS(annosvc->GetItemAnnotationType(b->mItemId,
>                                                          annoName,
>                                                          &b_type), 0);
>       } else {
>         NS_ENSURE_SUCCESS(annosvc->GetPageAnnotationType(b_uri, annoName,
>                                                          &b_type), 0);
>       }
>       // We better make the API not support this state, really
>+      // XXXmano: this is actually wrong for double<->int and in64<->int32
>       if (a_hasAnno && b_type != annoType)
>         return 0;
>       annoType = b_type;

s/in64/int64/

can you file a followup if this is deserving of it?

>   // test int64 anno type
>   var int64Key = testAnnoName + "/types/Int64";
>   var int64Val = 4294967296;
>-  annosvc.setPageAnnotationInt64(testURI, int64Key, int64Val, 0, 0);
>+  annosvc.setPageAnnotation(testURI, int64Key, int64Val, 0, 0);
>   annosvc.getPageAnnotationInfo(testURI, int64Key, flags, exp, mimeType, storageType);
>   do_check_eq(flags.value, 0);
>   do_check_eq(exp.value, 0);
>   do_check_eq(mimeType.value, null);
>-  do_check_eq(storageType.value, Ci.nsIAnnotationService.TYPE_INT64);

why did you remove the storageType tests here and below?

>+  // test that binay-accessors throw for wrong types

s/binay/binary

r=me, given these changes. sorry for the review delay. also, this breaks FUEL 0.2 in bug 380168. please comment on that bug so that mark is aware of these changes.

Comment 4

[Asaf Romano (gone)]

(In reply to comment #3)

> >-  do_check_eq(storageType.value, Ci.nsIAnnotationService.TYPE_INT64);
> 
> why did you remove the storageType tests here and below?

because you can no longer rely on it, xpconnect may choose "double" as the variant type in this case, for example.

> nit: s/may/should/ ?

"may", the variant-variants can be used in C++ context as well.

Comment 5

[Asaf Romano (gone)]

Attachment: as checked in

mozilla/browser/components/microsummaries/src/nsMicrosummaryService.js 1.66
mozilla/browser/components/places/content/bookmarkProperties.js 1.53
mozilla/browser/components/places/content/controller.js 1.165
mozilla/browser/components/places/content/toolbar.xml 1.98
mozilla/browser/components/places/content/treeView.js 1.13
mozilla/browser/components/places/content/utils.js 1.47
mozilla/browser/components/places/tests/unit/test_bookmarks_html.js 1.11
mozilla/toolkit/components/places/public/nsIAnnotationService.idl 1.15
mozilla/toolkit/components/places/src/nsAnnotationService.cpp 1.28
mozilla/toolkit/components/places/src/nsLivemarkService.js .19
mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp 1.106
mozilla/toolkit/components/places/tests/bookmarks/test_bookmarks.js 1.30
mozilla/toolkit/components/places/tests/unit/test_annotations.js 1.10
mozilla/toolkit/components/places/tests/unit/test_history.js 1.5
mozilla/toolkit/components/places/tests/unit/test_result_sort.js 1.6

Comment 6

[Tracy Walker [:tracy]]

Verified with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7pre) Gecko/2007071705 Minefield/3.0a7pre

Comment 7

[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