Closed
Bug 1466856
Opened 6 years ago
Closed 6 years ago
Importing old bookmark backups doesn't translate queries for folders
Categories
(Firefox :: Bookmarks & History, enhancement, P1)
Firefox
Bookmarks & History
Tracking
()
RESOLVED
FIXED
Firefox 62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: standard8, Assigned: standard8)
Details
(Whiteboard: [fxsearch])
Attachments
(1 file)
When I implemented the transition from folder ids to guids for queries, I forgot to also change things so that old backups would be imported correctly as well - they should also have their queries updated.
Assignee | ||
Comment 1•6 years ago
|
||
It turns out we only currently handle folder= query imports for JSON. I'm not sure if this is by design or not. It is probably less likely that we'd be receiving queries for import via HTML. Marco - let me know if you think we should handle HTML. I might push that to a separate bug.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•6 years ago
|
||
The nice thing about the tidy up here, is we get to simplify a lot of the json import, as well as avoiding the extra fetches & updates.
Updated•6 years ago
|
Status: NEW → ASSIGNED
Comment 4•6 years ago
|
||
mozreview-review |
Comment on attachment 8983553 [details] Bug 1466856 - Handle old style folder=<id> queries correctly when importing bookmarks from JSON. https://reviewboard.mozilla.org/r/249406/#review255778 ::: toolkit/components/places/BookmarkJSONUtils.jsm:318 (Diff revision 1) > + * > + * @param {Object} aNode The node to search. > + * @param {Array} aFolderIdMap An array mapping of old folder IDs to new folder GUIDs. > + */ > +function fixupSearchQueries(aNode, aFolderIdMap) { > + if (aNode.url && aNode.url.substr(0, 6) == "place:") { startsWith ::: toolkit/components/places/BookmarkJSONUtils.jsm:343 (Diff revision 1) > */ > -async function fixupQuery(aQueryURI, aFolderIdMap) { > - const reGlobal = /folder=([0-9]+)/g; > - const re = /([0-9]+)/; > - > - // Unfortunately .replace can't handle async functions. Therefore, > +function fixupQuery(aQueryURL, aFolderIdMap) { > + let invalid = false; > + let convert = function(str, existingFolderId) { > + let guid; > + if (Object.getOwnPropertyNames(OLD_BOOKMARK_QUERY_TRANSLATIONS).includes(existingFolderId)) { Object.keys? ::: toolkit/components/places/BookmarkJSONUtils.jsm:358 (Diff revision 1) > }; > - return uri.replace(reGlobal, convert); > + > + let url = aQueryURL.replace(/folder=([A-Za-z0-9_]+)/g, convert); > + if (invalid) { > + // One or more of the folders don't exist, cause an empty query so that > + // we don't start trying to display the whole database. nit: s/start trying/try/ ::: toolkit/components/places/tests/unit/test_import_mobile_bookmarks.js:137 (Diff revision 1) > }, "Should restore mobile bookmark folder contents into mobile root"); > > - // We rewrite queries to point to the root ID instead of the name > + // We rewrite queries to point to the mobile root GUID instead of the name > // ("MOBILE_BOOKMARKS") so that we don't break them if the user downgrades > // to an earlier release channel. This can be removed along with the anno in > // bug 1306445. Hm, that bug is fixed... Worth asking Sync team if this is a leftover :)
Attachment #8983553 -
Flags: review?(mak77) → review+
Assignee | ||
Comment 5•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8983553 [details] Bug 1466856 - Handle old style folder=<id> queries correctly when importing bookmarks from JSON. https://reviewboard.mozilla.org/r/249406/#review255778 > Hm, that bug is fixed... Worth asking Sync team if this is a leftover :) I think the test is still fine to have there, but I think the comment is obsolete. The action is a bookmarks import, and we're not changing the query when we export. The test is still valid though - we might as well check we get the right mobile folder guid.
Comment hidden (mozreview-request) |
Pushed by mbanner@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c02026018928 Handle old style folder=<id> queries correctly when importing bookmarks from JSON. r=mak
Comment 8•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c02026018928
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
You need to log in
before you can comment on or make changes to this bug.
Description
•