Port prototype jetpack.places to reboot jetpack

Also need a couple of new methods: bookmark.remove?, bookmark.save?

We will be monitoring all these issues after the rebooted Jetpack code base is released in the first week of March to ensure their causes are not duplicated. Many of the bugs/issues with the prototype version of Jetpack will be made irrelevant given the structure of the new SDK.

I'm iterating on this based on work in bug 522572. The basic idea has not changes much from what i originally discussed with David. Note this is an ambitious plan. First, don't forget to import the module: var places = require("places"); Then filter on history or bookmarks places.history.filter({ phrease: "search terms" }, myCallback, callbackScope); places.bookmarks.filter({ phrease: "search terms" }, myCallback, callbackScope); It is possible to just query pages regardless of their status places.filter({ phrease: "search terms" }, myCallback, callbackScope); The filter uses a PlacesQuery object (see Sketches section in https://wiki.mozilla.org/Firefox/Projects/PlacesQueryAPIRedesign) , thus all its options are available. Results are pushed to the callback as arrays of ResultItem objects, an empty array means the end of the results. Once defined a search is possible to act on results to physically remove elements from the database. places.bookmarks.filter({ phrase: "remove me please" }) .remove(myCallback, callbackScope); places.history.filter({ host: "remove.host.com" }) .remove(myCallback, callbackScope); Full clear history should be doable as: places.history.remove(myCallback, callbackScope); It is possible to annotate pages, if the query is limited to bookmarks this will set a bookmark annotation, otherwise will set a page annotation. places.bookmarks.filter({ uri: "^http://mozilla.org/annotateme/$" }) .annotate([{ name: "annoToAdd", value: "annoValue"], myCallback, callbackScope); To remove an annotation don't specify a value places.bookmarks.filter({ uri: "^http://mozilla.org/annotateme/$" }) .annotate([{ name: "annoToRemove" }], myCallback, callbackScope); To add a bookmark places.bookmarks.save({ uri: "http://moz.org/" , title: "a bookmark" , parent: 3 , position: 12 }, myCallback, callbackScope); To edit the last added bookmark for "http://moz.org" places.bookmarks.filter({ uri: "http://moz.org/" , sort: { by: "time", dir: "desc" } , limit: 1 }).save({ title: "a new title" , tags: ["foo", "bar"] }, myCallback, callbackScope); To remove tags just put a - in front of tag name, like tags: ["-foo", "bar"] will remove tag foo and add tag bar. TBD: (possibly follow-ups) - adding folders, separators - hiearchies - icons - livemarks - import-export - live updating results (won't come out of .filter(), that is supposed as a static results getter, will most likely be a new wrapper returning a resultSet with more methods, like .result()) - observers: ideally people could get a live result and be notified by it as a first layer. Raw observer?

Started updating the JEP page: https://wiki.mozilla.org/Labs/Jetpack/Reboot/JEP/114

posted on the newsgroup, message waiting moderators.

this is the last version I had

* got single-config working in the module. * updated module to the latest PlacesQuery changes. * various cleanup in the module. * included the latest PlacesQuery into the Jetpack places.js module, so no Fx build dependency. not sure that's how we want to keep it forever, but fine for the initial approach. TODO: * more tests * more docs (all query config options, all editing param options) Myk: Requesting feedback on the general approach of the API (filtering, chaining), so just need you to look at the docs for now.

> Myk: Requesting feedback on the general approach of the API (filtering, > chaining), so just need you to look at the docs for now. Sure, I can do that. In addition to these docs, I consulted <https://wiki.mozilla.org/Firefox/Projects/PlacesQueryAPIRedesign>, which seems to describe the API on which this is based. Here are a bunch of notes. 1. Overall, the semantics of "places", "places.bookmarks", and "places.history" as accessors to various kinds of places; the "filter" method for quick and easy searches; and the chaining of changes and removal to those searches all seem like a great design for this API (although I'm somewhat concerned about the ease with which chaining enables consumers to change/remove bookmarks en masse). 2. The use of the term "filter" for searching for places, bookmarks, and history entries is an intriguing abstraction, given its implication that the places, places.bookmarks, and places.history objects themselves comprise the complete sets of places, bookmarks, and history entries, and the same kinds of operations can be performed on them as are performed on their filtered subsets. However, presumably they aren't actually intended to imply that kind of access, given the complexities inherent in implementing such an API and the dangers manifest in making it available to consumers. So it would be better to call the method "search", which doesn't carry the same implication that it is reducing an existing set. 3. The docs sometimes use the term "pages" to describe the things that the Places API exposes, while at other times they use "places" (including in the name of the module itself). We should use one of these terms consistently. And the term "page" isn't an entirely accurate description of the things being exposed, since some of them reference fragments of pages (http://example.com/#foo), while others are better described as applications (which might eventually get formalized as the set of places the user identifies as apps and opens in app tabs or prism-like windows). The term "pages" is also overloaded and might lead to the API being misconstrued as one for accessing the pages currently loaded in the browser (i.e. a superset of the Tabs API). The term "places" isn't completely problem-free, either, but it's a better option than "pages" and the one with which we should stick. (It also happens to fit better with an emerging conceptual model of the browser as a user agent brokering access to "people, places, and things.") 4. We haven't yet implemented an API that solicits an object to use as the "this" object (like the callbackScope parameter) when executing a callback, and I'm reluctant to start now, because it complicates the API, is unnecessary in many cases, is easily (albeit clunkily) worked around when needed, may have security implications (not sure, and don't want to sow FUD, but would want to noodle on this a bit), and will become even less useful over time as developers migrate to ES5 (given its "bind" functionality). However, given that the first parameter to the filter method is an object specifying options that configure the search, and the search creates a query object, it isn't hard to make this into a Jetpack-like API. The callbacks should just be defined as conventional Jetpack on* callback properties in the options object, with the "this" object for a given callback being the query object, and the results of the query being available through iteration of the object's "results" property, i.e.: places.search({ /* search criteria */ onComplete: function() { for each (let place in this.results) { ... } } }); 5. The results of searches should be Bookmark and History objects rather than ResultItem objects. And their properties referencing related things (like a session, parent, or visit) should be (lazily loaded, as appropriate) getters of objects representing those things rather than merely the IDs of the records representing those things in the database. And acronyms in property names (like ID and URL) should be all upper case (unless the first word of the name, in which case they should be all lower case). And positional properties should be named "position" rather than "index". And it should be possible to change an individual bookmark through direct manipulation of its properties, to wit: places.bookmarks.search({ /* search criteria */ onComplete: function() { for each (let bookmark in this.results) { if (/* this is the droid I'm looking for */) { bookmark.title = "C3PO"; bookmark.position = "12"; bookmark.annotations.set("owner", "Amidala"); bookmark.annotations.remove("annoyingness"); // If we can countenance multiple database updates, setting a // property should immediately update the database to reflect its new // value; otherwise, API consumers should call a save() method // to update the database with the changes to the bookmark. bookmark.save(); } } } }); Note also the "live object" model Andrew Sutherland described in an earlier discussion of this API <http://groups.google.com/group/mozilla-labs-jetpack/browse_thread/thread/b5f177128c75f35#cffbdf4eadb3c8b3>. That seems like the right long-term goal, but Andrew's last words in that discussion <http://groups.google.com/group/mozilla-labs-jetpack/browse_thread/thread/b5f177128c75f35#f61f1e013b3dff84> are prescient, and we should satisfy ourselves with our existing API model for the initial implementation and then investigate implementation of a "live object" API for a future version. 6. Progressive Results > Results are pushed to the callback as arrays of ResultItem objects, > an empty array means the end of the results. I'm generally averse to magical semantics like "an empty array means the end of the results". And in the earlier discussion <http://groups.google.com/group/mozilla-labs-jetpack/browse_thread/thread/b5f177128c75f35>, ddahl and I agreed on the utility of supporting both a simple interface for accessing all results and a more complex interface for accessing progressive ones. The best way to do that here would be to provide two on* callback properties, one that gets called each time there are additional results and one that gets called only once all results have been returned, so the simple case looks like this: places.search({ /* search criteria */ onComplete: function() { // All results are now available in this.results. } }); while the complex case looks like this: places.search({ /* search criteria */ onProgress: function(results) { // Additional results are available in the "results" argument. // this.results contains the accumulated results. } onComplete: function() { // All results are now available in this.results. } }); 7. The way save() is overloaded to provide for both the creation and changing of bookmarks is confusing and error-prone. The two tasks should be implemented by different methods, create() and change() (not "save", whose semantics aren't quite right for this use case), where create() is called on places.bookmarks to create a bookmark: places.bookmarks.create({ /* attributes */ onCreate: function() { // The "this" object is a Bookmark object representing the bookmark. } }); and change() is called on a query object: places.bookmarks.search({ /* search criteria */ }).change({ /* changes */ }); 8. The chaining of searches with changes and removals is compatible with Jetpack-like on* callback properties, except that the properties should be defined on the query object: places.bookmarks.search({ /* search criteria */ onChange: function() { // The "this" object is the query object, and the search results // now reflect their new values. } }).change({ /* changes */ }); places.bookmarks.search({ /* search criteria */ onRemove: function() { // The "this" object is the query object, and the search results // now reflect their removal. But it is possible to recreate a bookmark // that was removed by passing it to places.bookmarks.create(). } }).remove(); 9. The changing of annotations should be rolled into change(), which should accept a collection of annotations to set and an array of the names of annotations to remove: places.bookmarks.search({ /* search criteria */ }).change({ annotationsToSet: { foo: "tball", bar: "tholomew" }, annotationsToRemove: [ "baz" ] } });

(In reply to comment #8) Thanks! Most of the comments make sense, and I'm implementing the changes. A few clarifications requested below. > 2. The use of the term "filter" for searching for places, bookmarks, and > history entries is an intriguing abstraction, given its implication that the > places, places.bookmarks, and places.history objects themselves comprise the > complete sets of places, bookmarks, and history entries, and the same kinds of > operations can be performed on them as are performed on their filtered subsets. > > However, presumably they aren't actually intended to imply that kind of access, > given the complexities inherent in implementing such an API and the dangers > manifest in making it available to consumers. So it would be better to call > the method "search", which doesn't carry the same implication that it is > reducing an existing set. I'm ok with s/filter/search/. However, I think I don't understand this comment. Yes, you can access all the bookmarks and history with this API. And like the XPCOM APIs, and the Firefox UI, you can perform any kind of operation on those sets. > 4. We haven't yet implemented an API that solicits an object to use as the > "this" object (like the callbackScope parameter) when executing a callback, and > I'm reluctant to start now, because it complicates the API, is unnecessary in > many cases, is easily (albeit clunkily) worked around when needed, may have > security implications (not sure, and don't want to sow FUD, but would want to > noodle on this a bit), and will become even less useful over time as developers > migrate to ES5 (given its "bind" functionality). Bah, I meant to remove the scope parameter from the Jetpack wrapper. Yep, I'll move the result callback to on* and add it to the options object. However, I'm not so keen on moving this completely from an API which allows consumers to receive results immediately to one forces callers to wait until the search is complete. How about we offer both onResult(result) like there is now, and with onComplete(results) for the entire result set? > 5. The results of searches should be Bookmark and History objects rather than > ResultItem objects. The containers of a set of results should be those objects? Can you clarify? > And it should be possible to change an individual bookmark through direct > manipulation of its properties, to wit: Agreed. Though, I'll do it in a followup, doesn't need to block the first cut. > 6. Progressive Results > > > Results are pushed to the callback as arrays of ResultItem objects, > > an empty array means the end of the results. Hrghg. I forgot to update that part of the docs. There are two significant changes in the actual implementation of both the underlying PlacesQuery implementation and the Jetpack wrapper: 1. Each result (row) is passed back individually. There was no benefit to passing randomly-sized groups of results back. 2. The result parameter to onResult is false when there are no more results. Moving to onComplete resolves the magical property problem. See above about moving to onResult(result) where result is never more than a single result. > The best way to do that here would be to provide two on* callback properties, > one that gets called each time there are additional results and one that gets > called only once all results have been returned, so the simple case looks like > this: Oh, I should've read the whole review through before responding :) So, we're in agreement about onResult/onComplete. But instead of using |this|, how do you feel about doing onResult(result) and onComplete(results)?

(In reply to comment #9) > (In reply to comment #8) > > 5. The results of searches should be Bookmark and History objects rather than > > ResultItem objects. > > The containers of a set of results should be those objects? Can you clarify? Ok, i see what you mean. Rather than two separate types, can we just have one (like the patch has now), typed as a Place object? This easily allows access to all potential properties of a given place, regardless of whether you originally queried .history or .bookmarks, as well as removing the need for callers to always check what type each item is. If a caller wants to know if a place is bookmarked, they can check it's isBookmarked property.

Changes in v2 so far: * s/filter/search/ * converted all code to use "options" like other modules * removed scope args, instead using the options argument as the scope in relevant cases * merged callbacks in to onResult/onComplete (need feedback vs onProgress) * onCreate for bookmark creation * onRemove for bookmark removal * made a Place object for result item (need feedback vs multiple types) * renamed/omitted result object properties where needed * more tests TODO: * resolve onResult vs onProgress * resolve result item type vs types * onChange for bookmark updates and annos * docs update * more tests

(In reply to comment #9) > I'm ok with s/filter/search/. However, I think I don't understand this comment. > Yes, you can access all the bookmarks and history with this API. And like the > XPCOM APIs, and the Firefox UI, you can perform any kind of operation on those > sets. The term "filter" is sometimes used to refer to a secondary in-memory querying of the results of an original search that has retrieved a set of results from persistent storage in order to obtain a subset of that search's results. So when I saw the term being used in this API, my first thought was that places.bookmarks and places.history are presenting themselves as complete sets of bookmarks and history that I can treat as such, enabling me to do: // Iterate over entire history. for (let history in places.history) { // Do something. } or even: // Make a change to all bookmarks. places.bookmarks.change({ /* changes */}); But that isn't actually the case. And making it the case (which is an intriguing API) is quite difficult and potentially dangerous, given the power it gives consumers to make changes to (or remove) all bookmarks at once. > > And it should be possible to change an individual bookmark through direct > > manipulation of its properties, to wit: > > Agreed. Though, I'll do it in a followup, doesn't need to block the first cut. Yup, agreed. The only pieces that we should make sure to do in the initial implementation are to name the properties per guidelines and deal with the properties that expose raw information about the database schema (like ID fields), which could be as simple as stripping through from this initial implementation. > So, we're in agreement about onResult/onComplete. Yup, and onResult with a single result (instead of onProgress with a set of results) is great. > But instead of using |this|, how do you feel about doing onResult(result) and > onComplete(results)? I prefer this.results, because it automatically persists the result set in the query object (so consumers can access it after onComplete returns), allows us to make partial results available as this.results in onResult, and is consistent with how the Request API makes request responses available in its onComplete handler. (In reply to comment #10) > Ok, i see what you mean. Rather than two separate types, can we just have one > (like the patch has now), typed as a Place object? This easily allows access to > all potential properties of a given place, regardless of whether you originally > queried .history or .bookmarks, as well as removing the need for callers to > always check what type each item is. If a caller wants to know if a place is > bookmarked, they can check it's isBookmarked property. Yes, that's fine.

* s/aResults/this.results/ * implemented .change()/onChange() A couple of issues wrt to exposing database internal ids: * I need to get access to database row ids for internal use. I'm currently exposing a bookmark item's row id via _itemId (undocumented), which is not desirable. * I'm looking at exposing .folder as a Place object. Opening a folder is fine, could hang .search() off of it, making querying of folders semantically identical to querying all bookmarks, history, all. However, specifying the folder to use when creating a bookmark, or as a parameter for searching, requires the internal code to access the row id (same problem as above). I'm currently moving ahead with the faux-private property approach, in order to get everything actually working, so we can see what things look like. An alternate approach could be to use the item's GUIDs. However, while that fixes the implementation exposure problem, it doesn't make things any nicer for the caller. Maybe there's a way we can have properties that are only exposed depending on the calling context, or something magical like that?

* read/write tests for bookmarks and folders (and all the bugs fixed along the way) TODO: * figure out what to do with _itemId * figure out when non-bookmark items should be shown in results. currently they're not shown in basic searches, and are only shown when a folder or folders are in the query options. which is not very intuitive. there's an "onlyContainers" option now. maybe i will change it to showContainers, defaulting to false. EIBTI.

If we don't have an alternative, exposing _itemId is OK, since the risk is minimal, although we should open a bug on the problem and fix it once we figure out how to do so. Also cc:ing Irakli for his thoughts on the problem, as he has done some thinking (and hacking, see bug 584707) on alternative ways to hide private properties.

Thing you can do to share access to a private property only with restricted group is: // define local function returning property function $(object) object._id // define privates property on your object as non-enumerable getter which returns value to a function defined above. Object.defineProperty(object, '_id', { get: function get() get.caller === $ ? id : undefined ,enumerable: false}); // Then pass your $ function to anyone you want to grant access to your private // so that they can access the data function consumer(result) { let id = $(result); } I'm not entirely sure that's a best feet here, but I'll take a look at your code too see if there are any better options. You can also look at some work I've done on bug 584707, but there I've implemented a way to define privates that can be inherited by derived compositions, which does not covers the use case of sharing privates with third parties. Non related opinions on the API: 1. +1 on onResult(result) / onComplete(results) vs onResult() / onComplete() since I coughs myself several times already trying to workaround bounded this with other APIs. It's also non DOMmy way and may cause confusions when callback is onResult: function() { [/* code */}.bind(whatever). As a compromise maybe we can pass result as argumnet and apply function call to `this` ? 2. I think it will be very convenient to make results being EventEmitter's so this will be correct as well: let bookmarks = places.bookmarks.search({ phrease: "search terms" }); bookmarks.addEventListener('result', myBookmarkManager.onBookmark); I have done some work on event emitter as well and it can be found here: Bug 588732

As a heads up, I am interested in using my narscribblus documentation/interactive examples tool with the Places jetpack capability. I think the complexity of both the Places API and its resulting data structures are well aligned with my tool which is designed to work with the (also complex) Thunderbird Gloda(NG) API. Specifically, I think it would be beneficial to be able to have documentation on the API that shows examples from the user's own Places database. The brief intro blog post to narscribblus can be found at: http://www.visophyte.org/blog/?p=562 I am going to pursue this over the course of the next few days. The expected deliverable would be a jetpack-built XPI that adds chrome URIs providing at least a page of tutorial style documentation and a page of more reference oriented documentation. Since I don't want to hijack the bug, please provide any feedback via direct e-mail or on the jetpack mailing list. When I have results I'll mail the jetpack mailing list and likely make a blog post.

minor test changes, converting docs over to contemporary formatting and filling them out further.

* add full-circle test for separators * add docs for the search options object

* more tests * documented the search result object

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product. To filter bugmail related to this change, filter on the word "looptid".

I'm not working on this at the moment, so unassigning. I'm also not convinced it ever has to land in the SDK, since modules are available to anyone, anywhere, anytime at https://wiki.mozilla.org/Labs/Jetpack/Modules. I'll put it up there.

Triaging with dcm, myk, mossop.

(Pushing all open bugs to the --- milestone for the new triage system)

So, is there going to be a Places module in the SDK? If not, how can I use Jetpack modules in addon builder? Thanks.

A Places Module is one of Jetpacks' Q2 goals, with a blog post coming soon for more details about it, but it will be in the SDK in the near future :)

Nice! Do you know if there is going to be support for managing livemarks? Thanks for this great job!

Pointer to Github pull-request

Commits pushed to master at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/a1ffa060007bd2fde311c6d8a491602cad8f2d28 Bug 545700 Places API https://github.com/mozilla/addon-sdk/commit/dbcc2a239f3c7654d21ecfddd202d0d30cb49f29 Merge pull request #1046 from jsantell/places-api Fix Bug 545700 - Places API, r+=@gozala

Looks like all tests are green. All of that purple is from canceled or broken builds on m-c earlier today. We can retrigger those test runs once we're sure those platforms have built successfully overnight tonight if we want the extra test coverage from the opt builds.

Sounds good!

Commits pushed to firefox25 at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/a1ffa060007bd2fde311c6d8a491602cad8f2d28 Bug 545700 Places API https://github.com/mozilla/addon-sdk/commit/dbcc2a239f3c7654d21ecfddd202d0d30cb49f29 Merge pull request #1046 from jsantell/places-api