Closed
Bug 1405317
Opened 7 years ago
Closed 7 years ago
Restoring bookmarks from json files ignores lastModified for bookmarks with an annotation
Categories
(Toolkit :: Places, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox56 | --- | wontfix |
firefox57 | --- | wontfix |
firefox58 | --- | fixed |
People
(Reporter: standard8, Assigned: standard8)
References
Details
(Keywords: regression, Whiteboard: [fxsearch])
Attachments
(1 file)
(deleted),
text/x-review-board-request
|
mak
:
review+
ritu
:
approval-mozilla-beta-
|
Details |
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 hidden (mozreview-request) |
Comment 2•7 years ago
|
||
mozreview-review |
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•7 years ago
|
||
mozreview-review |
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
Attachment #8915082 -
Flags: review?(mak77) → review+
Assignee | ||
Comment 4•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 5•7 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Updated•7 years ago
|
Status: NEW → ASSIGNED
Pushed by mbanner@mozilla.com:
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•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Updated•7 years ago
|
status-firefox-esr52:
--- → unaffected
Version: Trunk → 56 Branch
Assignee | ||
Comment 9•7 years ago
|
||
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
Attachment #8915082 -
Flags: approval-mozilla-beta?
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.
Attachment #8915082 -
Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment 11•7 years ago
|
||
(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.
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•