Closed Bug 1358127 Opened 3 years ago Closed 3 years ago

Fix bookmarks.search so it doesn't return the contents of tag folders

Categories

(WebExtensions :: General, enhancement, P2)

enhancement

Tracking

(firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: bsilverberg, Assigned: bsilverberg)

References

Details

(Whiteboard: [bookmarks] triaged)

Attachments

(1 file)

This is a spinoff from Bug 1347386.

The logic in queryBookmarks in Bookmarks.jsm [1], which is used by bookmarks.search() contains logic that excludes the tags folder, but not the contents of the tags folder, as per Marco's comment on bug 1347386 [2].

This should be fixed.

[1] http://searchfox.org/mozilla-central/source/toolkit/components/places/Bookmarks.jsm#1312
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1347386#c10
See Also: → 1347386
Marco, I'm trying to understand what needs to be changed here, but I'm a bit confused. How does the query need to be changed to check both the parent and the grandparent. I'm afraid I still don't understand how all this tag stuff works.
Flags: needinfo?(mak77)
Looking at other queries in Bookmarks.jsm, it looks like, in queryBookmarks, b.parent would be the parent, and p.parent would be the grandparent, and we are saying `WHERE p.parent <> :tags_folder`, so doesn't that address the grandparents? Do we also need `WHERE b.parent <> :tags_folder`?
tags are folders having parentGuid = tagsGuid (or parentId = tagsFolderId)
When you tag a bookmark (url), it gets cloned into the tag folder.
This is an internal implementation detail that will change in the future, for now any APIs must filter out those, since they are just duplicates and are not intended to be managed by normal bookmarks APIs.

so yes, you need to check both parent and grandparent.

Other question, is queryBookmarks supposed to also return separators and folders? Cause it doesn't seem to filter on type here.
Flags: needinfo?(mak77)
(In reply to Marco Bonardo [::mak] from comment #3)
> tags are folders having parentGuid = tagsGuid (or parentId = tagsFolderId)
> When you tag a bookmark (url), it gets cloned into the tag folder.
> This is an internal implementation detail that will change in the future,
> for now any APIs must filter out those, since they are just duplicates and
> are not intended to be managed by normal bookmarks APIs.
> 
> so yes, you need to check both parent and grandparent.

Thanks Marco, although I'm still not sure _how_ to do that. As I mention in comment #2, it looks like we are already filtering on grandparent. Do I just need to add an additional filter for `WHERE b.parent <> :tags_folder` or is something more than that needed?

> 
> Other question, is queryBookmarks supposed to also return separators and
> folders? Cause it doesn't seem to filter on type here.

For now it should not return seperators, but should return folders. I assumed it was currently doing both correctly, but I can test that.
Flags: needinfo?(mak77)
(In reply to Bob Silverberg [:bsilverberg] from comment #4)
> Do I just
> need to add an additional filter for `WHERE b.parent <> :tags_folder` or is
> something more than that needed?

Yes, that's correct.

> For now it should not return seperators, but should return folders. I
> assumed it was currently doing both correctly, but I can test that.

there is nothing filtering out separators here, unless you pass a valid url or query, but none of those seem to be required?
Flags: needinfo?(mak77)
Comment on attachment 8861116 [details]
Bug 1358127 - Fix bookmarks.search so it doesn't return the contents of tag folders,

https://reviewboard.mozilla.org/r/133076/#review136894

LGTM, thank you!
Attachment #8861116 - Flags: review?(mak77) → review+
Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/81a7160c5cbc
Fix bookmarks.search so it doesn't return the contents of tag folders, r=mak
https://hg.mozilla.org/mozilla-central/rev/81a7160c5cbc
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.