canmove, Confirmed users
725
edits
Places/AsyncAPIsForSync: Difference between revisions
→Notes and feedback on Proposal
| Line 198: | Line 198: | ||
== Notes and feedback on Proposal == | == Notes and feedback on Proposal == | ||
=== No need for "plural" methods === | |||
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 | 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 | ||
=== Interfaces === | |||
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. | ||
Name could either be nsIBookmarksService (drop ns?) or IAsyncBookmarks. --mak | Name could either be nsIBookmarksService (drop ns?) or IAsyncBookmarks. --mak | ||
Don't have a strong preference. I say just defer to sr on this. --sdwilsh | Don't have a strong preference. I say just defer to sr on this. --sdwilsh | ||
=== Parent Name=== | |||
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 | 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 | ||
=== Tags === | |||
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 | |||
We explicitly 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 | |||
=== nsIBookmarkInfo readonly === | |||
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 | ||
It shouldn't matter. We don't have to return the same nsIBookmarkInfo object that we are passed in (in fact, for threadsafety reasons, we shouldn't!) --sdwilsh | It shouldn't matter. We don't have to return the same nsIBookmarkInfo object that we are passed in (in fact, for threadsafety reasons, we shouldn't!) --sdwilsh | ||
=== Query by URI === | |||
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 | I don't think we have any need for querying bookmarks by URI. --philikon | ||
=== Update: missing attributes ignored === | |||
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? | ||
Having to collect info to be able to change them is going to be more expensive than changing them one by one. --mak | Having to collect info to be able to change them is going to be more expensive than changing them one by one. --mak | ||
How about updateItem then? The implementer should only have to pass in the id or the guid. I envisioned them also being able to specify only the parts that changed: | How about updateItem then? The implementer should only have to pass in the id or the guid. I envisioned them also being able to specify only the parts that changed: | ||
updateItem({id:2}, {guid:"GUID_HERE"}); | <code>updateItem({id:2}, {guid:"GUID_HERE"});</code> | ||
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 | 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 | ||