Last Comment Bug 629620 - Copied bookmarks shouldn't inherit all annotations, since they are new entitities.
: Copied bookmarks shouldn't inherit all annotations, since they are new entiti...
Status: RESOLVED FIXED
[places-next-wanted][fixed-in-places]
:
Product: Firefox
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 6
Assigned To: Marco Bonardo [::mak]
:
: Marco Bonardo [::mak]
Mentors:
Depends on:
Blocks: 755758
  Show dependency treegraph
 
Reported: 2011-01-28 00:45 PST by Richard Newman [:rnewman]
Modified: 2012-05-16 08:17 PDT (History)
2 users (show)
mak77: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1.0 (18.11 KB, patch)
2011-05-17 18:10 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
patch v1.1 (34.31 KB, patch)
2011-05-18 08:51 PDT, Marco Bonardo [::mak]
dietrich: review+
philipp: feedback-
Details | Diff | Splinter Review
patch v1.2 (27.73 KB, patch)
2011-05-18 14:23 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
add test to xpcshell.ini (572 bytes, patch)
2011-05-23 21:11 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review

Description Richard Newman [:rnewman] 2011-01-28 00:45:30 PST
* 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] 2011-01-28 04:06:08 PST
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] 2011-01-28 04:28:00 PST
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] 2011-01-28 07:04:45 PST
(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] 2011-02-03 18:10:21 PST
Note that this also affects sync/guid on 3.x: Bug 628788 works around this.
Comment 5 Marco Bonardo [::mak] 2011-05-17 18:10:58 PDT
Created attachment 533142 [details] [diff] [review]
patch v1.0

saving work, needs a test still.
Comment 6 Marco Bonardo [::mak] 2011-05-18 08:51:40 PDT
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.
Comment 7 Dietrich Ayala (:dietrich) 2011-05-18 09:54:23 PDT
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] 2011-05-18 10:14:00 PDT
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] 2011-05-18 10:50:42 PDT
> @@ +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] 2011-05-18 14:04:03 PDT
(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] 2011-05-18 14:23:02 PDT
Created attachment 533421 [details] [diff] [review]
patch v1.2
Comment 12 Marco Bonardo [::mak] 2011-05-18 14:27:11 PDT
http://hg.mozilla.org/projects/places/rev/77083dd59380
Comment 13 Marco Bonardo [::mak] 2011-05-23 20:44:59 PDT
http://hg.mozilla.org/mozilla-central/rev/77083dd59380
Comment 14 Marco Bonardo [::mak] 2011-05-23 21:11:12 PDT
Created attachment 534668 [details] [diff] [review]
add test to xpcshell.ini
Comment 15 Philipp von Weitershausen [:philikon] 2011-05-23 21:21:03 PDT
Comment on attachment 534668 [details] [diff] [review]
add test to xpcshell.ini

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

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