Closed Bug 1104796 Opened 6 years ago Closed 6 years ago

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

Categories

(Toolkit :: Places, defect)

defect
Not set
normal
Points:
1

Tracking

()

RESOLVED FIXED
mozilla37
Iteration:
37.1

People

(Reporter: mano, Assigned: mano)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch patch (obsolete) — Splinter Review
Attachment #8528438 - Flags: review?(mak77)
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.
Attachment #8528438 - Flags: review?(mak77) → feedback+
Attached patch patchSplinter Review
Attachment #8528438 - Attachment is obsolete: true
Attachment #8528802 - Flags: review?(mak77)
Status: NEW → ASSIGNED
Flags: qe-verify?
Flags: firefox-backlog+
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/);
Attachment #8528802 - Flags: review?(mak77) → review+
https://hg.mozilla.org/mozilla-central/rev/54f97b545e4c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Is manual QA coverage required for this patch? If so, could you please elaborate a bit on what should be verified here?
Flags: needinfo?(mano)
no, manual QA is not required, this is not yet exposed in the product and covered by unit tests.
Flags: qe-verify?
Flags: qe-verify-
Flags: needinfo?(mano)
You need to log in before you can comment on or make changes to this bug.