Store creation dates as milliseconds in the mirror

RESOLVED FIXED in Firefox 60

Status

()

enhancement
RESOLVED FIXED
Last year
Last year

People

(Reporter: lina, Assigned: lina)

Tracking

unspecified
Firefox 60
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(1 attachment)

We store modified times (`serverModified` and in the `meta` table) as milliseconds, creation dates in Sync records as milliseconds...but creation dates in the mirror as microseconds, because that's what Places uses. There are only a handful of times we convert between the two, and I should have used milliseconds from the beginning.

I don't think we need a migration for this, since the buffer is pref'd off, and we haven't started dogfooding yet...we can manually trash `weave/bookmarks.sqlite`.

Comment 2

Last year
mozreview-review
Comment on attachment 8957741 [details]
Bug 1444552 - Store bookmark creation dates as milliseconds in the mirror.

https://reviewboard.mozilla.org/r/226704/#review232486

Looks good to me modulo nits.

::: toolkit/components/places/SyncedBookmarksMirror.jsm:1521
(Diff revision 1)
>                                  parentTitle, dateAdded, type, title, isQuery,
>                                  url, tags, description, loadInSidebar,
>                                  smartBookmarkName, keyword, feedURL, siteURL,
>                                  position)
> -      SELECT b.id, b.guid, b.syncChangeCounter, p.guid, p.title, b.dateAdded,
> +      SELECT b.id, b.guid, b.syncChangeCounter, p.guid, p.title,
> +             IFNULL(b.dateAdded / 1000, ${

I'm not sure it makes a lot of sense to assign anything to the earliest bookmark timestamp. It's certainly very wrong, Why not Date.now()?

::: toolkit/components/places/SyncedBookmarksMirror.jsm:2062
(Diff revision 1)
>                   WHEN v.kind IN (${[
>                          SyncedBookmarksMirror.KIND.FOLDER,
>                          SyncedBookmarksMirror.KIND.LIVEMARK,
>                        ].join(",")}) THEN ${PlacesUtils.bookmarks.TYPE_FOLDER}
>                   ELSE ${PlacesUtils.bookmarks.TYPE_SEPARATOR} END),
> -           (CASE WHEN b.dateAdded < v.dateAdded THEN b.dateAdded
> +           (CASE WHEN b.dateAdded / 1000 < v.dateAdded THEN b.dateAdded

A comment explaining the `/ 1000` and `* 1000` logic here (specifically what units v.dateAdded/b.dateAdded have) would be appreciated next time I look at this.
Attachment #8957741 - Flags: review?(tchiovoloni) → review+
Comment hidden (mozreview-request)
Assignee

Comment 4

Last year
mozreview-review-reply
Comment on attachment 8957741 [details]
Bug 1444552 - Store bookmark creation dates as milliseconds in the mirror.

https://reviewboard.mozilla.org/r/226704/#review232486

> I'm not sure it makes a lot of sense to assign anything to the earliest bookmark timestamp. It's certainly very wrong, Why not Date.now()?

I reverted the `NOT NULL` constraint for `itemsToUpload.dateAdded`, as we talked about on IRC. If it's missing, we'll let Places maintenance take care of it (https://searchfox.org/mozilla-central/rev/44fa24847e4e73ce8932de4c8cf47559302e4ba2/toolkit/components/places/PlacesDBUtils.jsm#680), omit it from the record, and let the receiving clients ratchet the time backward.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 7

Last year
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a29e9196602f
Store bookmark creation dates as milliseconds in the mirror. r=tcsc

Comment 8

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/a29e9196602f
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
You need to log in before you can comment on or make changes to this bug.