Places/AsyncAPIsForSync: Difference between revisions

Jump to navigation Jump to search

Places/AsyncAPIsForSync (view source)

Revision as of 19:58, 12 November 2010

1,909 bytes removed ,  12 November 2010
Update proposal based on feedback from mak and remove feedback that was addressed.
(→‎Notes and feedback on Proposal 1: Responses to mak's feedback.)
(Update proposal based on feedback from mak and remove feedback that was addressed.)
Line 45: Line 45:
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?
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==
==Detailed Proposal==


The new methods can be added to a new interface.  Maybe nsIBookmarksService (and it only ever does async stuff)?
The new methods can be added to a new interface.  Maybe nsIBookmarksService (and it only ever does async stuff)?
Line 83: Line 83:
   readonly AString title;
   readonly AString title;
   /**
   /**
     * An array of nsIAnnotationInfo objects for the bookmark.
     * An array of nsIAnnotationInfo objects for the bookmark.  These are item annotations from nsIAnnotationService.
     */
     */
   readonly nsIVariant annotations;
   readonly nsIVariant annotations;
Line 130: Line 130:


  /**
  /**
   * Inserts a bookmark.  Required fields on nsIBookmarkInfo are:
   * Inserts a bookmark item.  Required fields on nsIBookmarkInfo are:
   *  - parentId or parentGuid
   *  - parentId or parentGuid
   *  - uri
   *  - uri
Line 141: Line 141:
   *        The object/function to notify when we have added the bookmark.
   *        The object/function to notify when we have added the bookmark.
   */
   */
  void insertBookmarkWithInfo(in nsIBookmarkInfo aInfo,
  void insertItem(in nsIBookmarkInfo aInfo,
                            in nsIBookmarkInfoCallback aCallback);
                [optional] 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.
Need to document about everything that would make us throw.  Also, how do we handle errors?  Want to keep the callback simple, ideally.
Line 149: Line 149:


  /**
  /**
   * Inserts many bookmarks.  Just like insertBookmarkWithInfo otherwise.
   * Inserts many bookmark items.  Just like insertItem otherwise.
   *
   *
   * @param aInfo
   * @param aInfo
Line 156: Line 156:
   *        The object/function to notify when we have added each bookmark.
   *        The object/function to notify when we have added each bookmark.
   */
   */
  void insertBookmarksWithInfo([array, size_is(aLength) in nsIBookmarkInfo aInfo,
  void insertItems([array, size_is(aLength) in nsIBookmarkInfo aInfo,
                              in unsigned long aLength,
                  in unsigned long aLength,
                              in nsIBookmarkInfoCallback aCallback);
                  [optional] 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.
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.
Line 166: Line 166:
  /**
  /**
   *  
   *  
   * Update the information about a bookmark.  Callers must specify the id, guid, xor the URI of the bookmark in question.
   * Update the information about a bookmark item.  Callers must specify the id, guid, xor the URI of the bookmark in question.
   *
   *
   * @param aIdentifier
   * @param aIdentifier
Line 175: Line 175:
   *        The object/function to notify when we have updated the information about the bookmark.
   *        The object/function to notify when we have updated the information about the bookmark.
   */
   */
  void updateBookmarkInfo(in nsIBookmarkInfo aIdentifier,
  void updateItem(in nsIBookmarkInfo aIdentifier,
                        in nsIBookmarkInfo aInfo,
                in nsIBookmarkInfo aInfo,
                        in nsIBookmarkInfoCallback aCallback);
                [optional] in nsIBookmarkInfoCallback aCallback);


=== updateBookmarksWithInfo ===
=== updateItem ===


  /**
  /**
   * Updates many bookmarks.  Just like updateBookmarkWithInfo otherwise.  aIdentifiers and aInfo must have a 1:1 mapping.
   * Updates many bookmark items.  Just like updateItem otherwise.  aIdentifiers and aInfo must have a 1:1 mapping.
   *
   *
   * @param aIdentifiers
   * @param aIdentifiers
Line 191: Line 191:
   *        The object/function to notify when we have added each bookmark.
   *        The object/function to notify when we have added each bookmark.
   */
   */
  void updateBookmarksWithInfo([array, size_is(aLength) in nsIBookmarkInfo aIdentifiers,
  void updateItems([array, size_is(aLength) in nsIBookmarkInfo aIdentifiers,
                              [array, size_is(aLength) in nsIBookmarkInfo aInfo,
                  [array, size_is(aLength) in nsIBookmarkInfo aInfo,
                              in unsigned long aLength,
                  in unsigned long aLength,
                              in nsIBookmarkInfoCallback aCallback);
                  [optional] in nsIBookmarkInfoCallback aCallback);


== Notes and feedback on Proposal 1 ==
== Notes and feedback on Proposal ==


The separate interface is the best bet, both for compatibility and clear sync/async separation.
The separate interface is the best bet, both for compatibility and clear sync/async separation.
Line 204: Line 204:
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
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
I think the whole setup is a bit volatile.  We should see what the Sync team says about it and if they care that they might have to do a second query to get the actual name. It's not hard to add it if need be, it just feels awkward. --sdwilsh
I think the whole setup is a bit volatile.  We should see what the Sync team says about it and if they care that they might have to do a second query to get the actual name. It's not hard to add it if need be, it just feels awkward. --sdwilsh
nsIAnnotationInfo could most likely drop "flags" support. It has never been really used nor we plan to start using it. --mak
It's also cheap to keep for now.  I think that as long as we support it on nsIAnnotaitonService, we should keep it here. --sdwilsh


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? --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? --mak
I thought about this, but I'm pretty sure that because we store tags as a bookmark in a different root, Sync will just work magically anyway.  We should confirm this with them though.  I'd rather not add it yet if we can avoid it because this is already a lot of work. --sdwilsh
I thought about this, but I'm pretty sure that because we store tags as a bookmark in a different root, Sync will just work magically anyway.  We should confirm this with them though.  I'd rather not add it yet if we can avoid it because this is already a lot of work. --sdwilsh
dynamic containers are a dead-feature-walking. nothing is using them.  --mak
Yeah, we only kinda support them via the type argument. --sdwilsh
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
Even if they have a bookmarks observer, they'd have to query to get things like guids (or also check for annotaitons, etc).  Using the callback is a lot easier.  However, it should be [optional] for all methods that take it. --sdwilsh


getBookmarkInfo takes a nsIBookmarkInfo object, thus we probably don't want all of its properties being readonly.
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
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
Last modified probably works fine, but we should check with the Sync team.  I suspect they might actually care about all cases, in which case, we need to change our approach a bit. --sdwilsh
Last modified probably works fine, but we should check with the Sync team.  I suspect they might actually care about all cases, in which case, we need to change our approach a bit. --sdwilsh
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
I'd be fine with insertItem. --sdwilsh
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
Consumer convenience, really.  The backend can easily just call the array method (this is how storage does the different executeAsyncs). --sdwilsh


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?
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?
Line 233: Line 217:
updateItem({id:2}, {guid:"GUID_HERE"});
updateItem({id:2}, {guid:"GUID_HERE"});
This would set the guid on item with id of 2 to "GUID_HERE".  Easy to implement because things will just throw NS_ERROR_NOT_IMPLEMENTED.  I hope this doesn't get logged to the error console though... --sdwilsh
This would set the guid on item with id of 2 to "GUID_HERE".  Easy to implement because things will just throw NS_ERROR_NOT_IMPLEMENTED.  I hope this doesn't get logged to the error console though... --sdwilsh
regarding the batch thing, same as above, do we need both? --mak
trivial to do, ends up being easier for the consumer (thinking really of the browser/fennec UI). --sdwilsh
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
bookmark annotation. --sdwilsh


=History=
=History=
Comrade693
590

edits

Retrieved from "https://wiki.allizom.org/Special:MobileDiff/267473"

Navigation menu