Closed
Bug 1104796
Opened 10 years ago
Closed 10 years ago
Bookmarks.remove disallows removing any direct child of the places root
Categories
(Toolkit :: Places, defect)
Toolkit
Places
Tracking
()
People
(Reporter: asaf, Assigned: asaf)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8528438 -
Flags: review?(mak77)
Comment 2•10 years ago
|
||
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+
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8528438 -
Attachment is obsolete: true
Attachment #8528802 -
Flags: review?(mak77)
Updated•10 years ago
|
Status: NEW → ASSIGNED
Flags: qe-verify?
Flags: firefox-backlog+
Comment 4•10 years ago
|
||
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+
Assignee | ||
Comment 5•10 years ago
|
||
Comment 6•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Comment 7•10 years ago
|
||
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)
Comment 8•10 years ago
|
||
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.
Description
•