1444552 - Store creation dates as milliseconds in the mirror

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement)

Description

[Lina Butler [:lina]]

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 1

[Lina Butler [:lina]]

Attachment: Bug 1444552 - Store bookmark creation dates as milliseconds in the mirror.

Review commit: https://reviewboard.mozilla.org/r/226704/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/226704/

Comment 2

[Thom Chiovoloni [:tcsc] (ex-moco)]

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.

Comment 3

[Lina Butler [:lina]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226704/diff/1-2/

Comment 4

[Lina Butler [:lina]]

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 5

[Lina Butler [:lina]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226704/diff/2-3/

Comment 6

[Lina Butler [:lina]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226704/diff/3-4/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a29e9196602f
Store bookmark creation dates as milliseconds in the mirror. r=tcsc

Comment 8

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/a29e9196602f