Last Comment Bug 895839 - Remove support for binary annotations
: Remove support for binary annotations
Status: RESOLVED FIXED
: addon-compat, dev-doc-needed
Product: Toolkit
Classification: Components
Component: Places (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla25
Assigned To: Mano (::mano, needinfo? for any questions; not reading general bugmail)
:
Mentors:
Depends on: 912855
Blocks: 691259 891303 901440
  Show dependency treegraph
 
Reported: 2013-07-19 04:12 PDT by Mano (::mano, needinfo? for any questions; not reading general bugmail)
Modified: 2015-03-12 02:28 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
untested, unbuilt patch (38.75 KB, patch)
2013-07-19 04:55 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Splinter Review
built, tested patch (46.99 KB, patch)
2013-07-19 08:39 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
mak77: review+
Details | Diff | Splinter Review
fixed this and that (65.74 KB, patch)
2013-07-27 11:22 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
gavin.sharp: superreview+
mak77: feedback+
Details | Diff | Splinter Review
cheked in (65.74 KB, patch)
2013-08-03 04:07 PDT, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Splinter Review

Description Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-07-19 04:12:46 PDT
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.
Comment 1 Marco Bonardo [::mak] 2013-07-19 04:52:13 PDT
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.
Comment 2 Marco Bonardo [::mak] 2013-07-19 04:54:02 PDT
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.
Comment 3 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-07-19 04:55:50 PDT
Created attachment 778423 [details] [diff] [review]
untested, unbuilt patch
Comment 4 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-07-19 08:39:09 PDT
Created attachment 778490 [details] [diff] [review]
built, tested patch

Mail sent to the developer of New Tab JumpStart.
Comment 5 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-07-22 12:11:53 PDT
http://code.google.com/p/new-tab-jumpstart/issues/detail?id=162
Comment 6 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-07-22 20:14:24 PDT
Status: Accepted
Owner: mlalevic [[at]] gmail
Labels: -Priority-Medium Priority-Critical
Comment 7 Marco Bonardo [::mak] 2013-07-26 05:14:58 PDT
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
Comment 8 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-07-27 10:56:59 PDT
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.
Comment 9 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-07-27 10:59:34 PDT
> typo: dunp... this makes me think this test cannot pass :)

It actually did because the catch block is the failure case.
Comment 10 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-07-27 11:04:30 PDT
Actually, there's toolkit/components/places/tests/unit/test_utils_setAnnotationsFor.js. I'm not sure how I didn't find that at first.
Comment 11 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-07-27 11:22:22 PDT
Created attachment 782181 [details] [diff] [review]
fixed this and that

and also added a test for annos removal.

I didn't use != null as I prefer being explicit about the input (=== undefined || === null).
Comment 12 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-07-28 03:09:12 PDT
I forgot to fix that typo. Will do that on checkin.

By the way, I added tests for both removal and 0-value.
Comment 13 Marco Bonardo [::mak] 2013-07-29 03:19:02 PDT
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?
Comment 14 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-07-29 04:49:50 PDT
Oh yeah. Not sure what happened! Same goes for review->feedback, right? :)
Comment 15 Marco Bonardo [::mak] 2013-07-29 10:02:48 PDT
well, you didn't ask for review so I gave feedback :p The previous review is still valid.
Comment 16 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-07-29 14:24:59 PDT
Can you give me a high-level overview of what this is doing, and what in particular you're asking me to SR?
Comment 17 Marco Bonardo [::mak] 2013-07-29 14:32:31 PDT
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.
Comment 18 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-07-29 22:05:48 PDT
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.
Comment 19 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-08-03 04:07:50 PDT
Created attachment 785376 [details] [diff] [review]
cheked in

http://hg.mozilla.org/integration/mozilla-inbound/rev/714a27789e9e
Comment 20 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2013-08-03 04:10:57 PDT
Oops, didn't mean to close it.
Comment 21 Carsten Book [:Tomcat] 2013-08-05 03:13:10 PDT
https://hg.mozilla.org/mozilla-central/rev/714a27789e9e

Note You need to log in before you can comment on or make changes to this bug.