653816 - GetBookmarkIdsForURI should not return tags and should be deprecated for Bookmarks.jsm API

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P3)

Description

[Marco Bonardo [:mak]]

This API is currently returning all ids for a URI, included fake-bookmarks made for tags :(
In the scope of making tags NOT-BOOKMARKS, the API users (internal and addons) should be audited, and it should be changed to return only real bookmarks.

Comment 1

[Milind L (:milindl)]

I'd like to work on this, but I am slightly unsure of what I should do.

There is an option `skipTag` inside the method with GetBookmarkIdsForURI seems to be using, but we pass it false. Is passing this as true all that's needed, or should I modify the query inside GetBookmarkIdsForURITArray to exclude fake bookmarks for tagging use? (this I guess is possible since GetBookmarkIdsForURI seems to be the only consumer for the TArray method I can find using searchfox).

Of course after this I need to change the consumers, it looks like many of them don't need the tags so should not be hard to remove, but nsTaggingService.js::getTagsForURI seems to need them, I am unsure how to facilitate that with the methods available.

Thanks :)

Comment 2

[Marco Bonardo [:mak]]

I think that the only consumer of GetBookmarkIdsForURI expecting it to return tags is indeed the tagging service. We can make the tagging service run its own custom query for now, using PlacesUtils.history.DBConnection.createStatement (legacy synchronous querying...).
That would be fine considered it's an interim solution and in the future GetBookmarkIdsForURI will go away, as well as the tagging service should be rewritten to be async (or merge with Bookmarks.jsm, TBD).
Thus, I'd suggest to replace the tagging call with a synchronous custom query, flip the false to true inside GetBookmarkIdsForURI and see which tests fail.

Comment 3

[Milind L (:milindl)]

Attachment: Bug 653816 - returning only nontags for GetBookmarkIdsForURI and fixing consumers,

Most consumers of `GetBookmarkIdsForURI` already don't need tags, the only
consumer which does (`TaggingService`) has been changed to use a separate
database query.

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

Comment 4

[Milind L (:milindl)]

so this is a preliminary patch, the idea above is what I've tried to do. A few questions, should I issue a deprecation warning inside the function using Deprecation.jsm, or should that wait until we are done creating an alternative for this?


The tests pass locally. (Tests containing GetBookmarkIdsForURI and those containing getTagsForURI were the ones I ran).

Comment 5

[Marco Bonardo [:mak]]

(In reply to Milind L (:milindl) from comment #4)
> so this is a preliminary patch, the idea above is what I've tried to do. A
> few questions, should I issue a deprecation warning inside the function
> using Deprecation.jsm, or should that wait until we are done creating an
> alternative for this?

We can't issue deprecation warnings until all the internal consumers are converted.

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 8872634 [details]
Bug 653816 - returning only nontags for GetBookmarkIdsForURI and fixing consumers,

https://reviewboard.mozilla.org/r/144172/#review148698

::: toolkit/components/places/PlacesUtils.jsm:1349
(Diff revision 1)
>     * @returns itemId of the found bookmark, or -1 if nothing is found.
>     */
>    getMostRecentBookmarkForURI:
>    function PU_getMostRecentBookmarkForURI(aURI) {
> -    var bmkIds = this.bookmarks.getBookmarkIdsForURI(aURI);
> -    for (var i = 0; i < bmkIds.length; i++) {
> +    let bmkIds = this.bookmarks.getBookmarkIdsForURI(aURI);
> +    return bmkIds[0] ? bmkIds[0] : -1;

it would probably be better and more readable to check bmkIds.length rather than bmkIds[0]

::: toolkit/components/places/nsNavBookmarks.cpp:2688
(Diff revision 1)
>    *aCount = 0;
>    *aBookmarks = nullptr;
>    nsTArray<int64_t> bookmarks;
>  
>    // Get the information from the DB as a TArray
> -  // TODO (bug 653816): make this API skip tags by default.
> +  nsresult rv = GetBookmarkIdsForURITArray(aURI, bookmarks, true);

this is the only call to GetBookmarkIdsForURITArray, I guess we should remove the third argument from it and any code that supports a false value.

::: toolkit/components/places/nsTaggingService.js:303
(Diff revision 1)
>  
> -    var tags = [];
> -    var bookmarkIds = PlacesUtils.bookmarks.getBookmarkIdsForURI(aURI);
> -    for (var i = 0; i < bookmarkIds.length; i++) {
> -      var folderId = PlacesUtils.bookmarks.getFolderIdForItem(bookmarkIds[i]);
> -      if (this._tagFolders[folderId])
> +    let tags = [];
> +    let db = PlacesUtils.history.DBConnection;
> +    let stmt = db.createStatement(
> +      // DOUBT: unsure why I'm getting guid etc along in the statement, we don't need them.
> +      `SELECT b.id AS bookmarkId, b.title, b.guid, b.parent, b.lastModified, t.guid, t.parent

yeah, just select what you need.

::: toolkit/components/places/nsTaggingService.js:315
(Diff revision 1)
> +    stmt.params.url = aURI.spec;
> +    stmt.params.tags_root = PlacesUtils.tagsFolderId;
> +    try {
> +      while (stmt.executeStep()) {
> +        try {
> +          let folderId = PlacesUtils.bookmarks.getFolderIdForItem(stmt.row.bookmarkId);

We don't need this call (that will run a further query) we are already running a query and we can make it return what we want.
You want to select all the folders that have tags_root as aparent and a child with the given uri.
I think in practice it means changing your query to SELECT t.id FROM...

Comment 7

[Milind L (:milindl)]

Comment on attachment 8872634 [details]
Bug 653816 - returning only nontags for GetBookmarkIdsForURI and fixing consumers,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/144172/diff/1-2/

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 8872634 [details]
Bug 653816 - returning only nontags for GetBookmarkIdsForURI and fixing consumers,

https://reviewboard.mozilla.org/r/144172/#review148824

thank you, LGTM! I'm starting a Try run.

I wonder if you may want to ask level 1 commit access for pushing to Try by yourself (see https://www.mozilla.org/en-US/about/governance/policies/commit/access-policy/), I would vouch for you.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/291fe1a6a375
returning only nontags for GetBookmarkIdsForURI and fixing consumers, r=mak

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/291fe1a6a375