1104796 - Bookmarks.remove disallows removing any direct child of the places root

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Asaf Romano (gone)]

In order to block consumers trying to remove the bookmarks toolbar/bookmarks menu/unfiled bookmarks/tag special folders, Bookmarks.remove blocks removing anything that is parent is the places root. This is too strict because:
1) It will block replacing the left pane root (which is created under the places root).
2) It will break tests that create stuff under the places root and wish to remove it.

Comment 1

[Asaf Romano (gone)]

Attachment: patch

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8528438 [details] [diff] [review]
patch

Review of attachment 8528438 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/Bookmarks.jsm
@@ +423,2 @@
>          throw new Error("It's not possible to remove Places root folders.");
> +      }

I think we could just do this before Task.spawn, before we needed to fetch the parentGuid, but here we do direct guid comparison and we already have the guid from arguments.

This is made possible from the new constant guids, it was not possible before.

@@ +443,5 @@
>        }
>  
>        // Remove non-enumerable properties.
>        return Object.assign({}, item);
> +    }.bind(this));

and then you don't need this.

::: toolkit/components/places/tests/bookmarks/test_bookmarks_remove.js
@@ +46,5 @@
>  add_task(function* remove_roots_fail() {
> +  let guids = [PlacesUtils.bookmarks.rootGuid,
> +               PlacesUtils.bookmarks.menuGuid,
> +               PlacesUtils.bookmarks.toolbarGuid,
> +               PlacesUtils.bookmarks.tagsGuid];

missing unfiled?

@@ +49,5 @@
> +               PlacesUtils.bookmarks.toolbarGuid,
> +               PlacesUtils.bookmarks.tagsGuid];
> +  for (let guid of guids) {
> +    try {
> +      yield PlacesUtils.bookmarks.remove(guid);

no more need to yield, then.

Comment 3

[Asaf Romano (gone)]

Attachment: patch

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8528802 [details] [diff] [review]
patch

Review of attachment 8528802 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/tests/bookmarks/test_bookmarks_remove.js
@@ +54,5 @@
> +      PlacesUtils.bookmarks.remove(guid);
> +      Assert.ok(false, "Should have thrown");
> +    } catch (ex) {
> +      Assert.ok(/It's not possible to remove Places root folders/.test(ex));
> +    }

Sorry I was unclear in the previous comment, here you can do

Assert.throws(() => PlacesUtils.bookmarks.remove(guid),
              /It's not possible to remove Places root folders/);

Comment 5

[Asaf Romano (gone)]

https://hg.mozilla.org/integration/fx-team/rev/54f97b545e4c

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/54f97b545e4c

Comment 7

[Andrei Vaida [:avaida]]

Is manual QA coverage required for this patch? If so, could you please elaborate a bit on what should be verified here?

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

no, manual QA is not required, this is not yet exposed in the product and covered by unit tests.