1512171 - provide better error message when attempt is made to add bookmark folder to root folder

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P5)

Description

[Dietrich Ayala (:dietrich)]

And update docs too, ty!

Comment 1

[David Durst [:ddurst]]

Could you provide some more detail here so that someone new could pick it up actionably? Thanks!

Comment 2

[Dietrich Ayala (:dietrich)]

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

[Caitlin Neiman [:caitmuenster]]

If this is your first contribution, please see https://wiki.mozilla.org/WebExtensions/Contribution_Onramp for how to get started. 

Mentor: Rob Wu

Comment 4

[pollet]

Hello, i would like to try this out as my first contribution

Comment 5

[Rob Wu [:robwu]]

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.

Comment 6

[pollet]

okay thank you

Comment 7

[Myeongjun Go]

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")?

Comment 8

[Myeongjun Go]

(In reply to Myeongjun Go from comment #7)

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")?

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

[Rob Wu [:robwu]]

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 (id and destination.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

Comment 10

[Myeongjun Go]

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

[Rob Wu [:robwu]]

I would go with throwIfRootId.

Once you've submitted a patch to Phabricator I'll assign this bug to you :)

Comment 12

[Myeongjun Go]

Attachment: Bug 1512171 - Provide better error message when attempt is made to add bookmark folder to root folder. r=robwu

Comment 13

[Myeongjun Go]

(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

[Rob Wu [:robwu]]

Thanks for the patch! I'll take a look.

Comment 15

[Myeongjun Go]

I fixed it again and submit patch.
Thank you very much about reviewing kindly :)

Comment 16

[Rob Wu [:robwu]]

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&regexp=false&path=extensions%2Ftest ).

Comment 17

[Luca Greco [:rpl] [:luca] [:lgreco]]

Added dev-doc-needed keyword, as we should also update the API docs on MDN to mention this behavior explicitly.

Comment 18

[Myeongjun Go]

Should I update the api docs on MDN to merge my patch?

Comment 19

[Pulsebot]

Pushed by [e-mail]:
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

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/185cd658ce9a

Comment 21

[Rob Wu [:robwu]]

(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

[Caitlin Neiman [:caitmuenster]]

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

[Madalin Cotetiu]

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

[Rob Wu [:robwu]]

Covered by automated tests, so qe-verify-.

Comment 25

[Irene Smith]

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.

Comment 26

[Rob Wu [:robwu]]

Looks good, thanks!