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

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
WebExtensions: General
P2
normal
RESOLVED FIXED
a month ago
a month ago

People

(Reporter: bsilverberg, Assigned: bsilverberg)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [bookmarks] triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a month ago
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
(Assignee)

Updated

a month ago
See Also: → bug 1347386
(Assignee)

Comment 1

a month ago
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)
(Assignee)

Comment 2

a month ago
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)
(Assignee)

Comment 4

a month ago
(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 hidden (mozreview-request)

Comment 7

a month ago
mozreview-review
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+

Comment 8

a month ago
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
Last Resolved: a month ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.