Closed Bug 895839 Opened 11 years ago Closed 11 years ago

Remove support for binary annotations

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: asaf, Assigned: asaf)

References

Details

(Keywords: addon-compat, dev-doc-needed)

Attachments

(1 file, 3 obsolete files)

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.
Keywords: addon-compat
OS: Mac OS X → All
Hardware: x86 → All
Blocks: 891303
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.
Attached patch untested, unbuilt patch (obsolete) — Splinter Review
Attached patch built, tested patch (obsolete) — Splinter Review
Mail sent to the developer of New Tab JumpStart.
Attachment #778423 - Attachment is obsolete: true
Attachment #778490 - Flags: review?(mak77)
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
Attachment #778490 - Flags: review?(mak77) → review+
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.
Attached patch fixed this and that (obsolete) — Splinter Review
and also added a test for annos removal.

I didn't use != null as I prefer being explicit about the input (=== undefined || === null).
Attachment #778490 - Attachment is obsolete: true
Attachment #782181 - Flags: superreview?(gavin.sharp)
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?
Attachment #782181 - Flags: feedback+
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.
Attachment #782181 - Flags: superreview?(gavin.sharp) → superreview+
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Keywords: dev-doc-needed
Oops, didn't mean to close it.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → NEW
Blocks: 901440
https://hg.mozilla.org/mozilla-central/rev/714a27789e9e
Status: NEW → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Depends on: 912855
Blocks: 691259
You need to log in before you can comment on or make changes to this bug.