1309332 - Don't return a Places bookmark object from `PlacesSyncUtils.bookmarks.remove`

Status: RESOLVED WONTFIX

(Firefox :: Sync, defect)

Description

[Lina Butler (ex-Mozilla)]

A tiny follow-up to bug 1295521: `PlacesSyncUtils.bookmarks.remove` still returns a Places bookmark instead of a Sync bookmark.

I could change it so that it fetches the bookmark, converts it to a Places bookmark, then removes it, but that seems wasteful...especially since `PlacesUtils.bookmarks.remove` will fetch it yet again. Since we don't actually use the returned object for anything, it can log errors and return a Boolean.

Comment 1

[Lina Butler (ex-Mozilla)]

Attachment: Bug 1309332 - Don't return a Places bookmark object from `PlacesSyncUtils.bookmarks.remove`.

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

Comment 2

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8799908 [details]
Bug 1309332 - Don't return a Places bookmark object from `PlacesSyncUtils.bookmarks.remove`.

https://reviewboard.mozilla.org/r/84978/#review83532

::: services/sync/modules/engines/bookmarks.js:685
(Diff revision 1)
>  
>    remove: function BStore_remove(record) {
> -    try {
> -      let info = Async.promiseSpinningly(PlacesSyncUtils.bookmarks.remove(record.id));
> +    let wasRemoved = Async.promiseSpinningly(PlacesSyncUtils.bookmarks.remove(record.id));
> +    if (wasRemoved) {
> -      if (info) {
> -        this._log.debug(`Removed item ${record.id} with type ${record.type}`);
> +      this._log.debug(`Removed item ${record.id} with type ${record.type}`);

Won't type usually be `"item"` here? Remember `remove` is called with records where `r.deleted === true`.

Comment 3

[Lina Butler (ex-Mozilla)]

Comment on attachment 8799908 [details]
Bug 1309332 - Don't return a Places bookmark object from `PlacesSyncUtils.bookmarks.remove`.

https://reviewboard.mozilla.org/r/84978/#review83532

> Won't type usually be `"item"` here? Remember `remove` is called with records where `r.deleted === true`.

Ack, yes, you told me this before. :-) This log line isn't helpful, then. I guees fetch, convert, and delete is the right thing to do, then? Or we can leave it.

Comment 4

[Lina Butler (ex-Mozilla)]

Comment on attachment 8799908 [details]
Bug 1309332 - Don't return a Places bookmark object from `PlacesSyncUtils.bookmarks.remove`.

Per IRC...Thom rewrote this function in bug 1299978, and the important methods already use sync IDs, so it's not worth it.