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

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);

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