1081106 - Implement fetchTree in Bookmarks.jsm

Status: NEW

(Toolkit :: Places, defect, P3)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

we must move the code from promiseBookmarksTree, but notice there are differences in input/output.

Comment 1

[Asaf Romano (gone)]

Initially I thought we can do without this in bug 1095425, staying with promiseBookmarksTree for the time being, but I discovered an issue that actually makes this a blocker.

Short story:
1) the async transactions tests use promiseBookmarksTree to ensure bookmarks were restored properly (~ Assert.deepEqual(originalTree, postTRedoTree) ).
2) promiseBookmarksTree exposes lastModified and dateAdded values in PRTime terms (microseconds) while Bookmarks.jsm uses Dates (which, when converted to primitive, are microseconds).
3) If PT just calls Bookmarks.insert, it's all good, because the PRTime value is just Date  * 1000.
4) However, if PT calls setItemAnnotationn, the lastModified value is set by cpp code that takes true microseconds values.
5) When PT reads that true-microseconds value using Bookmarks.jsm, it looses the micro-seconds precision, so when it restores the value on redo, lastModified was restored only in Bookmarks.jsm terms.
6) the deepEqual check fails in that case.

I think that fetchTree should expose dates using Date objects, rather than PRTime values. Backups code can use a replacer+reviver functions for serializing those to numbers (for better storage).

Given that, though, I think that for the time being we shouldn't make promiseBookmarksTree a wrapper around fetchTree. Let's first implement fetchTree the way we want it and the figure out the best way to utilize it in backups code.

Note: I think that exposing PRTime values in fetchTree isn't something we should do either way, but for the purpose of bug 1095425, an alternative solution is to make nsNaBookmarks loose the microseconds precision (it'd still store dates as PRTimes, but it'd just be ms_value * 1000).

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Yes, I think we should round up PRTime by losing microseconds in the backend.
One day we could even *gulp* move to milliseconds.

Comment 3

[Asaf Romano (gone)]

I filed and patched bug 1107308, so this no loner blocks PlacesTransactions-to-Bookmarks.jsm.