Places API feedback

[David Dahl]

Hello.

I just revised the jetpack.places API JEP 22: https://wiki.mozilla.org/Labs/Jetpack/JEP/22

"Places" is the Mozilla internal name for bookmarks and history.

The bugzilla implementation bug is here:
https://wiki.mozilla.org/Firefox/Projects/PlacesQueryAPIRedesign

The Places bug is here: https://bugzilla.mozilla.org/show_bug.cgi?id=522572

Any feedback would be great. A few bullet points:

* The "read" api methods will be implemented first.
* You will need to provide a callback to do something with the results
fetched from a query
* We may be able to drop this into the jetpack.future sooner rather
than later, I just need to implement the 'write' APIs and implement a
wrapper for Jetpack.

I am currently wrapping the existing Places APIs (which are
synchronous). A full async implementtion is being written in parallel.
The Async implementation is going to be faster and leave the main
thread alone. Callbacks are required as all new APIs targeting future
Firefox releases require off main thread execution.

Cheers,

David

[David Dahl]

On Mon, Nov 30, 2009 at 3:02 PM, David Dahl <david...@gmail.com> wrote:
> Hello.

> The bugzilla implementation bug is here:
> https://wiki.mozilla.org/Firefox/Projects/PlacesQueryAPIRedesign

Sorry, wrong url, that is the Places Wiki page. The real bug url is here: https://bugzilla.mozilla.org/show_bug.cgi?id=531940


Cheers,

David

[Myk Melez]

On 11/30/2009 03:02 PM, David Dahl wrote:
> I am currently wrapping the existing Places APIs (which are
> synchronous). A full async implementtion is being written in parallel.
> The Async implementation is going to be faster and leave the main
> thread alone. Callbacks are required as all new APIs targeting future
> Firefox releases require off main thread execution.
>

Looking good! A few notes:

1. jetpack.places.bookmark() feels like something that would make more
sense hanging off the active tab accessor (i.e. something like
tabs.activeTab.bookmark()), since there's nothing about jetpack.places
that suggests that its methods would affect the active tab.

2. It's not clear from the JEP where the bookmarks object comes from.
Does it hang off jetpack.places?

3. It'd be useful for fetch to permit intersection (OR) searches in
addition to union (AND) searches. A simple way to enable that would be
to make the first parameter optionally accept an array of objects, so
you could find just Groucho's bookmarks via:

jetpack.places.fetch({phrase: "Groucho Marx", where: "bookmarks"},
callback);


While finding both Groucho's and Harpo's bookmarks via:

jetpack.places.fetch([{phrase: "Groucho Marx", where: "bookmarks"},
{phrase: "Harpo Marx", where: "bookmarks"}],
callback);


4. I really like the simplicity of specifying a callback that gets all
the results at the end. However, since the asynchronous storage API can
return results progressively, it's worth thinking a bit about ways we
might enable callbacks to take advantage of this. Perhaps a callback
that is an object rather than a function could be considered a
progressive listener and get regular updates to a long running search?

5. The examples say that bookmark.save() is "called to make the title
change stick". Does it really only affect the title, or does it affect
the other attributes as well?

6. bookmark.tags() feels like it wants to be a property (that might be
implemented internally as a getter) rather than a method. And I'd
probably want to be able to set tags via bookmark.tags = [...] in
addition to being able to add a tag via bookmark.tag().

7. I don't see any mention of accessing history items, but presumably
their properties are similar to bookmarks (except for the
bookmarks-specific stuff)? I also don't see any mention of annotations; v2?

8. If there's a jetpack.places.bookmarks, then it'd make sense to hang a
fetch method off it that works just like jetpack.places.fetch but
assumes where: "bookmarks".

9. It's not clear what the distinction is between where: "history" and
where: "everywhere". Aren't bookmarks always also in history? Or does
where: "history" only return history items that aren't also bookmarked?

10. There are both term and phrase search parameters. What's the
difference, and which one do you do when you pass fetch a simple string?

11. Finally, I'll bikeshed a bit and suggest that find seems like a bit
more intuitive name for fetch! ;-)

-myk

[David Dahl]

On Nov 30, 6:50 pm, Myk Melez <m...@mozilla.org> wrote:

> 1. jetpack.places.bookmark() feels like something that would make more
> sense hanging off the active tab accessor (i.e. something like
> tabs.activeTab.bookmark()), since there's nothing about jetpack.places
> that suggests that its methods would affect the active tab.
>

I agree, we should plan on implementing this in the active tab. (or
all tabs - just add a bookmark method to all of them)

> 2. It's not clear from the JEP where the bookmarks object comes from.
> Does it hang off jetpack.places?
>

No, I will have to re-work the examples - the bookmarks object is
inside the callback function - the bookmarks is the iterator that is
passed into the callback.

> 3. It'd be useful for fetch to permit intersection (OR) searches in
> addition to union (AND) searches. A simple way to enable that would be
> to make the first parameter optionally accept an array of objects

An array of 'search objects' would be a good way to do this. At this
point, using the existing API, this would be difficult and slow to
implement - and is beyond the current APIs capability - at least
without 2 distinct, synchronous searches.
I will plan to implement this in the fully async rewrite.

>
> 4. I really like the simplicity of specifying a callback that gets all
> the results at the end. However, since the asynchronous storage API can
> return results progressively, it's worth thinking a bit about ways we
> might enable callbacks to take advantage of this. Perhaps a callback
> that is an object rather than a function could be considered a
> progressive listener and get regular updates to a long running search?
>

excellent idea, perhaps we can optionally provide a listener 'base
class' that does just that? I think the async implementation (in
MozStorage) will call the callback for each chunk that is fetched.

> 5. The examples say that bookmark.save() is "called to make the title
> change stick". Does it really only affect the title, or does it affect
> the other attributes as well?

It will affect all attributes

>
> 6. bookmark.tags() feels like it wants to be a property (that might be
> implemented internally as a getter) rather than a method. And I'd
> probably want to be able to set tags via bookmark.tags = [...] in
> addition to being able to add a tag via bookmark.tag().
>

I agree. I will update the JEP with getters/setters

> 7. I don't see any mention of accessing history items, but presumably
> their properties are similar to bookmarks (except for the
> bookmarks-specific stuff)? I also don't see any mention of annotations; v2?
>

History is quite similar, you change the 'where' property to 'history'
or 'everywhere' (to comingle history and bookmarks). Annotations will
be added later (asap).

> 8. If there's a jetpack.places.bookmarks, then it'd make sense to hang a
> fetch method off it that works just like jetpack.places.fetch but
> assumes where: "bookmarks".
>

I wasn't planning on it since jetpack.places.bookmarks.fetch gets a
bit verbose, however, that might make logical sense.

> 9. It's not clear what the distinction is between where: "history" and
> where: "everywhere". Aren't bookmarks always also in history? Or does
> where: "history" only return history items that aren't also bookmarked?
>

history will have everything in it (including bookmarks) - you just
won't know that it is a bookmark, nor will you have access to tags and
annotations.

> 10. There are both term and phrase search parameters. What's the
> difference, and which one do you do when you pass fetch a simple string?

'term' was a typo, I wrote this JEP before starting on the places API,
and am trying to make them very similar, using 'phrase' and 'fetch',
for instance.

> 11. Finally, I'll bikeshed a bit and suggest that find seems like a bit
> more intuitive name for fetch! ;-)

Yeah, I agree, I thought that making the 2 APIs use the same method
and properties had some merit. I may be wrong, since the places API is
internal to firefox (and will be used by add-on authors as well), and
the Jetpack API should be as elegant as possible. "find" has a pretty
nice ring to it.

Myk: Thanks for the great feedback.

David

[Myk Melez]

On 12/01/2009 09:41 AM, David Dahl wrote:
> On Nov 30, 6:50 pm, Myk Melez<m...@mozilla.org> wrote:
>
>> 1. jetpack.places.bookmark() feels like something that would make more
>> sense hanging off the active tab accessor (i.e. something like
>> tabs.activeTab.bookmark()), since there's nothing about jetpack.places
>> that suggests that its methods would affect the active tab.
>>
>>
> I agree, we should plan on implementing this in the active tab. (or
> all tabs - just add a bookmark method to all of them)
>

Right!

>> 2. It's not clear from the JEP where the bookmarks object comes from.
>> Does it hang off jetpack.places?
>>
> No, I will have to re-work the examples - the bookmarks object is
> inside the callback function - the bookmarks is the iterator that is
> passed into the callback.
>

Aha!

>> 4. I really like the simplicity of specifying a callback that gets all
>> the results at the end. However, since the asynchronous storage API can
>> return results progressively, it's worth thinking a bit about ways we
>> might enable callbacks to take advantage of this. Perhaps a callback
>> that is an object rather than a function could be considered a
>> progressive listener and get regular updates to a long running search?
>>
> excellent idea, perhaps we can optionally provide a listener 'base
> class' that does just that? I think the async implementation (in
> MozStorage) will call the callback for each chunk that is fetched.
>

That makes perfect sense!

>> 8. If there's a jetpack.places.bookmarks, then it'd make sense to hang a
>> fetch method off it that works just like jetpack.places.fetch but
>> assumes where: "bookmarks".
>>
> I wasn't planning on it since jetpack.places.bookmarks.fetch gets a
> bit verbose, however, that might make logical sense.
>

Erm, I just suggested this because I thought there was already a
jetpack.places.bookmarks. As there isn't, I don't think it's worth
adding one just to be able to do jetpack.places.bookmarks.fetch.

>> 9. It's not clear what the distinction is between where: "history" and
>> where: "everywhere". Aren't bookmarks always also in history? Or does
>> where: "history" only return history items that aren't also bookmarked?
>>
> history will have everything in it (including bookmarks) - you just
> won't know that it is a bookmark, nor will you have access to tags and
> annotations.
>

The distinction is still not clear to me. If history includes bookmarks,
how does it differ from everywhere? Is it that everywhere gives you
access to the tags and annotations of bookmarks, while history doesn't?

If so, that seems too slender a distinction, especially if history and
bookmark objects are identical except for the addition of a few extra
properties and methods to the latter. In that case, I'd suggest dropping
everywhere and have history return sets of objects that are either
History objects or Bookmark objects, where Bookmark is a subclass of
History.

>> 10. There are both term and phrase search parameters. What's the
>> difference, and which one do you do when you pass fetch a simple string?
>>
> 'term' was a typo, I wrote this JEP before starting on the places API,
> and am trying to make them very similar, using 'phrase' and 'fetch',
> for instance.
>

Aha!

>> 11. Finally, I'll bikeshed a bit and suggest that find seems like a bit
>> more intuitive name for fetch! ;-)
>>
> Yeah, I agree, I thought that making the 2 APIs use the same method
> and properties had some merit. I may be wrong, since the places API is
> internal to firefox (and will be used by add-on authors as well), and
> the Jetpack API should be as elegant as possible. "find" has a pretty
> nice ring to it.
>

I agree that the Jetpack API should be as elegant as possible, but so
should the Places API! So unless the latter is already locked down, my
recommendation would be to use find in both places. :-)

-myk

[David Dahl]

Just wanted to post that I have updated the Places JEP:
https://wiki.mozilla.org/Labs/Jetpack/JEP/22

There is another thing we might want to consider. Once we move to a
fully asynchronous API, the callback that is passed in may need to
change slightly. The MozStorage 'statement.executeAsync()' takes a
callback object where we have several callback methods:
'handleResult', 'handleError', 'handleCompletion'.

I suggest we begin to support all of these methods now, whereby we
allow a function to be passed in as the callback with no attached
methods to fire for 'handleResult', and an object with all 3 methods
so the developer has very fine-grained control over the places query.
I think we also may want another [optional] method 'beforeResults',
where the developer can specify a "1 time scaffolding function" for UI
changes, for instance where you need to initialize a DOM element like
a <LI> or <TABLE> to iterate results into.

The documentation for executeAsync is here:
https://developer.mozilla.org/en/Storage#Asynchronously

Cheers,

David

[Myk Melez]

On 12/01/2009 11:40 AM, David Dahl wrote:
> I suggest we begin to support all of these methods now, whereby we
> allow a function to be passed in as the callback with no attached
> methods to fire for 'handleResult', and an object with all 3 methods
> so the developer has very fine-grained control over the places query.
>

Note that mozIStorageStatementCallback::handleResult is actually the
method that gets called every time some results are available, not just
once all results have been retrieved.

There's no precise equivalent to the simple function callback in
mozIStorageStatementCallback, but that's ok, since your API can still
implement it (in addition to support for either
mozIStorageStatementCallback callbacks or some simpler variant that
handles progressive results).

-myk

[David Dahl]

On Dec 1, 11:32 am, Myk Melez <m...@mozilla.org> wrote:

> The distinction is still not clear to me. If history includes bookmarks,
> how does it differ from everywhere? Is it that everywhere gives you
> access to the tags and annotations of bookmarks, while history doesn't?
>
> If so, that seems too slender a distinction, especially if history and
> bookmark objects are identical except for the addition of a few extra
> properties and methods to the latter. In that case, I'd suggest dropping
> everywhere and have history return sets of objects that are either
> History objects or Bookmark objects, where Bookmark is a subclass of
> History.
>

I guess this is a little too much influnced by the existing API and
schema where we have moz_bookmarks and moz_history which are both
child sets of moz_places. While the history will always have all urls
you have visited, bookmarks are a separate entity because of the
expiration of history visits. This makes all of the queries under the
hood a bit more complex. tags and annotations are only used on
bookmarks as child records of moz_bookmarks. A history object will
never have a tags or an annotations. I personally would like to re-
configure the entire schema, using the existing schema as a base, but
I think there might be some resistance to this idea for any number of
possibly good reasons.

### side note ###
I think a simpler way to design the schema would to make it so that
all "places" are history objects, and become bookmarks when you update
the object's isBookmark property. The expiration will ignore these
rows, and you end up with 1 table instead of 3. (moz_places,
moz_bookmarks, moz_historyvisits) Places can suffer from very slow
queries. I keep thinking if we want fast queries we need simpler
queries. To do this we need a smarter schema.

This means that the initial APIs will be basic, and once we have a
SQLBuilder and full async MozStorage usage, we can get more
functionality going.

[Andrew Sutherland]

On Tue, Dec 1, 2009 at 11:40 AM, David Dahl <david...@gmail.com> wrote:
> I think we also may want another [optional] method 'beforeResults',
> where the developer can specify a "1 time scaffolding function" for UI
> changes, for instance where you need to initialize a DOM element like
> a <LI> or <TABLE> to iterate results into.

Is the places API (as exposed by Jetpack) conceptually returning
result rows or live objects? My definition of 'result rows' means
they are accurate at the time of the query but potentially become
stale after that point and no effort is made to have at most a single
JS instance per underying database row in memory at a time.

It sounds like a hybrid of both right now. If the eventual goal is
more on the live objects side of the house, you might want your query
results API to resemble Thunderbird's "gloda" API a bit more and the
async storage API a bit less. In gloda, queries beget collections
which have listeners with (optional)
onItemsAdded/onItemsModified/onItemsRemoved/onQueryCompleted
functions.

In gloda, while you can lock down a collection so new matches to
queries won't show up, having the onItemsRemoved semantics always
available is useful in the case where messages get deleted. Even if
you don't care about keeping your UI up-to-date (without performing a
re-query), it might simplify things if you could avoid the case where
the user holds onto an object reference for a no-longer-bookmark
bookmark and starts doing things.

Andrew

[David Dahl]

I have been wowed by gloda for sure. I have been going through the
tests and source to figure out how you guys did the faceted search.
What are the best api source files to look at?

What happens in the case of a user trying to call a method on an
object that no longer exists? I just did a search in thunderbird
(nightly), opened a result message and deleted it, went back to the
search results and tried to open that deleted message again. it just
opens an empty message with the original title in the tab's titlebar.

Am I testing the wrong thing?

I definately want to follow the path that gloda has laid down. The
transformation in thunderbird is staggering. Up until now I was just
considering all search results as 'result rows' and less like objects
with listeners, etc.

David

On Dec 1, 2:06 pm, Andrew Sutherland <sombr...@alum.mit.edu> wrote:

[Andrew Sutherland]

On Tue, Dec 1, 2009 at 3:20 PM, David Dahl <david...@gmail.com> wrote:
> I have been wowed by gloda for sure. I have been going through the
> tests and source to figure out how you guys did the faceted search.
> What are the best api source files to look at?

:)

Some new 30,000 feet gloda documentation you might find useful is:
https://developer.mozilla.org/en/Thunderbird/gloda

The logic that walks the objects in the set and counts the number of
times each value is seen for an attribute is here:
http://mxr.mozilla.org/comm-central/source/mailnews/db/gloda/modules/facet.js

The definition of the attributes and the logic that sets them for each
message during the indexing process can be found here (attributes are
extensible):
http://mxr.mozilla.org/comm-central/source/mailnews/db/gloda/modules/fundattr.js
http://mxr.mozilla.org/comm-central/source/mailnews/db/gloda/modules/explattr.js

The presentation logic that creates the above faceters and causes XBL
bindings to be created is here:
http://mxr.mozilla.org/comm-central/source/mail/base/content/glodaFacetView.js

The XBL bindings are here:
http://mxr.mozilla.org/comm-central/source/mail/base/content/glodaFacetBindings.xml

> What happens in the case of a user trying to call a method on an
> object that no longer exists? I just did a search in thunderbird
> (nightly), opened a result message and deleted it, went back to the
> search results and tried to open that deleted message again. it just
> opens an empty message with the original title in the tab's titlebar.
>
> Am I testing the wrong thing?

Unfortunately the faceted search UI was developed in a bit of a rush
and pre-dated serious correction fixes related to our deletion logic.
If you look in glodaFacetView.js you will find that there is no code
in the "onItemsRemoved" handler. So I am not entirely practicing what
I preach.

That bug is less serious than it would appear because gloda message
objects are immutable and all operations from that UI need to be
translated to a native message header (which will fail, as you
observed when you clicked) before code can try and do anything that
might lead to inconsistent state.

> I definately want to follow the path that gloda has laid down. The
> transformation in thunderbird is staggering. Up until now I was just
> considering all search results as 'result rows' and less like objects
> with listeners, etc.

'Result rows' is definitely a reasonable answer. Gloda's problem
space is much more expansive and can justify a certain amount of
overkill.

My suggestion would then to be to update JEP 22 (or linked
documentation) to mention, briefly:

- The impact of mutation of the returned rows by other code on results
returned. It sounds like the answer is "no impact".
- The impact on returned rows of the use of their mutation methods.
If you have result-row returned copy "A" of a bookmark in one jetpack
feature and then copy "B" in another, and "A" adds the "Apple" tag,
and then "B" adds the "Banana" tag, will the bookmark still end up
with both tags or will the latter operation clobber the first? Will
the "B" copy notice the "A" copy when it triggers the mutation, or
will it still think only "Banana" is there? (Or will "B" only think
Banana is there, but then when the async mutation actually completes
it will get updated to have both?)


I guess my original proposal was that if you are going to eventually
end up at a Gloda-style solution you might make your API pretend that
is what you are doing right now, but that's probably asking for
trouble. Probably better to just force the jetpacks to update
themselves to the new code and semantics when that day comes.

Andrew

[David Dahl]

On Dec 1, 4:20 pm, Andrew Sutherland <sombr...@gmail.com> wrote:

thanks for the great feedback! I am going to spend a little time
digesting this before and while coding anything else.

> My suggestion would then to be to update JEP 22 (or linked
> documentation) to mention, briefly:
>
> - The impact of mutation of the returned rows by other code on results
> returned.  It sounds like the answer is "no impact".
> - The impact on returned rows of the use of their mutation methods.
> If you have result-row returned copy "A" of a bookmark in one jetpack
> feature and then copy "B" in another, and "A" adds the "Apple" tag,
> and then "B" adds the "Banana" tag, will the bookmark still end up
> with both tags or will the latter operation clobber the first?  Will
> the "B" copy notice the "A" copy when it triggers the mutation, or
> will it still think only "Banana" is there?  (Or will "B" only think
> Banana is there, but then when the async mutation actually completes
> it will get updated to have both?)

It may or not be feasible in rev 1 to have all the fancy observers and
what not in the resultRow objects, but I like the idea of the object
being able to update itself in-place. I wonder how the UI callback
will be used in this case? It sounds like it might become quite
complicated - perhaps too complicated (at first) for the main API
exposed to jetpack developers, but good for Firefox work ( and to eat
the dogfood).

> I guess my original proposal was that if you are going to eventually
> end up at a Gloda-style solution you might make your API pretend that
> is what you are doing right now, but that's probably asking for
> trouble.  Probably better to just force the jetpacks to update
> themselves to the new code and semantics when that day comes.
>

I agree with the latter - until we can roll out a fully async
implementation.

David

[Dietrich Ayala]

>>> 11. Finally, I'll bikeshed a bit and suggest that find seems like a bit
>>> more intuitive name for fetch! ;-)
>>>
>> Yeah, I agree, I thought that making the 2 APIs use the same method
>> and properties had some merit. I may be wrong, since the places API is
>> internal to firefox (and will be used by add-on authors as well), and
>> the Jetpack API should be as elegant as possible. "find" has a pretty
>> nice ring to it.
>>
> I agree that the Jetpack API should be as elegant as possible, but so
> should the Places API! So unless the latter is already locked down, my
> recommendation would be to use find in both places. :-)
>
> -myk

The idea is to have only *one* API, that's both used internally, and
exposed in Jetpack. The APIs for Places data should be designed to be
Jetpack-caller-friendly, while providing extensibility mechanisms that
enable the complex requirements of internal use.

IMO the same API design philosophy taken with JEPs should be used for
"internal" APIs wherever possible. This is especially important since
Jetpack will *be* internal soon. I'm pretty excited to see
Jetpack-ness infect the Firefox core :)

[David Dahl]

Good point. I was not entirely thinking about it in that way.