Places/AsyncAPIsForSync

Overview

Tracking bug for Sync: bug 606353

Current situation

Right now Sync calls various synchronousm Places API methods to read and write records, with the exception of history reads where it rolls its own asynchronous SQL queries.

Problem

Synchronous sqlite I/O is hurting us a lot on mobile.

Proposed solution

Provide powerful asynchronous methods to replace Sync's many synchronous calls. So instead of adding a bookmark and then adding a bunch of annotations to it, it would be great if Places had an API that would let us do it one go, do all the writes async and then call us back.

Bookmarks

bug 519514

Description of Sync's bookmark record (includes folders, separators, livemarks, etc.): https://wiki.mozilla.org/Labs/Weave/Developer/BrowserObjects#bookmark

Read

For syncing out as well as conflict resolution, right now we call

• getFolderIdForItem()
• getBookmarkURI()
• getKeywordForBookmark()
• and get the various "bookmarkProperties/" annotations (description, loadInSidebar, staticTitle, etc.).

I imagine that this information could be retrieved with one query which could be wrapped into getBookmarkInfoAsync() or similar API call.

Write

We currently use the following nsINavBookmarksService methods to create items:

• insertBookmark()
• createFolder()
• insertSeparator()
• as well as nsILivemarkService::createLivemark()

When we make these or a subset of these async, how will be notified of the ID (and later, when we add GUIDs, their GUID)? Through an nsINavBookmarkObserver? Or will there be a callback we can pass into? We need the ID / GUID so that we can set various "bookmarkProperties/" annotations (description, loadInSidebar, staticTitle, etc.) as well as keywords via setKeywordForBookmark(). Ideally, of course, we could follow aforementioned theme and have the insert*Async() methods allow us to pass those in as well. Then we'd only need a callback for special cases such as microsummaries and tags.

To update already existing items, we call setItemTitle(), changeBookmarkURI(), setKeywordForBookmark() and update various annations. Perhaps there could be an updateBookmarkAsync() akin to insertBookmarkAsync() to save us these various separate method calls?

Detailed Proposal 1

The new methods can be added to a new interface. Maybe nsIBookmarksService (and it only ever does async stuff)?

The only thing not covered here so far is livemarks. Not sure if it's worth an API for them or not (can we get data on how many people store now?).

nsIAnnotationInfo

/**
 * Interface that describes an annotation.
 */
interface nsIAnnotationInfo : nsISupports
{
  readonly AUTF8String name;
  readonly nsIVariant value;
  readonly long flags;
  readonly unsigned short expiration;
}

Ideally, we could use this for a future async annotation service. For now, it's just purposed for nsIBookmarkInfo.

nsIBookmarkInfo

/**
 * Interface that describes a bookmark.
 */
interface nsIBookmarkInfo : nsISupports
{
  readonly ACString guid;
  readonly long long id;
  readonly unsigned short type;
  readonly long long parentId;
  readonly ACString parentGuid;
  readonly long index;
  readonly nsIURI uri;
  readonly AString keyword;
  readonly AString title;
  /**
   * An array of nsIAnnotationInfo objects for the bookmark.
   */
  readonly nsIVariant annotations;
}

The idea here is that this interface will be returned and given to places APIs when you ask for information about a bookmark, and when you want to add a bookmark. Because we have the type present, this will handle bookmarks, folders, separators, and dynamic containers (which, if I recall correctly, handles microsummaries and live bookmarks).

I included guids here, which makes this gated (possibly) on bug 607117.

Not clear to me if we need tags on here or not. Need input from the Sync team.

nsIBookmarkInfoCallback

interface nsIBookmarkInfoCallback : nsISupports
{
  /**
   * Called when one of the bookmark methods is done with its work.
   *
   * @param aBookmarkInfo
   *        The information about the bookmark, or undefined if nothing was found.
   */
  void onComplete(in nsIBookmarkInfo aBookmarkInfo);
}

This should be marked with [function] so JS consumers can just pass a function in if they want.

getBookmarkInfo

/**
 * Gets all information known about a bookmark.  Callers must specify the id, guid, or the URI of the bookmark in question.
 *
 * @param aInfo
 *        The bookmark info object that contains the id xor the guid xor the URI of the bookmark.
 * @param aCallback
 *        The object/function to notify when we have the information about the bookmark.
 */
void getBookmarkInfo(in nsIBookmarkInfo aInfo,
                     in nsIBookmarkInfoCallback aCallback);

Consumers would do something like this: bs.getBookmarkInfo({guid:"..."}, function(aInfo) { ... });

C++ consumers won't be happy, but I'm not sure I care.

insertBookmark

/**
 * Inserts a bookmark.  Required fields on nsIBookmarkInfo are:
 *  - parentId or parentGuid
 *  - uri
 *  - title
 * Everything else is optional.
 *
 * @param aInfo
 *        The bookmark info object that contains the data needed.
 * @param aCallback
 *        The object/function to notify when we have added the bookmark.
 */
void insertBookmarkWithInfo(in nsIBookmarkInfo aInfo,
                            in nsIBookmarkInfoCallback aCallback);

Need to document about everything that would make us throw. Also, how do we handle errors? Want to keep the callback simple, ideally.

insertBookmarksWithInfo

/**
 * Inserts many bookmarks.  Just like insertBookmarkWithInfo otherwise.
 *
 * @param aInfo
 *        The bookmark info objects that contain the data needed.
 * @param aCallback
 *        The object/function to notify when we have added each bookmark.
 */
void insertBookmarksWithInfo([array, size_is(aLength) in nsIBookmarkInfo aInfo,
                             in unsigned long aLength,
                             in nsIBookmarkInfoCallback aCallback);

Just like insertBookmarkWithInfo, but takes a big array of bookmark info and does it all at once. This is basically the batch mode version. I suspect mak and I are going to debate how to best do batch mode, so this may change a lot still.

updateBookmarkWithInfo

/**
 * 
 * Update the information about a bookmark.  Callers must specify the id, guid, xor the URI of the bookmark in question.
 *
 * @param aIdentifier
 *        The bookmark info object that contains the id xor the guid xor the URI of the bookmark.
 * @param aInfo
 *        The information to update about the bookmark.
 * @param aCallback
 *        The object/function to notify when we have updated the information about the bookmark.
 */
void updateBookmarkInfo(in nsIBookmarkInfo aIdentifier,
                        in nsIBookmarkInfo aInfo,
                        in nsIBookmarkInfoCallback aCallback);

updateBookmarksWithInfo

/**
 * Updates many bookmarks.  Just like updateBookmarkWithInfo otherwise.  aIdentifiers and aInfo must have a 1:1 mapping.
 *
 * @param aIdentifiers
 *        Array of bookmark info objects that contains the id xor the guid xor the URI of the bookmarks.
 * @param aInfo
 *        Array of information to update about the bookmark.
 * @param aCallback
 *        The object/function to notify when we have added each bookmark.
 */
void updateBookmarksWithInfo([array, size_is(aLength) in nsIBookmarkInfo aIdentifiers,
                             [array, size_is(aLength) in nsIBookmarkInfo aInfo,
                             in unsigned long aLength,
                             in nsIBookmarkInfoCallback aCallback);

Notes and feedback on Proposal 1

The separate interface is the best bet, both for compatibility and clear sync/async separation. Name could either be nsIBookmarksService (drop ns?) or IAsyncBookmarks. --mak

Sync seem to use parentName for reconciliation, but does not look like something that should be part of the API, both the parent and the name can change easily and having 2 volatile informations seem fragile. On the other side caching parent names on the fly in a local hash is probably as efficient. --mak

nsIAnnotationInfo could most likely drop "flags" support. It has never been really used nor we plan to start using it. --mak

nsIBookmarkInfo should most likely include tags or we need a way to set tags async. It's indeed impossible to create a bookmark async and tag it synchronously. it could be possible to add tags in the bookmarks added notification, but is it worth it? dynamic containers are a dead-feature-walking. nothing is using them. --mak

is nsIBookmarkInfoCallback really useful? Probably yes if the implementer does not want to have a bookmarks observer, that on our side is also expensive. Most likely if the implementer has an observer the callback has no use. --mak

getBookmarkInfo takes a nsIBookmarkInfo object, thus we probably don't want all of its properties being readonly. Notice that while passing an id or guid will return a single bookmark, passing an uri is not guaranteed to do so. the same uri can have multiple bookmarks. what to do here? either don't allow by uri, or return last modified bookmark (that is what we do today) --mak

insertBookmarkWithInfo looks a strange name. First it is clear I have to pass some info to create a bookmark, second this can create bookmarks, folders, separators... maybe we should go for a generic createItem() method --mak

insertBookmarksWithInfo same as above. Fine for batching since it's handled internally. Is the single instance useless if one can just use the multiple one with a 1-sized array? --mak

updatebookmarkWithInfo same as above for name. Actually how to handle missing information in the info object? Does the implementer have to collect all info, change and then submit? Or things that should not be changed must be null? How to set a null value then? Having to collect info to be able to change them is going to be more expensive than changing them one by one. --mak

regarding the batch thing, same as above, do we need both? --mak

Regarding annotations, what should be set? a page or a bookmark annotation? most likely so far we just need bookmark annotations. Page annotations are mostly used for charsets. --mak

History

bug 606966

Same as with bookmarks, really, but much less complicated.

Description of Sync's history records: https://wiki.mozilla.org/Labs/Weave/Developer/BrowserObjects#history

Read

We already roll our own async SQL queries to fetch the metadata for a history entry and its visits (two queries). We could push this down to Places, but given that we already do the right thing here, it probably has low priority.

Write

Right now call nsINavHistoryService::addVisit() for each visit in a history record that doesn't exist locally yet and then nsINavHistoryService::setPageTitle() to set the page title. As discussed in https://bugzilla.mozilla.org/show_bug.cgi?id=606966#c14, an API that would allow us to pass in the page title and a list of visits would be great. It would asynchronously set the page title and add all the visits (unless they exist already)

Detailed Proposal

Introduce

• addVisitsAsync(uri, title, [array of visits], callback)

TODO flesh out