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

RESOLVED FIXED in Firefox 6

Status

()

Firefox
Bookmarks & History
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: rnewman, Assigned: mak)

Tracking

Trunk
Firefox 6
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [places-next-wanted][fixed-in-places])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

7 years ago
* 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?
(Assignee)

Comment 1

7 years ago
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.
(Assignee)

Comment 2

7 years ago
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
(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.
(Reporter)

Comment 4

7 years ago
Note that this also affects sync/guid on 3.x: Bug 628788 works around this.
Summary: Smart Bookmarks take their Places/SmartBookmark anno with them when copied → Copied bookmarks shouldn't clone parent annos
(Assignee)

Updated

6 years ago
Whiteboard: [places-next-wanted]
(Assignee)

Updated

6 years ago
Assignee: nobody → mak77
Status: NEW → ASSIGNED
(Assignee)

Updated

6 years ago
OS: Mac OS X → All
Hardware: x86_64 → All
(Assignee)

Comment 5

6 years ago
Created attachment 533142 [details] [diff] [review]
patch v1.0

saving work, needs a test still.
(Assignee)

Comment 6

6 years ago
Created attachment 533298 [details] [diff] [review]
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.
Attachment #533142 - Attachment is obsolete: true
Attachment #533298 - Flags: review?(dietrich)
Attachment #533298 - Flags: feedback?(philipp)
(Assignee)

Updated

6 years ago
Flags: in-testsuite?
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?
Attachment #533298 - Flags: review?(dietrich) → review+
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.
Attachment #533298 - Flags: feedback?(philipp) → feedback-
(Assignee)

Comment 9

6 years ago
> @@ +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.
(Assignee)

Updated

6 years ago
Summary: Copied bookmarks shouldn't clone parent annos → Copied bookmarks shouldn't inherit all annotations, since they are new entitities.
(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 :)
(Assignee)

Comment 11

6 years ago
Created attachment 533421 [details] [diff] [review]
patch v1.2
Attachment #533298 - Attachment is obsolete: true
(Assignee)

Comment 12

6 years ago
http://hg.mozilla.org/projects/places/rev/77083dd59380
Whiteboard: [places-next-wanted] → [places-next-wanted][fixed-in-places]
(Assignee)

Comment 13

6 years ago
http://hg.mozilla.org/mozilla-central/rev/77083dd59380
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 6
(Assignee)

Comment 14

6 years ago
Created attachment 534668 [details] [diff] [review]
add test to xpcshell.ini
Comment on attachment 534668 [details] [diff] [review]
add test to xpcshell.ini

landed: http://hg.mozilla.org/mozilla-central/rev/8d63b3ef2593
Blocks: 755758
You need to log in before you can comment on or make changes to this bug.