Bookmark's `modified` timestamp is older than its `created` timestamp

ASSIGNED
Assigned to

Status

()

Firefox for Android
Data Providers
ASSIGNED
6 months ago
6 months ago

People

(Reporter: Grisha, Assigned: Grisha)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

6 months ago
Right after adding a bookmark, and looking at the db record, I see that modified is slightly older:

1492637568420 vs 1492637568419
Comment hidden (mozreview-request)
(Assignee)

Updated

6 months ago
Assignee: nobody → gkruglov
Status: NEW → ASSIGNED
(Assignee)

Updated

6 months ago
Attachment #8861217 - Flags: review?(rnewman) → review?(s.kaspari)

Comment 2

6 months ago
mozreview-review
Comment on attachment 8861217 [details]
Bug 1357896 - Let BrowserProvider handle bookmark timestamps on insertion

https://reviewboard.mozilla.org/r/133180/#review136344

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java
(Diff revision 1)
>          }
>  
> +        // Timestamps (modified and created) will be set by BrowserProvider.
>          values.put(Bookmarks.URL, uri);
>          values.put(Bookmarks.PARENT, folderId);
> -        values.put(Bookmarks.DATE_MODIFIED, now);

Isn't a simpler solution to add one line?

```
values.put(Bookmarks.DATE_CREATED, now);
```

::: mobile/android/base/java/org/mozilla/gecko/db/LocalBrowserDB.java:1183
(Diff revision 1)
>          debug("Bumping parent modified time for addition to: " + folderId);
>          final String where  = Bookmarks._ID + " = ?";
>          final String[] args = new String[] { String.valueOf(folderId) };
>  
>          ContentValues bumped = new ContentValues();
> -        bumped.put(Bookmarks.DATE_MODIFIED, now);
> +        bumped.put(Bookmarks.DATE_MODIFIED, System.currentTimeMillis());

The idea of using `now` was to have the modified parent and the child have the same modified time. That seems like a nice property to have: it ensures that if clock drift occurs then either both or neither of the two records will be retrieved from the database.

Comment 3

6 months ago
mozreview-review
Comment on attachment 8861217 [details]
Bug 1357896 - Let BrowserProvider handle bookmark timestamps on insertion

https://reviewboard.mozilla.org/r/133180/#review138388

Seems like Richard reviewd this already. Apart from that LGTM. :)
Attachment #8861217 - Flags: review?(s.kaspari) → review+
You need to log in before you can comment on or make changes to this bug.