Closed Bug 1433697 Opened 2 years ago Closed 2 years ago

Handle deletions that don't exist on one side when merging synced bookmarks

Categories

(Firefox :: Sync, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: Lina, Assigned: Lina)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

On a first sync, the server will likely have tombstones for items that don't exist locally. Our `subsumes` check is overzealous, and complains that those remote tombstones aren't mentioned in the merged tree...which is correct, but expected in this case.

I think we'll want to add remaining local deletions that aren't already mentioned in the merged tree to `deleteRemotely`, and remaining remote deletions to `deleteLocally`.

With this fixed, I can sync my tree. \o/
Comment on attachment 8946091 [details]
Bug 1433697 - Handle deletions that don't exist on one side when merging synced bookmarks.

https://reviewboard.mozilla.org/r/216116/#review221872

::: toolkit/components/places/SyncedBookmarksMirror.jsm:1097
(Diff revision 1)
>        await this.db.execute(`
>          UPDATE items SET
>            needsMerge = 0
>          WHERE needsMerge AND
> -              guid IN (${guidsParams})`,
> -        guids);
> +              guid IN (SELECT guid FROM itemsRemoved
> +                       WHERE NOT isUntagging)`);

I decided to keep tombstones for bookmarks that don't exist locally as `needsMerge = 1`, so that we can add them to `moz_bookmarks_deleted` once we start persisting them in bug 1343103.

Not strictly related to the fix, but I also realized the `guids` array is unnecessary, since we just inserted IDs and GUIDs for everything we're going to delete into `itemsRemoved`. The `NOT isUntagging` is for formality more than anything, and can go away once we stop using bookmarks to represent tags.
Comment on attachment 8946091 [details]
Bug 1433697 - Handle deletions that don't exist on one side when merging synced bookmarks.

https://reviewboard.mozilla.org/r/216116/#review221876


Static analysis found 1 defect in this patch.
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/places/tests/sync/test_bookmark_deletion.js:599
(Diff revision 1)
> +  // value.
> +  let menuDateAdded = new Date();
> +  await PlacesUtils.bookmarks.update({
> +    guid: PlacesUtils.bookmarks.menuGuid,
> +    dateAdded: menuDateAdded,
> +  })

Error: Missing semicolon. [eslint: semi]
Comment on attachment 8946091 [details]
Bug 1433697 - Handle deletions that don't exist on one side when merging synced bookmarks.

https://reviewboard.mozilla.org/r/216116/#review222076

This seems to make sense.

::: toolkit/components/places/SyncedBookmarksMirror.jsm:1068
(Diff revision 2)
>          guids);
>  
> -      let guidsParams = new Array(guids.length).fill("?").join(",");
> -
>        // Recalculate frecencies.
>        await this.db.execute(`

Can you add a comment explaining that the `NOT isUntagging` is "for formality more than anything, and can go away once we stop using bookmarks to represent tags."?
Attachment #8946091 - Flags: review?(tchiovoloni) → review+
Priority: -- → P1
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3b0bd7f88df7
Handle deletions that don't exist on one side when merging synced bookmarks. r=tcsc
https://hg.mozilla.org/mozilla-central/rev/3b0bd7f88df7
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
You need to log in before you can comment on or make changes to this bug.