629620 - Copied bookmarks shouldn't inherit all annotations, since they are new entitities.

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Richard Newman [:rnewman]]

* New profile, 2011-01-27 Minefield build
* Open "Show all Bookmarks"
* Option-drag "Most Visited" into "Bookmarks Menu" to create a copy.
* Quit and reopen Minefield. (Just making sure that DBs are flushed; probably unnecessary.)
* In a privileged web console:

  Components.utils.import("resource://services-sync/util.js");
  Svc.Annos.getItemsWithAnnotation("Places/SmartBookmark", {})
    .map(function(id) [id, Utils.anno(id, "Places/SmartBookmark")]);


The latter will print something like:

  13,MostVisited,14,RecentlyBookmarked,15,RecentTags,105,MostVisited

In other words: duplicating "Most Visited" resulted in two separate records, with different IDs, both with the same anno.

In Bug 610501 philiKON suggested that this behavior seemed undesirable and unpredictable; that these Smart Bookmark annotations should be unique.

Other annos being copied have also caused us grief -- old Sync GUIDs, for example. Perhaps only a whitelisted set of annos should be copied with a bookmark?

Is this anno copying intentional?

Comment 1

[Marco Bonardo [:mak]]

no it is not, actually I don't think that we should whitelist, a blacklist seems better since we can't know which annotations an add-on could add to elements.

Comment 2

[Marco Bonardo [:mak]]

hm, even if by copying we are creating a new element, so maybe in this case you're right. probably we should only copy the description

Comment 3

[Philipp von Weitershausen [:philikon]]

(In reply to comment #2)
> hm, even if by copying we are creating a new element, so maybe in this case
> you're right. probably we should only copy the description

That's indeed what I would propose we do.

Comment 4

[Richard Newman [:rnewman]]

Note that this also affects sync/guid on 3.x: Bug 628788 works around this.

Comment 5

[Marco Bonardo [:mak]]

Attachment: patch v1.0

saving work, needs a test still.

Comment 6

[Marco Bonardo [:mak]]

Attachment: patch v1.1

This includes a test (A boring one).

As it is, this will only copy the following annotations: description, load in sidebar, post data, read only.
Any other annotation is ignored (apart feedURI and siteURI on livemarks that are copied differently).

Reasonable?

PS: philipp, the Sync test change needs review from your side too.

Comment 7

[Dietrich Ayala (:dietrich)]

Comment on attachment 533298 [details] [diff] [review]
patch v1.1

Review of attachment 533298 [details] [diff] [review]:
-----------------------------------------------------------------

r=me w/ these fixed, thanks!

::: browser/components/places/src/PlacesUIUtils.jsm
@@ +96,5 @@
>      return bundle.GetStringFromName(key);
>    },
>  
>    /**
> +   * Get a transaction for copying a uri item from one container to another.

in the comment, clarify what you mean by URI item.

@@ +107,5 @@
>     *          The index within the container the item is copied to
> +   * @return A nsITransaction object that performs the copy.
> +   *
> +   * @note Since a copy creates a completely new item, only some internal
> +   *       annotations are synced from the old one.

if there's not a const or explicit list somewhere to point at, should at least point at the code that makes that determination, eg "see blah.js for details".

@@ +133,5 @@
> +                 this.LOAD_IN_SIDEBAR_ANNO,
> +                 PlacesUtils.POST_DATA_ANNO,
> +                 PlacesUtils.READ_ONLY_ANNO,
> +               ].indexOf(aAnno.name) != -1;
> +      }, this);

Yeah, per above comment, should actually break this list out into a property i think, so it's easier to track down.

@@ +313,3 @@
>          break;
>        case PlacesUtils.TYPE_X_MOZ_PLACE:
> +        if (copy || data.id == -1) { // Id is -1 if the place is not bookmarked.

update -1 to the const while there?

Comment 8

[Philipp von Weitershausen [:philikon]]

Comment on attachment 533298 [details] [diff] [review]
patch v1.1

> function test_copying_places() {
> 
>   // Gecko <2.0
>   if (store._haveGUIDColumn) {
>     _("We have a GUID column; not testing anno GUID fixing.");
>     return;
>   }

This whole test should be defunct in Gecko 2.0+. I ripped out store._haveGUIDColumn in bug 648338, but apparently forgot to delete this test (I'm amazed it passes!). Please just get rid of the whole function. r=me for that.

Comment 9

[Marco Bonardo [:mak]]

> @@ +313,3 @@
> >          break;
> >        case PlacesUtils.TYPE_X_MOZ_PLACE:
> > +        if (copy || data.id == -1) { // Id is -1 if the place is not bookmarked.
> 
> update -1 to the const while there?

which const? This is not the index, it's the magic number for no item id (not bookmarked), that has never been consistent ( somewhere we use 0, somewhere we use -1 :( ), if we want to move it to a NO_ID const should probably be done elsewhere with a global check for consistency.

Comment 10

[Philipp von Weitershausen [:philikon]]

(In reply to comment #8)
> This whole test should be defunct in Gecko 2.0+. I ripped out
> store._haveGUIDColumn in bug 648338, but apparently forgot to delete this
> test (I'm amazed it passes!). Please just get rid of the whole function.
> r=me for that.

Actually, I removed this function now in a follow-up patch for bug 654900, so if you just leave it alone, the problem will fix itself :)

Comment 11

[Marco Bonardo [:mak]]

Attachment: patch v1.2

Comment 12

[Marco Bonardo [:mak]]

http://hg.mozilla.org/projects/places/rev/77083dd59380

Comment 13

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/77083dd59380

Comment 14

[Marco Bonardo [:mak]]

Attachment: add test to xpcshell.ini

Comment 15

[Philipp von Weitershausen [:philikon]]

Comment on attachment 534668 [details] [diff] [review]
add test to xpcshell.ini

landed: http://hg.mozilla.org/mozilla-central/rev/8d63b3ef2593