1405317 - Restoring bookmarks from json files ignores lastModified for bookmarks with an annotation

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P1)

Description

[Mark Banner (:standard8)]

Prior to bug 1095426, bookmarks which were imported would have always respected the last modified time and imported that into the database.

After that bug, any bookmark that has an annotation has the last modified date set to the current time.

Comment 1

[Mark Banner (:standard8)]

Attachment: Bug 1405317 - When inserting trees of bookmarks, avoid updating the last modified date when marking annotations.

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

Comment 2

[Code Review Bot [:reviewbot]]

Comment on attachment 8915082 [details]
Bug 1405317 - When inserting trees of bookmarks, avoid updating the last modified date when marking annotations.

https://reviewboard.mozilla.org/r/186348/#review191354


C/C++ static analysis didn't find any defects in this patch. Hooray!

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 8915082 [details]
Bug 1405317 - When inserting trees of bookmarks, avoid updating the last modified date when marking annotations.

https://reviewboard.mozilla.org/r/186348/#review191380

I'm not a bot :)

Could please also remove this case http://searchfox.org/mozilla-central/rev/e62604a97286f49993eb29c493672c17c7398da9/toolkit/components/places/PlacesTransactions.jsm#1045 (remove lines 1045-1046)
I also think NewBookmark, NewFolder, NewLivemark should pass true.
The comments in NewBookmark undo and redo need to be updated... Note that I don't recall if we really bump an item last modified when we tag it, it sounds unlikely, so I'm not sure if the comments are wrong regarding tags. We surely increase lastModified of bookmarks representing tags, but I don't think we do for the original bookmark. Worth a check.

::: toolkit/components/places/Bookmarks.jsm:1510
(Diff revision 1)
>        let livemark = await PlacesUtils.livemarks.addLivemark(item);
>  
>        let id = livemark.id;
>        if (item.annos && item.annos.length) {
> +        // Note: we intentionally for annotations to skip updating the last modified
> +        // value for the bookmark.

I'm not English mother tongue, but the phrasing sounds strange?

::: toolkit/components/places/Bookmarks.jsm:1528
(Diff revision 1)
>   * @param {Object} item The bookmark item with possible special data to be inserted.
>   */
>  async function handleBookmarkItemSpecialData(itemId, item) {
>    if (item.annos && item.annos.length) {
> -    PlacesUtils.setAnnotationsForItem(itemId, item.annos, item.source)
> +    // Note: we intentionally for annotations to skip updating the last modified
> +    // value for the bookmark.

ditto

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 8915082 [details]
Bug 1405317 - When inserting trees of bookmarks, avoid updating the last modified date when marking annotations.

https://reviewboard.mozilla.org/r/186348/#review191380

I took a look, we only bump the last modified of the tag when we modify it, we don't bump the last modified of the bookmark being tagged.

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 8915082 [details]
Bug 1405317 - When inserting trees of bookmarks, avoid updating the last modified date when marking annotations.

https://reviewboard.mozilla.org/r/186348/#review191380

We also discussed over irc about the nsNavBookmark.cpp changes, and decided to make it so that the onItemChanged notification for the annotation was sent out regardless of if we were modifying the lastModified date or not - as there are various things relying on it.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8915082 [details]
Bug 1405317 - When inserting trees of bookmarks, avoid updating the last modified date when marking annotations.

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/71267c7616d7
When inserting trees of bookmarks, avoid updating the last modified date when marking annotations. r=mak

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/71267c7616d7

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 8915082 [details]
Bug 1405317 - When inserting trees of bookmarks, avoid updating the last modified date when marking annotations.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1095426
[User impact if declined]: Restoring a (json) bookmarks file could cause bookmarks with descriptions or other annotations to have the wrong last modified time.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes, I just checked it.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]:  No
[Why is the change risky/not risky?]: Simple addition of a parameter to turn of changing of the last modified time when we shouldn't change it. Most of the places code has a reasonable amount of unit testing as well.
[String changes made/needed]: None

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8915082 [details]
Bug 1405317 - When inserting trees of bookmarks, avoid updating the last modified date when marking annotations.

This doesn't seem like a simple change based on the patch size. Also this isn't a new regression in 57. Unless there are very strong reasons to uplift this, I'd prefer letting this one ride the 58 train.

Comment 11

[Cornel Ionce [:noni] [Hubs QA]]

(In reply to Mark Banner (:standard8) from comment #9)
> [Is this code covered by automated tests?]: Yes
> [Needs manual test from QE? If yes, steps to reproduce]: No

Marking as qe- since this issue has automated coverage.