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

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P2)

Description

[Bob Silverberg [:bsilverberg]]

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

Comment 1

[Bob Silverberg [:bsilverberg]]

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.

Comment 2

[Bob Silverberg [:bsilverberg]]

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`?

Comment 3

[Marco Bonardo [:mak]]

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.

Comment 4

[Bob Silverberg [:bsilverberg]]

(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.

Comment 5

[Marco Bonardo [:mak]]

(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?

Comment 6

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1358127 - Fix bookmarks.search so it doesn't return the contents of tag folders,

Also fix bookmarks.search so it doesn't return separators.

Review commit: https://reviewboard.mozilla.org/r/133076/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/133076/

Comment 7

[Marco Bonardo [:mak]]

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!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/81a7160c5cbc
Fix bookmarks.search so it doesn't return the contents of tag folders, r=mak

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/81a7160c5cbc