Re: [IndexedDB] IDBKeyRange cleanup

On Sat, Oct 30, 2010 at 12:24 AM, Jonas Sicking <jonas@sicking.cc> wrote:

> On Sun, Oct 24, 2010 at 8:06 AM, Jeremy Orlow <jorlow@chromium.org> wrote:
> > On Sat, Oct 23, 2010 at 2:45 AM, Jonas Sicking <jonas@sicking.cc> wrote:
> >>
> >> Hi all,
> >>
> >> IDBKeyRange is in need of some cleanup. The first issue is its
> >> constructors. Currently the IDL for IDBKeyRange, define that the
> >> constructors, .only, .leftBound, .rightBound, .bound, on object
> >> instances themselves. I don't think this is intentional since first of
> >> all it makes it impossible to create the first IDBKeyRange, second,
> >> second it seems strange to have constructor functions on instances.
> >>
> >> It seems like the intent is that the following should work:
> >>
> >> x = new IDBKeyRange.bound(...);
> >> and possibly also
> >> x = IDBKeyRange.bound(...);
> >> (the latter works for for example XMLHttpRequest, JS-heads have also
> >> informed me that it should work).
> >
> > The latter is what we do in Chromium and what I'm pretty sure we agreed
> upon
> > in earlier threads.  Since these methods are factories, it seems as
> though
> > the new operator should not be necessary.
> >
> >>
> >> Unfortunately there is no way to express this in WebIDL, so I think we
> >> need to describe it in prose instead. I'll raise this with Cameron,
> >> but I think that since at this point only IndexedDB uses these
> >> "static" functions, it might not make sense to add support to WebIDL.
> >
> > I believe "class methods" is the more accepted term for such methods.
>  I'd
> > rather we look at adding these to WebIDL now rather than waiting for the
> > next instance that uses them as I'd hope we didn't add a new concept just
> > for one interface in one API.  Prose seems like the right solution in the
> > mean time though.
> >
> >>
> >> The other thing is that the .flags attribute is really not very
> >> javascript friendly as it uses bitfields. It also is weird in that it
> >> has multiple bits, but some have strong interdependencies. For example
> >> if SINGLE is set LEFT_OPEN and RIGHT_OPEN must be false, and
> >> LEFT_BOUND and RIGHT_BOUND must be true. It also seems like the
> >> LEFT_BOUND flag is closely correlated to the .left property.
> >>
> >> I suggest the following API instead:
> >>
> >> interface IDBKeyRange {
> >>  readonly attribute any left;
> >>  readonly attribute any right;
> >>  readonly attribute boolean leftOpen;
> >>  readonly attribute boolean rightOpen;
> >> };
> >>
> >> the .left and .right properties return 'null' or 'undefined' (to be
> >> discussed) when the keyrange isn't bound in that direction.
> >
> > I agree the flags attribute is confusing and not very JavaScript like.  I
> > also like your solution.  null is probably what it should return when not
> > bounded.
>

Why did you end up using undefined rather than null?  "The <a>key range</a>
<a href="#widl-IDBKeyRange-lower"><code>lower</code></a> value is
<code>undefined</code> or <a>less than</a> <var>key value</var>."


> > What should the open param return if its associated left/right is not
> bound?
> >  I don't care much, but it should definitely be specced.
> >>
> >> Another question I had is if "left" and "right" really are good names.
> >> Seems to me that "upper" and "lower" are better words, but it might be
> >> that left/right is established concepts in the database community? I
> >> don't feel strongly on this issue. Oh, I see now that bug 10397 is
> >> filed on this exact issue.
> >
> > Yup.  Agree that left/right are bad names.
> > The fact that we're discussing things over again points to the fact that
> we
> > really need to get on top of all the bugs against the spec.  Most of them
> > should be pretty easy.  Will the editors have time to address these
> before
> > TPAC?  If not, does anyone have a problem with me becoming an editor?
> >
> >>
> >> Lastly, should we allow values to be passed directly to all APIs that
> >> currently take IDBKeyRanges? It seems nice to not require people to
> >> create an IDBKeyRange object using patterns like:
> >>
> >> req = objectStore.openCursor(new IDBKeyRange.only(foo));
> >>
> >> and instead allow
> >>
> >> req = objectStore.openCursor(foo);
> >
> > I'm OK with this.
> >
> >>
> >> Similarly, should we allow IDBObjectStore.get on and IDBIndex.get to
> >> take a IDBKeyRange and return the first value that matches the
> >> keyrange. This would allow things like "give me the first entry with a
> >> value greater than X".
> >
> > That seems like it could be valuable.  I'd support such a change.
>
> I've implemented these changes in
> http://dvcs.w3.org/hg/IndexedDB/rev/e5a08025e518
>
> There is definitely a lot of work remaining on the spec. I've added a
> lot of infrastructure which should make this *much* easier, such as
> defining algorithms for request handling, but the work to fully
> integrate that everywhere is still remaining. Much of it tightening up
> the language to make things more strictly defined. One thing that is
> totally lacking for example is integration with event loops, another
> is cursors.
>
> So the more help we can get, the better. We'd just need to be better
> at assigning bugs to ourselves before working on them as to avoid
> stepping on each others toes.
>

The text looks better.  The formatting seems to be screwed up though.  (A
large part of the doc is now red.)

I only skimmed, but I did notice one thing right away: 1.401: comma before
or is incorrect.  Also, is this implying that opening a cursor that has no
results should throw and/or fire on error?  It doesn't seem super clear.

Thanks for doing this,
J