1435145 - Handle children moved out of a deleted synced folder into a different subtree

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P1)

Description

[Lina Butler (ex-Mozilla)]

The fix for bug 1434800 is (embarrassingly :-) incomplete. It handles the case where a child moved out of a deleted folder and into a folder that we already walked, but not where a child moved into a different folder that we didn't walk yet.

Comment 1

[Lina Butler (ex-Mozilla)]

Attachment: Bug 1435145 - Handle children moved out of a deleted synced folder into a different subtree.

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

Comment 2

[Lina Butler (ex-Mozilla)]

Comment on attachment 8947707 [details]
Bug 1435145 - Handle children moved out of a deleted synced folder into a different subtree.

https://reviewboard.mozilla.org/r/217430/#review223176

::: toolkit/components/places/SyncedBookmarksMirror.jsm:3308
(Diff revision 1)
> -    let locallyMovedOrDeleted = this.checkForLocalMoveOrDeletionOfRemoteNode(
> -      mergedNode, remoteChildNode);
> -    if (locallyMovedOrDeleted) {
> +    let structureChange = this.checkForLocalStructureChangeOfRemoteNode(
> +      mergedNode, remoteParentNode, remoteChildNode);
> +    if (structureChange == BookmarkMerger.STRUCTURE.DELETED) {
>        return true;
>      }
>  

This is the important part: we still want to merge the child if it's *moved*, because the logic below this check decides where to keep it. I expanded the comment above this function to clarify that the two are separate cases.

::: toolkit/components/places/SyncedBookmarksMirror.jsm:3741
(Diff revision 1)
>      for (let remoteChildNode of remoteNode.children) {
> -      let locallyMovedOrDeleted = this.checkForLocalMoveOrDeletionOfRemoteNode(
> -        mergedNode, remoteChildNode);
> -      if (locallyMovedOrDeleted) {
> -        // The remote child doesn't exist locally, or is already moved or
> -        // deleted locally, so we can safely ignore it.
> +      let structureChange = this.checkForLocalStructureChangeOfRemoteNode(
> +        mergedNode, remoteNode, remoteChildNode);
> +      if (structureChange == BookmarkMerger.STRUCTURE.MOVED ||
> +          structureChange == BookmarkMerger.STRUCTURE.DELETED) {
> +        // The remote child is already moved or deleted locally, so we should

Here, however, the child can be moved or deleted, and both should stop us from treating it as an orphan.

Comment 3

[Lina Butler (ex-Mozilla)]

Comment on attachment 8947707 [details]
Bug 1435145 - Handle children moved out of a deleted synced folder into a different subtree.

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

Comment 4

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8947707 [details]
Bug 1435145 - Handle children moved out of a deleted synced folder into a different subtree.

https://reviewboard.mozilla.org/r/217430/#review223184

Comment 5

[Lina Butler (ex-Mozilla)]

Comment on attachment 8947707 [details]
Bug 1435145 - Handle children moved out of a deleted synced folder into a different subtree.

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

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9ffe77b5c891
Handle children moved out of a deleted synced folder into a different subtree. r=markh

Comment 7

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/9ffe77b5c891