Bug 62293

Summary: IndexedDB: implement IDBFactory.cmp method
Product: WebKit Reporter: Mark Pilgrim (Google) <pilgrim>
Component: New BugsAssignee: Joshua Bell <jsbell>
Status: RESOLVED FIXED    
Severity: Normal CC: andrewjames085, dgrogan, hans, jsbell, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
test case
none
Patch
none
Patch
none
Patch
none
Patch none

Description Mark Pilgrim (Google) 2011-06-08 09:35:01 PDT
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.
Comment 1 Joshua Bell 2011-09-30 16:14:18 PDT
Created attachment 109366 [details]
Patch
Comment 2 Joshua Bell 2011-09-30 16:18:25 PDT
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
Comment 3 David Grogan 2011-09-30 17:27:15 PDT
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.
Comment 4 Hans Wennborg 2011-10-03 07:06:18 PDT
(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.
Comment 5 Joshua Bell 2011-10-03 09:43:00 PDT
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.
Comment 6 Joshua Bell 2011-10-03 10:34:57 PDT
Created attachment 109493 [details]
Patch
Comment 7 Joshua Bell 2011-10-03 10:35:58 PDT
Note that new patch retains per-spec ordering sense for IDBFactory.cmp() - we should NOT land this until discussion on public-webapps is resolved.
Comment 8 David Grogan 2011-10-03 10:45:29 PDT
LGTM
Comment 9 Joshua Bell 2011-10-10 13:18:20 PDT
Created attachment 110387 [details]
Patch
Comment 10 Joshua Bell 2011-10-10 13:20:36 PDT
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.
Comment 11 Hans Wennborg 2011-10-11 01:12:35 PDT
(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
Comment 12 Joshua Bell 2011-10-11 10:04:56 PDT
Tony, would you mind giving this a look?
Comment 13 Tony Chang 2011-10-11 10:18:09 PDT
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.
Comment 14 Joshua Bell 2011-10-11 10:21:55 PDT
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.
Comment 15 Joshua Bell 2011-10-11 11:10:52 PDT
Created attachment 110542 [details]
Patch
Comment 16 WebKit Review Bot 2011-10-11 12:21:21 PDT
Comment on attachment 110542 [details]
Patch

Clearing flags on attachment: 110542

Committed r97168: <http://trac.webkit.org/changeset/97168>
Comment 17 WebKit Review Bot 2011-10-11 12:21:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Andrew James 2015-06-15 21:58:15 PDT
This post is best for learning. It gives excellent information. <a href="http://www.w3schools.com">Visit W3Schools</a>