Bug 62293 – IndexedDB: implement IDBFactory.cmp method

• Component: New Bugs
• Status: RESOLVED FIXED

[Mark Pilgrim (Google)]

Created attachment 96435 [details]
test case

http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#widl-IDBFactory-cmp-int-any-first-any-second specifies a method for comparing two keys. WebKit does not implement this method.

[Joshua Bell]

Created attachment 109366 [details]
Patch

[Joshua Bell]

Current issues:
* null is incorrectly supported as a valid key
* NaN is incorrectly supported as a valid key
* Array keys are not supported
* Exception thrown on invalid key is per spec

[David Grogan]

Comment on attachment 109366 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109366&action=review

LG. It's nice to whittle down the amount of api we don't support.

> LayoutTests/storage/indexeddb/factory-basics.html:61
> +function testCompare()

This should probably go into its own layout test, based on its size and that it's not related to opening and closing databases.  We'll put the closeDatabase() stuff here though.

> LayoutTests/storage/indexeddb/factory-basics.html:112
> +        // FIXME: Encoding for expected.txt file

What happens with these guys?

> LayoutTests/storage/indexeddb/factory-basics.html:126
> +        shouldBeTrue("indexedDB.cmp("+key1+","+key2+") === 1");

Nit: can you add spaces between the string concatenation pluses?

> LayoutTests/storage/indexeddb/factory-basics.html:133
> +        "void 0",

What type is this treated as?

> Source/WebCore/storage/IDBFactory.cpp:103
> +    // FIXME: Determine where exceptions are thrown for invalid keys

I don't follow.  We throw wrong exceptions for some keys and should add logic here to fix that?  Oh, I see, you're talking about fixing the "code should be 2. Was 17." lines.  Is that right?  Do you want to do that in this patch or bag it until later?  I guess either way remove the FIXME, and just file a bug if you don't want to deal with fixing it.

> Source/WebCore/storage/IDBFactory.idl:36
> +        long cmp(in IDBKey first, in IDBKey second)

This is long because there's no int in idl?  Aaaaaaand your email to public-webapps answers that for me (there is no int in idl and the spec should probably be changed to return long.)

> Source/WebCore/storage/IDBKey.cpp:52
> +        return (m_date < other->m_date) ? 1 :

The spec's logic seems backwards to me. (Your implementation follows it correctly, though.)

The spec says "returns -1 if the first key is greater than the second."
man strcmp says: "returns an integer less than, equal to, or greater than zero if s1 is found, respectively, to be less than, to match, or be greater than s2."

Isn't strcmp's way conventional?  If this is indeed backwards we should we bring it up on public webapps.

[Hans Wennborg]

(In reply to comment #3)
> Isn't strcmp's way conventional?  If this is indeed backwards we should we bring it up on public webapps.

Yes, please raise this on public-webapps.

I hadn't realized before that cmp() is the reverse of strcmp. It seems like a bug, and I wouldn't want that reverse logic to start finding its way into the code.

[Joshua Bell]

Comment on attachment 109366 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109366&action=review

>> LayoutTests/storage/indexeddb/factory-basics.html:112
>> +        // FIXME: Encoding for expected.txt file
> 
> What happens with these guys?

Ah, these non-ASCII characters didn't survive the javascript -> dumprendertree -> browser -> my clipboard -> terminal -> expected.txt -> test comparison with encoding intact. This is a test issue (which I'll address), not an implementation issue.

>> LayoutTests/storage/indexeddb/factory-basics.html:133
>> +        "void 0",
> 
> What type is this treated as?

void is a one-argument operator that always produces the undefined ES value. "void 0" is more robust than "undefined" because the latter is not a JavaScript keyword, it's a reference to a *usually* undefined property on the global object; this global property wasn't actually defined as immutable until ES5 so in some browsers a malicious script could say "window.undefined = blah" and then undefined... isn't. So some of us paranoid folks prefer this style. I had a comment in for this but took it out - I'll put it back.

>> Source/WebCore/storage/IDBFactory.cpp:103
>> +    // FIXME: Determine where exceptions are thrown for invalid keys
> 
> I don't follow.  We throw wrong exceptions for some keys and should add logic here to fix that?  Oh, I see, you're talking about fixing the "code should be 2. Was 17." lines.  Is that right?  Do you want to do that in this patch or bag it until later?  I guess either way remove the FIXME, and just file a bug if you don't want to deal with fixing it.

My bad, I did investigate but didn't take out the FIXME. The exception originates when we're creating an IDBKey from a V8 value. I'll file a bug.

>> Source/WebCore/storage/IDBKey.cpp:52
>> +        return (m_date < other->m_date) ? 1 :
> 
> The spec's logic seems backwards to me. (Your implementation follows it correctly, though.)
> 
> The spec says "returns -1 if the first key is greater than the second."
> man strcmp says: "returns an integer less than, equal to, or greater than zero if s1 is found, respectively, to be less than, to match, or be greater than s2."
> 
> Isn't strcmp's way conventional?  If this is indeed backwards we should we bring it up on public webapps.

Agreed, mail sent. I never remember the strcmp (etc) ordering and always look at the docs anyway, but consistency is a Good Thing.

[Joshua Bell]

Created attachment 109493 [details]
Patch

[Joshua Bell]

Note that new patch retains per-spec ordering sense for IDBFactory.cmp() - we should NOT land this until discussion on public-webapps is resolved.

[David Grogan]

LGTM

[Joshua Bell]

Created attachment 110387 [details]
Patch

[Joshua Bell]

Comment on attachment 110387 [details]
Patch

There's consensus on public-webapps from moz & ie that cmp's semantics should match strcmp(), so this latest patch implements it that way.

[Hans Wennborg]

(In reply to comment #10)
> (From update of attachment 110387 [details])
> There's consensus on public-webapps from moz & ie that cmp's semantics should match strcmp(), so this latest patch implements it that way.

LGTM

[Joshua Bell]

Tony, would you mind giving this a look?

[Tony Chang]

Comment on attachment 110387 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110387&action=review

> LayoutTests/storage/indexeddb/factory-basics-expected.txt:11
> +FAIL typeof indexedDB.deleteDatabase === 'function' should be true. Was false.

Is this something that's not implemented yet or is it supposed to fail?  In either case, you should probably mention this in the changelog.

> LayoutTests/storage/indexeddb/factory-cmp-expected.txt:98
> +FAIL indexedDB.cmp('�',[]) === -1 should be true. Threw exception Error: TYPE_MISMATCH_ERR: DOM Exception 17
> +FAIL indexedDB.cmp([],'�') === 1 should be true. Threw exception Error: TYPE_MISMATCH_ERR: DOM Exception 17

Is this something that's not implemented yet or is it supposed to fail?

> Source/WebCore/storage/IDBFactory.cpp:105
> +    return first->compare(second.get());

I'm surprised you don't need to static_cast to avoid a compiler warning.

[Joshua Bell]

Comment on attachment 110387 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110387&action=review

>> LayoutTests/storage/indexeddb/factory-basics-expected.txt:11
>> +FAIL typeof indexedDB.deleteDatabase === 'function' should be true. Was false.
> 
> Is this something that's not implemented yet or is it supposed to fail?  In either case, you should probably mention this in the changelog.

That function is NYI - I'll update with comment and changelog

>> LayoutTests/storage/indexeddb/factory-cmp-expected.txt:98
>> +FAIL indexedDB.cmp([],'�') === 1 should be true. Threw exception Error: TYPE_MISMATCH_ERR: DOM Exception 17
> 
> Is this something that's not implemented yet or is it supposed to fail?

Array keys are NYI. I'll add comment/changelog.

>> Source/WebCore/storage/IDBFactory.cpp:105
>> +    return first->compare(second.get());
> 
> I'm surprised you don't need to static_cast to avoid a compiler warning.

Will add the cast, thanks.

[Joshua Bell]

Created attachment 110542 [details]
Patch

[WebKit Review Bot]

Comment on attachment 110542 [details]
Patch

Clearing flags on attachment: 110542

Committed r97168: <http://trac.webkit.org/changeset/97168>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Andrew James]

This post is best for learning. It gives excellent information. <a href="http://www.w3schools.com">Visit W3Schools</a>