provide better error message when attempt is made to add bookmark folder to root folder
Categories
(WebExtensions :: General, enhancement, P5)
Tracking
(firefox68 fixed)
| Tracking | Status | |
|---|---|---|
| firefox68 | --- | fixed |
People
(Reporter: dietrich, Assigned: myeongjun.ko, Mentored)
Details
(Keywords: dev-doc-complete, good-first-bug)
Attachments
(1 file)
And update docs too, ty!
|
||
Comment 1•5 years ago
|
||
Could you provide some more detail here so that someone new could pick it up actionably? Thanks!
|
Reporter | |
Comment 2•5 years ago
|
||
Sure! John-galt asked me to file it, so he will probably let us know what is correctly actionable, but some details on my experience are below.
Pseudo-code:
let root = (await bookmarks.getTree())[0];
let myFolder = await bookmarks.create({ title: "my folder", parentId: root.id });
This throws an error because the policy is that extensions cannot add items to the root node.
Two things should probably happen:
1. The error message doesn't say anything helpful. I can't remember what it says, but was nothing related to the root (hehe) cause of the error - or at least no discernible to me. The error message should be updated to inform the developer what has happened.
2. I found out about the policy by asking on IRC, even after reading through to docs. There should probably be documentation for it.
|
||
Comment 3•5 years ago
|
||
If this is your first contribution, please see https://wiki.mozilla.org/WebExtensions/Contribution_Onramp for how to get started. Mentor: Rob Wu
Hello, i would like to try this out as my first contribution
|
||
Comment 5•5 years ago
|
||
Hi Pollet, comment 2 has described a test case to manually check the behavior of the bookmarks API.
This is extension code, so if you aren't familiar with the area yet, check out https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions
To get started with setting up a development environment and submitting a patch, take a look at the tutorial at
https://wiki.mozilla.org/WebExtensions/Contribution_Onramp
The relevant code is in browser/components/extensions/parent/ext-bookmarks.js , which you can view at https://searchfox.org/mozilla-central/source/browser/components/extensions/parent/ext-bookmarks.js
If you have any other questions, feel free to needinfo me.
|
Assignee | |
Comment 7•5 years ago
|
||
Is this bug don't solve yet?
When create function call, create function check if it is root node. And Is it throw error such as throw new Error("This id is root node")?
|
|
Assignee | |
Comment 8•5 years ago
|
||
(In reply to Myeongjun Go from comment #7)
Is this bug don't solve yet?
Whencreate functioncall,create functioncheck if it is root node. And Is it throw error such asthrow new Error("This id is root node")?
I didn't refer to mentor.
Please, Check my comment. If it is correct and pollet doesn't try anymore, I wanna try to solve it.
|
|
||
Comment 9•5 years ago
|
||
The bug is still open. The fix that you described sounds right!
This is not only an issue with the browser.bookmarks.create method, but also every other method of the bookmarks API. So to fix the bug, please create a helper function that throws ExtensionError("The root folder cannot be modified") when the given ID is the root ID, and call this helper function for each of the following methods (for the properties/variables between parentheses):
create(bookmark.parentId)move(idanddestination.parentId)update(id)remove(id)removeTree(id)
You should also include a unit test to verify that the fix is working as expected, for example by appending test cases to https://searchfox.org/mozilla-central/source/browser/components/extensions/test/xpcshell/test_ext_bookmarks.js
|
|
Assignee | |
Comment 10•5 years ago
|
||
Thanks for replying :)
I start to work on issue. I'm going to make helper function right over create function. Function name is going to be named checkRootId(id). Or, You could recommend proper name.
|
|
||
Comment 11•5 years ago
|
||
I would go with throwIfRootId.
Once you've submitted a patch to Phabricator I'll assign this bug to you :)
|
|
Assignee | |
Comment 12•5 years ago
|
||
|
|
Assignee | |
Comment 13•5 years ago
|
||
(In reply to Myeongjun Go from comment #12)
I fixed it :)
If It is needed to revise after you're review patch, Tell me about problems.
|
|
||
Comment 14•5 years ago
|
||
Thanks for the patch! I'll take a look.
|
|
Assignee | |
Comment 15•5 years ago
|
||
I fixed it again and submit patch.
Thank you very much about reviewing kindly :)
|
|
||
Comment 16•5 years ago
|
||
You're welcome, and thanks for your contribution!
The next step is getting a review from a module peer (which I'm not; I've added Luca to your review - expect a reaction tomorrow or next week).
Once he has approved your patch, you can edit this bug and add the checkin-needed keyword to request a check-in of your patch.
Before requesting a check-in, do make sure that your patch passes the relevant tests (see https://searchfox.org/mozilla-central/search?q=browser.bookmarks&case=false®exp=false&path=extensions%2Ftest ).
|
||
Comment 17•5 years ago
|
||
Added dev-doc-needed keyword, as we should also update the API docs on MDN to mention this behavior explicitly.
|
|
Assignee | |
Comment 18•5 years ago
|
||
Should I update the api docs on MDN to merge my patch?
|
|
Assignee | |
Updated•5 years ago
|
|
||
Comment 19•5 years ago
|
||
Pushed by nerli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/185cd658ce9a
Provide better error message when attempt is made to add bookmark folder to root folder. r=robwu,rpl
|
||
Comment 20•5 years ago
|
||
| bugherder | ||
|
|
||
Comment 21•5 years ago
|
||
(In reply to Myeongjun Go from comment #18)
Should I update the api docs on MDN to merge my patch?
The documentation is only updated after a patch lands.
In this case, it seems best to add a brief explanation as a new (third) paragraph of the introduction section at https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/bookmarks
|
|
||
Comment 22•5 years ago
|
||
Congrats on landing the patch, Myeongjun! 🎉 Your contribution has been added to our contribution wiki at https://wiki.mozilla.org/Add-ons/Contribute/Recognition and your Mozillians.org profile has been vouched. 🚀
Welcome onboard! I look forward to seeing you around the project.
|
||
Comment 23•5 years ago
|
||
Can you please provide some STR for this issue so we can check it manually? If no manual testing is needed please mark it as "qe-verify- "
|
|
||
Comment 24•5 years ago
|
||
Covered by automated tests, so qe-verify-.
|
||
Comment 25•5 years ago
|
||
Here are the updates I made. I'm not entirely sure it needs to be formatted as a warning. Let me know if you would like to change that or make it a note instead. Also, please review my changes and let me know if I missed anything!
-
Added the following warning to Bookmarks API:
Extensions cannot create, modify, or delete bookmarks in the root node of the bookmarks tree. Doing so causes an error with the message: "The bookmark root cannot be modified"
-
Added the following warning to the bookmarks.create method:
If your extension tries to create a new bookmark in the bookmark tree's root node, it raises an error: "The bookmark root cannot be modified" and the bookmark won't be created.
-
Added the following warning to bookmarks.move method:
If your extension attempts to move a bookmark into the bookmarks tree root node, the call will raise an error with the message: "The bookmark root cannot be modified" and the move won't be completed.
-
Added the following to warning bookmarks.remove:
If your extension attempts to remove a bookmark from the bookmarks tree root node, the call will raise an error with the message: "The bookmark root cannot be modified" and the bookmark won't be removed.
-
Added the following message to bookmarks.removeTree:
If your extension attempts to remove a bookmark tree from the bookmarks tree root node, the call will raise an error with the message: "The bookmark root cannot be modified" and the bookmark won't be removed.
-
Added the following message to bookmarks.update:
If your extension attempts to update a bookmark in the bookmarks tree root node, the call will raise an error with the message: "The bookmark root cannot be modified" and the bookmark won't be updated.
Description
•