Closed Bug 1251269 Opened 8 years ago Closed 8 years ago

Implement browser.bookmarks.getRecent()

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set
normal

Tracking

(firefox47 fixed)

RESOLVED FIXED
mozilla47
Iteration:
47.3 - Mar 7
Tracking Status
firefox47 --- fixed

People

(Reporter: bsilverberg, Assigned: bsilverberg)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete, Whiteboard: [bookmarks]triaged)

Attachments

(1 file)

Implement browser.bookmarks.getRecent(), which is documented at https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/bookmarks/getRecent

Question: Should this just return the bookmarks that are stored by Firefox in the Recently Bookmarked folder?
Assignee: nobody → bob.silverberg
Blocks: 1213674
Status: NEW → ASSIGNED
No longer depends on: 1213674, 1221764, 1208907, 1213675, 1225743
Flags: needinfo?(kmaglione+bmo)
Flags: blocking-webextensions+
That sounds right
Flags: needinfo?(kmaglione+bmo)
What we want to do is return the most recently added bookmarks. That is the logic used by Firefox itself. It seems like it’s as simple as querying for bookmarks and returning them in reverse chronological order, by date added, and limiting the number of results to the `numberOfItems` argument. In looking for code to do this, I found the following:

 `browser-places.js` contains a _updateRecentBookmarks function [1] that contains the logic used for displaying in the browser. It uses `PlacesUtils.history`, and the `asyncExecuteLegacyQueries` method on `QueryInterface(Ci.nsPIPlacesDatabase)`, which is nice because it offers an option for `maxResults` as well as an option for `sortingMode` which yields the exact result set we want. I was able to get similar code working, but it returns a `mozIStorageResultSet` which is not the easiest thing to work with. You need to convert each result into a format that the API expects to return. 

There is a function that sort-of does this conversion in `Bookmarks.jsm` called `rowsToItemsArray` [2], but it expects the StorageResultSet to be in a different format. `rowsToItemsArray` is designed for use with other methods in `Bookmarks.jsm`, but none of the methods in that file allow us to either sort the results or limit the number.

`Bookmarks.jsm` makes use of `PlacesUtils.withConnectionWrapper` [3] and `db.executeCached` to run its own customized queries. This works to work, but it is unfortunate that it doesn’t give us access to the simple querying as seen in [1].

It seems like the best approach would be to add code to `Bookmarks.jsm` to support this requirement, but I’m not sure if we should add another entire method with its own custom query, or whether we could enhance an existing method, e.g., `queryBookmarks` to allow us to specify a sort order, and also, perhaps, a maxResults.

Of course it's also possible there is another, better, solution that I haven't stumbled across yet.

What advice do you have, Kris?

[1] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-places.js#1367
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/Bookmarks.jsm#1228
[3] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/Bookmarks.jsm#905
[4] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/Bookmarks.jsm#883
Flags: needinfo?(kmaglione+bmo)
For the record, I cannot just get the tree from the Recently Added folder as it returns as empty. I think it only appears to have items when the dynamic code referenced as [1] above is run.
Updating queryBookmarks to accept a limit and a sort order sounds like a good solution to me.
Flags: needinfo?(kmaglione+bmo) → needinfo?(mak77)
(In reply to Bob Silverberg [:bsilverberg] from comment #2)
> It seems like the best approach would be to add code to `Bookmarks.jsm` to
> support this requirement, but I’m not sure if we should add another entire
> method with its own custom query, or whether we could enhance an existing
> method, e.g., `queryBookmarks` to allow us to specify a sort order, and
> also, perhaps, a maxResults.

Note, I'd not want queryBookmarks to become a new complete querying API, since we already have one that is something planned to be replaced (from a long time honestly). That method has been added as a temporary gap filler for the chrome.bookmarks.search api, but it's likely not what we want long term.

Why is it so hard to work with asyncExecuteLegacyQueries? is it missing some data, or is the problem just having to do a loop to convert the data?
I'd like to understand the development price in both cases, if it's close, I'd prefer to avoid increasing the reach of the new API, since it's just a temporary gap filler. if it's much cheaper to extend queryBookmarks, then I'm open to evaluate that, but it should not become a new complete querying API add-ons may rely on, since that would make it hard to remove in future.
Flags: needinfo?(mak77)
I think the main problem is that all of our current code is designed to work with Bookmarks.jsm conventions, so we don't have any utilities to convert raw result rows to Chrome-style Bookmark objects.

We could definitely add a helper to do that, but then we have two separate code paths for the same logic, which I'd love to avoid if possible. But I'm OK with it if bsilverberg is.

I can see why you wouldn't want to extend the queryBookmarks method. As far as WebExtensions go, I think this is the last part of the bookmarks API that would require any changes, so we're definitely not looking to make it into a generic query API.

Even so, I can think of a couple of alternatives:

1) Since this query is used in a couple of places internally, it might be worth just adding a special case for it. We already have special case code in nsNavHistory.cpp to optimize these kinds of queries, so we're probably going to wind up dealing with it either way.

2) We could add an `executeLegacyQueries` method to Bookmarks.jsm that behaves like `asyncExecuteLegacyQueries`, but handles results using Bookmarks.jsm conventions. We'd still be adding a new interface that we intend to remove, but any code that we migrate to it from the old interface would be halfway between the old legacy interface that we intend to remove, and the new one we intend to implement, so there shouldn't be any effort lost.

I think I'd prefer option #2, since it seems like it should result in the least work in the long run.
I think I like Kris' suggestion #2 as well, and he is correct that the issue with asyncExecuteLegacyQueries is not so much that I cannot work with the result, rather that we already have code (in Bookmarks.jsm) that converts bookmark data to the desired format, and I don't want to duplicate that code. I will look into doing it the way Kris suggests in #2 above.
(In reply to Kris Maglione [:kmag] from comment #6)
> 2) We could add an `executeLegacyQueries` method to Bookmarks.jsm that
> behaves like `asyncExecuteLegacyQueries`, but handles results using
> Bookmarks.jsm conventions.

I'm sold on a convertLegacyQueriesResults helper.
I was trying to figure out what the resultset looks like that I'm getting back from asyncExecuteLegacyQueries in order to write a method that converts it. I believe the query that ultimately gets run is defined at [1]. That does appear to contain all of the data we need (based on rowsToItemsArray [2]), except for parentGuid [3]. I believe this is a blocker to using asyncExecuteLegacyQueries as we do need that parentGuid. 

mak / kmag: what do you suggest?

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsNavHistory.cpp#1500
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/Bookmarks.jsm#1228
[3] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/places/Bookmarks.jsm#1237
FTR, I think I can solve this issue very easily with a couple of small changes to Bookmarks.jsm to allow us to pass a sort order to queryBookmarks. Maybe that is acceptable? I'm fine with trying to write a converter for queries returned by asyncExecuteLegacyQueries but I am concerned that the result set doesn't contain all of the data we need.
As the fix is quite simple, I decided to just implement it and submit it for review. This way you can decide if the change to Bookmarks.jsm is acceptable. If my submission is not deemed acceptable and you'd like me to pursue other solutions I am perfectly happy to do so.
(In reply to Bob Silverberg [:bsilverberg] from comment #9)
> I was trying to figure out what the resultset looks like that I'm getting
> back from asyncExecuteLegacyQueries in order to write a method that converts
> it. I believe the query that ultimately gets run is defined at [1]. That
> does appear to contain all of the data we need (based on rowsToItemsArray
> [2]), except for parentGuid [3]. I believe this is a blocker to using
> asyncExecuteLegacyQueries as we do need that parentGuid. 

Hm. The two options are to either add a parentGuid to bookmark result nodes, or to do a second query after we have results, to get the GUIDs of the parent IDs.

I don't know what the performance cost of the former would be to existing queries, so I'll defer to Marco.

(In reply to Bob Silverberg [:bsilverberg] from comment #12)
> As the fix is quite simple, I decided to just implement it and submit it for
> review. This way you can decide if the change to Bookmarks.jsm is
> acceptable. If my submission is not deemed acceptable and you'd like me to
> pursue other solutions I am perfectly happy to do so.

I'm definitely not OK with passing raw SQL to the query function. If we go this route, we should pass a property name from the result object, and let the query function deal with the SQL.
(In reply to Kris Maglione [:kmag] from comment #13)
> 
> I'm definitely not OK with passing raw SQL to the query function. If we go
> this route, we should pass a property name from the result object, and let
> the query function deal with the SQL.

Ok, yeah, I can see that that's not ideal. I'm not totally sure about what you are suggesting though. I'm wondering if it would be better to just add a full method to Bookmarks.jsm to return the recent bookmarks, rather than trying to extend an existing method? If the API isn't ever going to need any other sorting, then adding that to queryBookmarks isn't really buying us much anyway. I'm going to move forward with that for now, but feel free to shoot it down and/or suggest something else.
Comment on attachment 8724842 [details]
MozReview Request: Bug 1251269 - Implement browser.bookmarks.getRecent(), r=kmag r=mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37183/diff/1-2/
I've updated the review with a different approach.
you really want a limit argument, or you're returning ALL bookmarks. Also the original API has a numberOfItems argument, for good reasons.
Also, return Task.spawn == Task.async.
Comment on attachment 8724842 [details]
MozReview Request: Bug 1251269 - Implement browser.bookmarks.getRecent(), r=kmag r=mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37183/diff/2-3/
Thanks mak. I added a numberOfItems argument to getRecent in Bookmarks.jsm. I am confused by what you mean by "return Task.spawn == Task.async". I tried replacing the Task.spawn with Task.async, but that resulted in errors like:

116 INFO TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html | Error: Error: An unexpected error occurred :: Async*@moz-extension://cbe9ebe4-8277-0744-ad7d-8703ca23ba09/%7B38f4db7d-ffa5-6d42-a369-a264029c9186%7D.js:315:12
promise callback*backgroundScript@moz-extension://cbe9ebe4-8277-0744-ad7d-8703ca23ba09/%7B38f4db7d-ffa5-6d42-a369-a264029c9186%7D.js:31:3
@moz-extension://cbe9ebe4-8277-0744-ad7d-8703ca23ba09/%7B38f4db7d-ffa5-6d42-a369-a264029c9186%7D.js:1:11

    testHandler@SimpleTest/ExtensionTestUtils.js:50:7
    testResult@SimpleTest/ExtensionTestUtils.js:60:36

All of the similar code in Bookmarks.jsm use Task.spawn, so I'm not sure a) why this should be different and b) how to change it to use async instead of spawn.
it's just a shorthand, instead of
method: function() {
  return Task.spawn(function* (...
you can have:
method: Task.async(function* (
Comment on attachment 8724842 [details]
MozReview Request: Bug 1251269 - Implement browser.bookmarks.getRecent(), r=kmag r=mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37183/diff/3-4/
Thanks mak. I have updated the patch accordingly.
Comment on attachment 8724842 [details]
MozReview Request: Bug 1251269 - Implement browser.bookmarks.getRecent(), r=kmag r=mak

https://reviewboard.mozilla.org/r/37183/#review34013

The WebExtension parts look good to me, but I'm not the best person to review Places code.

::: toolkit/components/extensions/test/mochitest/test_ext_bookmarks.html:310
(Diff revision 4)
> -    browser.test.notifyPass("bookmarks");
> +    Promise.resolve().then(() => {

We need to return both of these do make sure the handlers are actually called before we get to notifyPass.

::: toolkit/components/places/Bookmarks.jsm:1037
(Diff revision 4)
> +       ORDER BY b.dateAdded DESC, b.position

Shouldn't `b.position` have `DESC` too?

::: toolkit/components/places/Bookmarks.jsm:1038
(Diff revision 4)
> +       LIMIT ${numberOfItems}

This should use a parameter placeholder. I know that you already checked for a safe value, but if you don't use a placeholder, you'll get a new cached query for each limit value that's passed in.
Attachment #8724842 - Flags: review?(kmaglione+bmo) → review+
Attachment #8724842 - Flags: review?(mak77)
Comment on attachment 8724842 [details]
MozReview Request: Bug 1251269 - Implement browser.bookmarks.getRecent(), r=kmag r=mak

https://reviewboard.mozilla.org/r/37183/#review34099

please also add a test_getRecent to http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/tests/bookmarks/ like for all the other bookmark APIs.

::: toolkit/components/places/Bookmarks.jsm:512
(Diff revision 4)
> +   *

nit: remove empty line

::: toolkit/components/places/Bookmarks.jsm:518
(Diff revision 4)
> +    if (!typeof numberOfItems==='number' || (numberOfItems%1)!==0) {

please also check numberOfItems > 0
I'd consider 0 a coding error, since there's no use-case for it.

::: toolkit/components/places/Bookmarks.jsm:1032
(Diff revision 4)
> +              p.parent AS _grandParentId

please NULL AS _parentId, NULL AS _grandParentId, NULL AS _childCount, NULL AS _id
we don't need those here

::: toolkit/components/places/Bookmarks.jsm:1037
(Diff revision 4)
> +       ORDER BY b.dateAdded DESC, b.position

why position? position has nothing to do with recency, if you want to provide a secondary sorting I'd rather use ROWID DESC

::: toolkit/components/places/Bookmarks.jsm:1041
(Diff revision 4)
> +    return rows.length ? rowsToItemsArray(rows) : null;

what does the spec say in case there are no bookmarks, null or empty array?
Attachment #8724842 - Flags: review?(mak77)
Attachment #8724842 - Flags: review?(mak77)
Oops, sorry, I got the two bugs that kmag and mak are reviewing mixed up. Let me figure out what I'm doing, and I apologize if I just created extra noise.
Comment on attachment 8724842 [details]
MozReview Request: Bug 1251269 - Implement browser.bookmarks.getRecent(), r=kmag r=mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37183/diff/4-5/
Attachment #8724842 - Flags: review?(mak77)
Attachment #8724842 - Flags: review?(mak77)
https://reviewboard.mozilla.org/r/37183/#review34013

> We need to return both of these do make sure the handlers are actually called before we get to notifyPass.

I think I fixed this. I ended up with similar confusion to that I had on the other review where I was missing `return`s, and I addressed this with extra `then`s. Let me know if this is good or if I should be doing something else.

> Shouldn't `b.position` have `DESC` too?

In my testing it looked like I wanted `b.position` to be sorted ascending. That was how to guarantee that I got the bookmarks back in the reverse order to how they were created in the test. mak made a different suggestion on sorting, so I'm going to try his.
https://reviewboard.mozilla.org/r/37183/#review34099

Done!

> why position? position has nothing to do with recency, if you want to provide a secondary sorting I'd rather use ROWID DESC

As mentioned above, in my test I found that using `position` would return the bookmarks to me in the reverse order to their creation. I needed this for the test because multiple bookmarks could share a `dateAdded`. I will change it to use `ROWID DESC` instead.

> what does the spec say in case there are no bookmarks, null or empty array?

I believe the API expects an empty array, so I guess I might as well just return one from here. I'll make that change.
Comment on attachment 8724842 [details]
MozReview Request: Bug 1251269 - Implement browser.bookmarks.getRecent(), r=kmag r=mak

https://reviewboard.mozilla.org/r/37183/#review34361

r=me on the Places part.

::: toolkit/components/places/Bookmarks.jsm:1041
(Diff revision 5)
> +      `, { tags_folder: PlacesUtils.tagsFolderId, number_of_items: numberOfItems });

nit: if you name it :numberOfItems in the query, here you can just ", numberOfItems }" using ES6 shorthand.

::: toolkit/components/places/tests/bookmarks/xpcshell.ini:49
(Diff revision 5)
> +[test_bookmarks_getRecent.js]

please retain alphabetical order in .ini files.
Attachment #8724842 - Flags: review?(mak77) → review+
Attachment #8724842 - Attachment description: MozReview Request: Bug 1251269 - Implement browser.bookmarks.getRecent(), r?kmag → MozReview Request: Bug 1251269 - Implement browser.bookmarks.getRecent(), r=kmag r=mak
Comment on attachment 8724842 [details]
MozReview Request: Bug 1251269 - Implement browser.bookmarks.getRecent(), r=kmag r=mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37183/diff/5-6/
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1cfb0f3e893e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.