Re: [IndexedDB] IDBKeyRange cleanup

On Monday, November 1, 2010, Jeremy Orlow <jorlow@chromium.org> wrote:
> 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>."

null is actually currently defined as a valid key, so we can't use it
to signal an absence of key. Though that's something I want to change.
(part of the whole keyPath-for-indexes discussion).

However undefined also feels more javascripy to show that something is
missing or not defined :)

>> 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.)

Eep! How'd I miss that. Will fix asap.

> I only skimmed, but I did notice one thing right away: 1.401: comma before or is incorrect.

I'm sure there are lots of editorial issues. Didn't bother spending
too much time on it since I wanted to get to a point when the document
is more stable.

> 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.

That's what the spec said prior to my changes. Didn't bother fixing it
since cursors had bigger issues.

/ Jonas

Received on Monday, 1 November 2010 20:16:14 UTC