1068007 - Design an async bookmarking API

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

by taking inspiration from mozIAsyncHistory, we need an API that can replace all usage of nsINavBookmarks with few methods as possible.

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I just found this old document where we (me and sdwilsh) first designed a possible async API. This is a little bit outdated but has some very interesting comments and points about Sync.

https://wiki.mozilla.org/index.php?title=Places/AsyncAPIsForSync

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

So, what do we do with sorting?
Currently it's done through setItemIndex that is very sucky cause that API can easily destroy the whole data coherence.

There are 2 uses for that API currently.

1. sorting transaction. I'm honestly starting to think we could make sorting not-undoable, even if that sucks a little bit... I don't want to provide an API to change the index blindly like setItemIndex. I'd prefer to provide a sort() API, but that's not undoable.

2. CloudSync... well I'm not even sure what is cloudsync, let alone knowing why it's using our most dangerous API instead of moveItem...

Comment 3

[Asaf Romano (gone)]

Ideally, we would have some sort of a SetOrder API, which takes an folder guid and an array of item guids, specifying the new children order. It would throw for anything but an array that contains all the children-item-guids in that folder, and only them.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1

Let's start from a first pass on the API, it might change while we move on with the implementation, but I think this is a good start and better to get feedback sooner than later.

Comment 5

[Mark Hammond [:markh] [:mhammond]]

I'm not sure what info I can offer.  The sync bookmarks engine *and* the bookmarks API itself are both somewhat black holes to me.

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

sorry, I thought you were involved with them, but looking at the log sounds like you were not. I should have checked earlier.

Then the ball goes to Richard! Why is CloudSyncBookmarks.jsm using setItemIndex? Can it stop doing that? (see bug 539517, that unfortunately was never completed)

Comment 7

[Asaf Romano (gone)]

Comment on attachment 8491023 [details] [diff] [review]
patch v1


>+ * This module provides an async API for managing bookmarks.
>+ *
>+ * Bookmarks are organized in a tree structure, and can be bookmarked uris,
>+ * folders or separators. Duplicate URIs are allowed, so a single URI can be
>+ * contained in one or more folders.

I think "Multiple bookmarks for the same URI are allowed, [thus/meaning that] a single URI may be bookmarked under multiple folders, or even multiple times under a single folder." reads better. But really, "Multiple bookmarks for the same URI are allowed." is clear enough, IMO.

>+ *
>+ * Each bookmarked item is represented by a javascript object having the
>+ * following properties:
>+ *
>+ *  - guid (string)
>+ *      The globally unique identifier of the item.
>+ *  - parentGUID (string)
>+ *      The globally unique identifier of the parent folder containing the item.

"(an empty string for the places root)"?

>+ *  - parentTitle (string)
>+ *      The title of the parent folder containing the item.

What? Why?

>+ *  - index (number)
>+ *      The (0-based) position of the item in the parent folder.

I'd remove the parentheses.

>+ *  - dateAdded (number)
>+ *      The time at which the item was added, in microseconds from the epoch.
>+ *  - lastModified (number)
>+ *      The time at which the item was last modified, in microseconds from the
>+ *      epoch.

I think we should consider using something like DOMHighResTimeStamp (which is just a float). One advantage of that is that we can protect callers from accidentally passing milliseconds-longs (you may then also use window.performance.now, which if I recall correctly, works around those Windows timers bugs).

If you decide to stick to PRTime though, do mention it explicitly because it's easy to overlook.

>+ *  - type (number)
>+ *      The item's type, either bookmark, folder or separator.
>+ *      @see nsINavBookmarks.TYPE_* constants.
>+ *
>+ *  The following properties are only valid for bookmarks or folders.
>+ *
>+ *  - title (string)
>+ *      The item's title, otherwise this property isn't set.
>+ *

"if any"/"if appropriate"/"for bookmarked URLs and folders".

The semantics of null (void) titles should be documented here as well

>+ *  The following properties are only valid for bookmarks:
>+ *
>+ *  - uri (nsIURI)
>+ *      The item's uri, otherwise this property isn't set.

When is uri unset for bookmarks?

>+ *  - keyword (string)
>+ *      The associated keyword, otherwise this property isn't set.

"if any"?

>+ *  - frecency (number)
>+ *      The frecency of the item, -1 if unknown.

Any reason to put this in the bookmarks interface? It's history stuff.

>+ * Each successful operation notifies through the nsINavBookmarksObserver
>+ * interface.

I'd argue we should only do so as long as the old API exists. I think it makes sense to introduce a non-xpcom alternative (in which you can include just the callbacks you want, and those callbacks don't take a dozen of arguments).


>+let Bookmarks = Object.freeze({
>+  /**
>+   * Creates or updates a bookmarked item.
>+   *

We need to mention somewhere here that unless the consumer has a compelling reason. This interface shouldn't be used directly (transactions should be used).

>+   * If the given guid is found, the corresponding item will be updated,
>+   * otherwise a new item with that guid will be created. If no guid is provided
>+   * a new bookmark is created and a new guid is generated for it.
>+   *
>+   * In case of creation, a minimum set of properties must be provided:
>+   *  - type

I think the type constants should be redefined in this module.

>+   *  - parentGuid

By the way, in transactions I use GUID and parentGUID. I can certainly change that, or you could. In any case, the interfaces should be in sync.

>+   *  - index, though if not provided will default to appending

not part of the minimal set then.

>+   *  - title, only for bookmarks and folders

title-less bookmarks are allowed (both in the current API and in PlacesTransactions).

>+   *  - uri, only for bookmarks


>+   * In case of update, it's not necessary to pass all of the properties,
>+   * since undefined properties won't be taken into account for the update.
>+   * Moreover, the item's type and the guid will be ignored, since they are
>+   * immutable after creation.

Consider throwing (or rejecting, I guess) if they're set.

>+  /**
>+   * Removes a bookmarked item.
>+   *
>+   * Input can either be a guid or a javascript object with one of the following
>+   * properties set:
>+   *  - guid: if set, only the corresponding item is removed.
>+   *  - parentGuid: if set and a folder, any children of that folder will be
>+   *                removed, but not the folder itself.
>+   *  - uri: if set, any bookmark for that uri will be removed.
>+   * If multiple of these properties are set, the method will reject.
>+   *
>+   * Any other property is ignored, known properties may be overwritten.
>+   *
>+   * @param guidOrInfo
>+   *        The globally unique identifier of the item to remove, or a
>+   *        javascript object representing it, as defined above.
>+   *

What's the use cases for parentGuid and uri?

>+  /**
>+   * Fetches information about a bookmarked items.
>+   *

s/a//

>+
>+  /**
>+   * Sorts contents of a folder based on a provided array of guids.
>+   *
..
>+  sort: Task.async(function* (parentGuid, sortedChildrenGuids) {

It's really setOrder. because it'll also be used when you *undo* sorting.

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

So, regarding the addition of fetchTree, there's a small issue due to the fact bookmarks API used nsiNavBookmarks.TYPE_* constants, while the tree uses PlacesUtils.TYPE_X_* constants... Both use a .type property.

I think we should change it to use basic constants and fix the consumers to to the conversion to PlacesUtils flavors.
Or use .flavor?
What do you think?

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Richard, could you also please help us figuring if Sync still needs the parent title for reconciliation and if eventually there are alternatives for that?

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v2

this fixes the comments, apart timestamps (we can discuss that at a later time) and new notifications (we don't have time or resources to attack that now).

I added fetchTree to replace promiseBookmarksTree, for now I think to COPY its code and do the changes required by the new API conventions (for example type not being a PlacesUtils.TYPE_X_, parentGuid being always defined and so on), then we can adapt consumers and remove the "old" implementation.

I removed frecency and parentTitle until we know if those are really needed.

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

So, regarding parentTitle, as I was remembering it's used for reconciliation, so basically when guids are not guids across 2 systems but 2 bookmarks look exactly identical, Sync decides what to keep based on the parent title.
It should happen more rarely nowadays that we restore guids from json/html, so I will keep that for bug 1068054.

Regarding setItemIndex, it's a little OT here, so I will now file a bug for it and cc ack there.

Comment 12

[Asaf Romano (gone)]

Comment on attachment 8491409 [details] [diff] [review]
patch v2

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

::: toolkit/components/places/Bookmarks.jsm
@@ +10,5 @@
> + * Bookmarks are organized in a tree structure, and can be bookmarked URIs,
> + * folders or separators.  Multiple bookmarks for the same URI are allowed.
> + *
> + * Note that if you are handling bookmarks operations in the UI, you should
> + * not directly use this API, but rather use PlacesTransactions.jsm, so that

"you should not use this API directly." reads better, I think.

@@ +13,5 @@
> + * Note that if you are handling bookmarks operations in the UI, you should
> + * not directly use this API, but rather use PlacesTransactions.jsm, so that
> + * any operation is undo/redo-able.
> + *
> + * Each bookmarked item is represented by a javascript object having the

s/a javascript object/an object

@@ +19,5 @@
> + *
> + *  - guid (string)
> + *      The globally unique identifier of the item.
> + *  - parentGuid (string)
> + *      The globally unique identifier of the parent folder containing the item.

s/parent/

@@ +34,5 @@
> + *  The following properties are only valid for bookmarks or folders.
> + *
> + *  - title (string)
> + *      The item's title, if any.  Empty titles and null titles are considered
> + *      the same and stored as NULLs.

Don't mention how they are stored, just that in both cases the property is unset on retrieval.

@@ +45,5 @@
> + *      The associated keyword, if any.
> + *
> + * Each successful operation notifies through the nsINavBookmarksObserver
> + * interface.  To listen to such notifications you must register using
> + * nsINavBookmarksService addObserver and removeObserver methods.

So at the very least I'd add a shortcut to these methods here.

@@ +46,5 @@
> + *
> + * Each successful operation notifies through the nsINavBookmarksObserver
> + * interface.  To listen to such notifications you must register using
> + * nsINavBookmarksService addObserver and removeObserver methods.
> + * Note that bookmark addition or sorting changes won't notify onItemMoved for

s/sorting/order/

@@ +47,5 @@
> + * Each successful operation notifies through the nsINavBookmarksObserver
> + * interface.  To listen to such notifications you must register using
> + * nsINavBookmarksService addObserver and removeObserver methods.
> + * Note that bookmark addition or sorting changes won't notify onItemMoved for
> + * items that will have their indexes changed.

so google says both indexes, and indices are legit :)

@@ +48,5 @@
> + * interface.  To listen to such notifications you must register using
> + * nsINavBookmarksService addObserver and removeObserver methods.
> + * Note that bookmark addition or sorting changes won't notify onItemMoved for
> + * items that will have their indexes changed.
> + * Similarly, lastModified changes not done on purpose (like changing another

s/on purpose/explicitly

@@ +83,5 @@
> +   * These should stay consistent with nsINavBookmarksService.idl
> +   */
> +  get TYPE_BOOKMARK() 1,
> +  get TYPE_FOLDER() 2,
> +  get TYPE_SEPARATOR() 3,

getters? Object.freeze (as you already do) for plain properties has the same effect.

@@ +89,5 @@
> +  /**
> +   * Creates or updates a bookmarked item.
> +   *
> +   * If the given guid is found, the corresponding item will be updated,
> +   * otherwise a new item with that guid will be created. If no guid is provided

Please find a way to describe the ability of presetting guids later here. It reads as if that's what one should do by default.

@@ +98,5 @@
> +   *  - parentGuid
> +   *  - URI, only for bookmarks
> +   * Unless you've a very sound reason, such as an undo manager implementation,
> +   * or synchronization, you should never pass a guid on creation.
> +   * If index is not specified, it will default to appending.

"index", the index or an index.

@@ +100,5 @@
> +   * Unless you've a very sound reason, such as an undo manager implementation,
> +   * or synchronization, you should never pass a guid on creation.
> +   * If index is not specified, it will default to appending.
> +   *
> +   * In case of update, it's not necessary to pass all of the properties,

In the update case.

But actually, even on creation it's valid. Maybe this should read something like "In the update case, you should only set the properties which should be changed".

@@ +105,5 @@
> +   * since undefined properties won't be taken into account for the update.
> +   * Moreover, the item's type and the guid will be ignored, since they are
> +   * immutable after creation.  Note that if the passed in values are not
> +   * coherent with the known values, this will reject.
> +   * Passing null or an empty string as keyword, will clear any keyword

remove the coma.

And I think "clears" is better than "will clear", but I'd not trust myself about that.

@@ +111,5 @@
> +   *
> +   * Any property that doesn't apply to the specific item type is also ignored.
> +   *
> +   * @param info
> +   *        javascript object representing a bookmarked item, as defined above.

an object

@@ +137,5 @@
> +
> +  /**
> +   * Removes a bookmarked item.
> +   *
> +   * Input can either be a guid or a javascript object with one of the following

or an info object having...

@@ +163,5 @@
> +    throw new Error("Not yet implemented");
> +  }),
> +
> +  /**
> +   * Fetches information about bookmarked items.

a bookmarked item.

@@ +170,5 @@
> +   * properties set:
> +   *  - guid
> +   *      retrieves one item with the specified guid
> +   *  - parentGuid and index
> +   *      retrieves one item by its position

the item

What happens if the index is invalid? What about DEFAULT_INDEX?

@@ +174,5 @@
> +   *      retrieves one item by its position
> +   *  - URI
> +   *      retrieves one ore more items for the given URI.
> +   *  - keyword
> +   *      retrieves one or more items for the given keyword.

all items having

@@ +175,5 @@
> +   *  - URI
> +   *      retrieves one ore more items for the given URI.
> +   *  - keyword
> +   *      retrieves one or more items for the given keyword.
> +   * If multiple of these properties are set, the method will reject.

The wording here is somewhat strange (maybe it's just me though).

@@ +177,5 @@
> +   *  - keyword
> +   *      retrieves one or more items for the given keyword.
> +   * If multiple of these properties are set, the method will reject.
> +   *
> +   * Any other property is ignored, known properties may be overwritten.

, -> . or ;

@@ +184,5 @@
> +   *        The globally unique identifier of the item to fetch, or a
> +   *        javascript object representing it, as defined above.
> +   *
> +   * @return {Promise} resolved when the fetch is complete.
> +   * @resolves to a javascript object representing the found item, as described

an object (the are more instances later).

@@ +208,5 @@
> +    throw new Error("Not yet implemented");
> +  }),
> +
> +  /**
> +   * Sorts contents of a folder based on a provided array of GUIDs.

It doesn't sort

@@ +212,5 @@
> +   * Sorts contents of a folder based on a provided array of GUIDs.
> +   *
> +   * @param parentGuid
> +   *        The globally unique identifier of the folder whose contents should
> +   *        be sorted.

ditto.

@@ +221,5 @@
> +   * @rejects JavaScript exception.
> +   */
> +  // XXX WIP XXX Will replace functionality from these methods:
> +  // void setItemIndex(in long long aItemId, in long aNewIndex);
> +  setOrder: Task.async(function* (parentGuid, sortedChildrenGuids) {

How about "reorder"?

@@ +283,5 @@
> +   * @rejects JavaScript exception.
> +   */
> +  // XXX WIP XXX: will replace functionality for these methods:
> +  // PlacesUtils.promiseBookmarksTree()
> +  fetchTree: Task.async(function* (guid = "", options = {}) {

Let's have this right after fetch.

Comment 13

[Richard Newman [:rnewman]]

Note that Sync only retrieves the parent title the hard way:

        record.parentName = PlacesUtils.bookmarks.getItemTitle(parent);

so I don't think you need to treat parentTitle specially.

Comment 14

[Asaf Romano (gone)]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #8)
> So, regarding the addition of fetchTree, there's a small issue due to the
> fact bookmarks API used nsiNavBookmarks.TYPE_* constants, while the tree
> uses PlacesUtils.TYPE_X_* constants... Both use a .type property.
> 
> I think we should change it to use basic constants and fix the consumers to
> to the conversion to PlacesUtils flavors.
> Or use .flavor?
> What do you think?

Yes, but you'll need to fix backups code to handle both options, at least for few releases. As agreed over IRC, for now we'll have a "fixed" copy of promiseBookmarksTree.

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v3

I think this is ready for SR.

We can land this patch as-is, it will be the base for other preparation work like forwarding calls from PlacesUtils.bookmarks (that I'm going to file now).

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8492184 [details] [diff] [review]
patch v3

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

::: toolkit/components/places/Bookmarks.jsm
@@ +24,5 @@
> + *      This will be an empty string for the Places root folder.
> + *  - index (number)
> + *      The 0-based position of the item in the parent folder.
> + *  - dateAdded (number, microseconds from the epoch)
> + *      The time at which the item was added.  This is a PRTime.

Adding a (microseconds) to clarify the unit of a PRTime would probably be useful.

@@ +48,5 @@
> + * interface.  To listen to such notifications you must register using
> + * nsINavBookmarksService addObserver and removeObserver methods.
> + * Note that bookmark addition or order changes won't notify onItemMoved for
> + * items that have their indexes changed.
> + * Similarly, lastModified changes not done on explicitly (like changing another

s/on//?

@@ +109,5 @@
> +   * coherent with the known values, this rejects.
> +   * Passing null or an empty string as keyword clears any keyword
> +   * associated with this bookmark.
> +   *
> +   * Any property that doesn't apply to the specific item type is also ignored.

Would it be feasible to reject in this case rather than just ignoring? I don't know the use cases all that well.

@@ +143,5 @@
> +   * properties set:
> +   *  - guid: if set, only the corresponding item is removed.
> +   *  - parentGuid: if it's set and is a folder, any children of that folder is
> +   *                removed, but not the folder itself.
> +   *  - URI: if set, any bookmark for that URI is removed.

Combining all of these cases into a single function is a bit odd. The parentGUID case in particular - can we keep a separate removeFolderChildren?

@@ +276,5 @@
> +   * @param parentGuid
> +   *        The globally unique identifier of the folder whose contents should
> +   *        be reordered.
> +   * @param sortedChildrenGuids
> +   *        Sorted array of the children's GUIDs.

Worth specifying what happens if sortedChildrenGuids contains non-existent children GUIDs, or if the list is incomplete.

Comment 17

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #16)
> @@ +109,5 @@
> > +   * coherent with the known values, this rejects.
> > +   * Passing null or an empty string as keyword clears any keyword
> > +   * associated with this bookmark.
> > +   *
> > +   * Any property that doesn't apply to the specific item type is also ignored.
> 
> Would it be feasible to reject in this case rather than just ignoring? I
> don't know the use cases all that well.

yes, I think my idea was actually to ignore properties that are not "ours" but I wrote differently.

> @@ +143,5 @@
> > +   * properties set:
> > +   *  - guid: if set, only the corresponding item is removed.
> > +   *  - parentGuid: if it's set and is a folder, any children of that folder is
> > +   *                removed, but not the folder itself.
> > +   *  - URI: if set, any bookmark for that URI is removed.
> 
> Combining all of these cases into a single function is a bit odd. The
> parentGUID case in particular - can we keep a separate removeFolderChildren?

Yes, though I coalesced these for some reasons:
1. history.removePlaces is going to work the same, and likely we should provide a shorthand history.remove... then the 2 would have some coherence.
2. in the bookmarks object we already have parentGuid, so it looked natural.
3. we'd still have guid or uri, so it would still not be a 1-shot api

> 
> @@ +276,5 @@
> > +   * @param parentGuid
> > +   *        The globally unique identifier of the folder whose contents should
> > +   *        be reordered.
> > +   * @param sortedChildrenGuids
> > +   *        Sorted array of the children's GUIDs.
> 
> Worth specifying what happens if sortedChildrenGuids contains non-existent
> children GUIDs, or if the list is incomplete.
right, I guess we could ignore non-existing guids (it might be hard for the consumer to keep the list up-to-date... incomplete list I don't know, I guess unknown items could be appended?

Comment 18

[:Gavin Sharp [email: [e-mail]]]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #17)
> Yes, though I coalesced these for some reasons:
> 1. history.removePlaces is going to work the same, and likely we should
> provide a shorthand history.remove... then the 2 would have some coherence.
> 2. in the bookmarks object we already have parentGuid, so it looked natural.
> 3. we'd still have guid or uri, so it would still not be a 1-shot api

Sounds fine, wasn't a strong objection. Rejecting if multiple properties are specified solves most of the potential confusion problem.

Comment 19

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

todo from irc: change sorted -> ordered, and resync promiseBookmarksTree javadoc for includeItemIds.

Comment 20

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

with fixed comments:
https://hg.mozilla.org/integration/fx-team/rev/f809f2ecc047

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f809f2ecc047