canmove, Confirmed users
725
edits
Places/AsyncAPIsForSync: Difference between revisions
Jump to navigation
Jump to search
→Notes and feedback on Proposal
Comrade693 (talk | contribs) (→Notes and feedback on Proposal: Address one piece of feedback that I missed before.) |
|||
| Line 197: | Line 197: | ||
== Notes and feedback on Proposal == | == Notes and feedback on Proposal == | ||
Due to the way Sync engines work, we have no need for the batch operation methods (insertBookmarks, updateItems) as we deal with each bookmark record individually. We're running this in batchmode anyway. --philikon | |||
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 206: | ||
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 | ||
Having the parent name in there feels awkward but it would make our code much easier while making the places code only slightly more complicated (I hope). I will have to think about alternatives to our current duping algorithm, but there's probably little alternative to having it match folder names. --philikon | |||
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 | ||
We ignore the tags folder right now, so it doesn't magically work. Sync uses the tagging service API explicitly. So either we should perhaps start syncing the tags folder (if that does indeed magically work) or nsIBookmarkInfo should contain the tags. --philikon | |||
getBookmarkInfo takes a nsIBookmarkInfo object, thus we probably don't want all of its properties being readonly. --mak | getBookmarkInfo takes a nsIBookmarkInfo object, thus we probably don't want all of its properties being readonly. --mak | ||
| Line 213: | Line 217: | ||
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 | ||
I don't think we have any need for querying bookmarks by URI. --philikon | |||
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 219: | Line 224: | ||
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 | ||
Yes, it would be great if missing properties on the nsIBookmarkInfo object would simply be ignored in the update. The same should go for annotations. This means that this API wouldn't allow us to remove annotations, but that's ok since we have no need for it. But we do avoid having to first get all the information before updating it. --philikon | |||
=History= | =History= | ||