It's time for binary annotations to go. We don't use them at all. "New Tab JumpStart" is the only addon that uses them. It does so for storing thumbnails for a "new tab page" implementation (it precedes the built-in feature, I think). It could easily adopt the Thumbnails service.

we should reach them through mail, and politely ask if they can move out of binary annotations in the next 12 weeks. There are various options for them, Thumbnails service (preferred), but also string annotations with files in a profile folder.

The actual reasoning about the removal it's also that they complicate quite a bit the code for the new transactions manager, and since we don't need them it's a pointless cost.

Mail sent to the developer of New Tab JumpStart.

Status: Accepted Owner: mlalevic [[at]] gmail Labels: -Priority-Medium Priority-Critical

Comment on attachment 778490 [details] [diff] [review] built, tested patch Review of attachment 778490 [details] [diff] [review]: ----------------------------------------------------------------- <3 ::: toolkit/components/places/PlacesUtils.jsm @@ +826,5 @@ > */ > setAnnotationsForURI: function PU_setAnnotationsForURI(aURI, aAnnos) { > var annosvc = this.annotations; > aAnnos.forEach(function(anno) { > + if (!("value" in anno)) { as discussed over irc, we should allow both undefined and null values to clear the anno, I think here you want if (anno.value == null) That's also to reduce eventual API breakage since this is an API behavioral change. please add a test to verify we can set a 0 valued annotation, but null and undefined properly remove the annotation. @@ +850,5 @@ > setAnnotationsForItem: function PU_setAnnotationsForItem(aItemId, aAnnos) { > var annosvc = this.annotations; > > aAnnos.forEach(function(anno) { > + if (!("value" in anno)) { ditto ::: toolkit/components/places/nsAnnoProtocolHandler.cpp @@ +10,4 @@ > * > + * The reference to annoations ("moz-anno") is a leftover from previous > + * iterations of this component. As of now the moz-anno protocol is independent > + * of annoations. typo: annoations ::: toolkit/components/places/nsNavHistoryResult.cpp @@ +1146,4 @@ > } \ > } > > + if (annoType == nsIAnnotationService::TYPE_STRING) { nit: could be converted to a switch ::: toolkit/components/places/tests/unit/test_annotations.js @@ +15,4 @@ > try { > var annosvc= Cc["@mozilla.org/browser/annotation-service;1"].getService(Ci.nsIAnnotationService); > } catch(ex) { > + dunp("Ex: " + ex); typo: dunp... this makes me think this test cannot pass :) @@ +53,4 @@ > // main > function run_test() > { > + dump("++\n") looks like you added a bunch of debug spew in this test, if you think it's worth to have some additional info you should add it properly, otherwise just clean it up please

I'll file a separate bug for testing [get/set]AnnotationsFor[URI/Item] in general. This API is not tested at all and it doesn't make sense to test just this case.

> typo: dunp... this makes me think this test cannot pass :) It actually did because the catch block is the failure case.

Actually, there's toolkit/components/places/tests/unit/test_utils_setAnnotationsFor.js. I'm not sure how I didn't find that at first.

and also added a test for annos removal. I didn't use != null as I prefer being explicit about the input (=== undefined || === null).

I forgot to fix that typo. Will do that on checkin. By the way, I added tests for both removal and 0-value.

Comment on attachment 782181 [details] [diff] [review] fixed this and that Review of attachment 782181 [details] [diff] [review]: ----------------------------------------------------------------- it looks good. Please file a follow-up bug to add a maintenance task that will kill any existing binary annotation. ::: toolkit/components/places/tests/unit/test_utils_setAnnotationsFor.js @@ +34,5 @@ > value: "test2", > + expires: Ci.nsIAnnotationService.EXPIRE_NEVER }, > + { name: "testAnno/test3", > + flags: 0, > + value: 5, did you mean value: 0 here?

Oh yeah. Not sure what happened! Same goes for review->feedback, right? :)

well, you didn't ask for review so I gave feedback :p The previous review is still valid.

Can you give me a high-level overview of what this is doing, and what in particular you're asking me to SR?

Annotations on pages or bookmarks, can be different types: integer, string, double... and binary content. The binary annotations have always been used very rarely, and the times someone used them they were abused to store hundreds MBs of data (See old Google Toolbar). Moreover, since they are binary data, the code must be more complex due to their existence to handle mime-types content length and such. This bug is to completely remove the binary annotations feature, the scope is to simplify porting the synchronous transaction manager to an asynchronous version, dropping things we really don't need and that would complicate the code porting. Regardless, we were planning from quite some time to remove them. Moreover in future we'll completely remove annotations, but those require a bit more attention, since they are more used in add-ons and code. The SR is to approve the removal of the feature, we only found one add-on using them, and already filed a bug in their tracker and got ACK from its author.

What Marco Said. There are three API changes in this patch, which I'm asking for SR on: 1. Binary annotations removal. 2. getAnnotationURI removal, which was only useful for binary annotations. 3. Simplifying the API for PlacesUtils.[get/set]AnnoatisonsFor[Item/URI]. As Marco said, these changes only affect a single addon, and we've already notified its developer about that.

Oops, didn't mean to close it.