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)

enhancement

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.
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.
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.
Status: NEW → ASSIGNED
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+
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.
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
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.

Attachment

General

Created:
Updated:
Size: