Closed
Bug 1251269
Opened 8 years ago
Closed 8 years ago
Implement browser.bookmarks.getRecent()
Categories
(WebExtensions :: Untriaged, defect)
WebExtensions
Untriaged
Tracking
(firefox47 fixed)
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 | ||
Updated•8 years ago
|
Updated•8 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 2•8 years ago
|
||
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)
Assignee | ||
Comment 3•8 years ago
|
||
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.
Comment 4•8 years ago
|
||
Updating queryBookmarks to accept a limit and a sort order sounds like a good solution to me.
Flags: needinfo?(kmaglione+bmo) → needinfo?(mak77)
Comment 5•8 years ago
|
||
(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)
Comment 6•8 years ago
|
||
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.
Assignee | ||
Comment 7•8 years ago
|
||
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.
Comment 8•8 years ago
|
||
(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.
Assignee | ||
Comment 9•8 years ago
|
||
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
Assignee | ||
Comment 10•8 years ago
|
||
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.
Assignee | ||
Comment 11•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/37183/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37183/
Attachment #8724842 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 12•8 years ago
|
||
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.
Comment 13•8 years ago
|
||
(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.
Assignee | ||
Comment 14•8 years ago
|
||
(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.
Assignee | ||
Comment 15•8 years ago
|
||
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/
Assignee | ||
Comment 16•8 years ago
|
||
I've updated the review with a different approach.
Comment 17•8 years ago
|
||
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.
Assignee | ||
Comment 18•8 years ago
|
||
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/
Assignee | ||
Comment 19•8 years ago
|
||
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.
Comment 20•8 years ago
|
||
it's just a shorthand, instead of method: function() { return Task.spawn(function* (... you can have: method: Task.async(function* (
Assignee | ||
Comment 21•8 years ago
|
||
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/
Assignee | ||
Comment 22•8 years ago
|
||
Thanks mak. I have updated the patch accordingly.
Comment 23•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8724842 -
Flags: review?(mak77)
Comment 24•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8724842 -
Flags: review?(mak77)
Assignee | ||
Comment 25•8 years ago
|
||
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.
Assignee | ||
Comment 26•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8724842 -
Flags: review?(mak77)
Assignee | ||
Comment 27•8 years ago
|
||
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.
Assignee | ||
Comment 28•8 years ago
|
||
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 29•8 years ago
|
||
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+
Assignee | ||
Updated•8 years ago
|
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
Assignee | ||
Comment 30•8 years ago
|
||
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/
Assignee | ||
Comment 31•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=57a25f43d633
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 32•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cfb0f3e893e
Keywords: checkin-needed
Comment 33•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1cfb0f3e893e
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Comment 34•8 years ago
|
||
I've updated: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/bookmarks/getRecent https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Chrome_incompatibilities
Keywords: dev-doc-needed → dev-doc-complete
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•