Bug 59692 - IndexedDB: Unit tests for LevelDB key coding functions
Summary: IndexedDB: Unit tests for LevelDB key coding functions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Hans Wennborg
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-28 06:12 PDT by Hans Wennborg
Modified: 2011-05-05 01:36 PDT (History)
3 users (show)

See Also:


Attachments
Patch (14.76 KB, patch)
2011-04-28 06:17 PDT, Hans Wennborg
no flags Details | Formatted Diff | Diff
Patch (15.08 KB, patch)
2011-05-03 02:46 PDT, Hans Wennborg
no flags Details | Formatted Diff | Diff
Patch (15.54 KB, patch)
2011-05-04 07:07 PDT, Hans Wennborg
steveblock: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hans Wennborg 2011-04-28 06:12:35 PDT
IndexedDB: Unit tests for LevelDB key coding functions
Comment 1 Hans Wennborg 2011-04-28 06:17:12 PDT
Created attachment 91476 [details]
Patch
Comment 2 David Grogan 2011-04-28 19:30:58 PDT
Comment on attachment 91476 [details]
Patch

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

LGTM

I checked about 80% of these to make sure I understood what was going on.

> Source/WebCore/storage/IDBLevelDBCoding.cpp:192
> +    int shift = 0;

How did these get by the layout tests?

> Source/WebKit/chromium/tests/IDBLevelDBCodingTest.cpp:84
> +TEST(IDBLevelDBCodingTest, EncodeInt)

We just use encodeint and decodeint for the meta data right?  Integers in the key are always encoded as part of a NumberType IDBKey?  How do we convert integers as the Value into a string?  Just dump the SerializedScriptValue internal representation?  Sorry for the lame out-of-scope questions.

> Source/WebKit/chromium/tests/IDBLevelDBCodingTest.cpp:103
> +            n += 1234;

This maxes out at 499 * 1234 + 1000 * 63 = 678766?  Does it handle INT64_MAX?  What does it do with INT64_MAX + 5?

> Source/WebKit/chromium/tests/IDBLevelDBCodingTest.cpp:298
> +    keys.append(IDBKey::createDate(100000));

What does the parameter to createDate represent?
Comment 3 Hans Wennborg 2011-05-03 02:44:33 PDT
(In reply to comment #2)
> (From update of attachment 91476 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91476&action=review
> 
> LGTM
> 
> I checked about 80% of these to make sure I understood what was going on.
> 
> > Source/WebCore/storage/IDBLevelDBCoding.cpp:192
> > +    int shift = 0;
> 
> How did these get by the layout tests?

That's a good question. I suppose none of the tests caused us to encode any integers larger than 255. Having these unit tests will make me sleep better at night :)

> 
> > Source/WebKit/chromium/tests/IDBLevelDBCodingTest.cpp:84
> > +TEST(IDBLevelDBCodingTest, EncodeInt)
> 
> We just use encodeint and decodeint for the meta data right?

Yes.

> Integers in the key are always encoded as part of a NumberType IDBKey?

Yes.

> How do we convert integers as the Value into a string?  Just dump the SerializedScriptValue internal representation?

Exactly. From the back-end point of view, all values are String objects.

> Sorry for the lame out-of-scope questions.

No worries, I'm glad someone is paying attention :)

> 
> > Source/WebKit/chromium/tests/IDBLevelDBCodingTest.cpp:103
> > +            n += 1234;
> 
> This maxes out at 499 * 1234 + 1000 * 63 = 678766?  Does it handle INT64_MAX?

Yes, that works. Adding that to the test.

> What does it do with INT64_MAX + 5?

How do you mean? We can't pass that to the function anyway..

> 
> > Source/WebKit/chromium/tests/IDBLevelDBCodingTest.cpp:298
> > +    keys.append(IDBKey::createDate(100000));
> 
> What does the parameter to createDate represent?

For date values, we use a double representing number of seconds since the epoch. This is the same as V8 does internally.
Comment 4 Hans Wennborg 2011-05-03 02:46:18 PDT
Created attachment 92057 [details]
Patch
Comment 5 Hans Wennborg 2011-05-03 02:47:15 PDT
Steve, would you like to take a look?
Comment 6 David Grogan 2011-05-03 18:24:03 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 91476 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=91476&action=review
> > > Source/WebKit/chromium/tests/IDBLevelDBCodingTest.cpp:103
> > > +            n += 1234;
> > 
> > This maxes out at 499 * 1234 + 1000 * 63 = 678766?  Does it handle INT64_MAX?
> 
> Yes, that works. Adding that to the test.
> 
> > What does it do with INT64_MAX + 5?
> 
> How do you mean? We can't pass that to the function anyway..

I suppose the compiler wouldn't let us?  I was curious what this would yield, just so we know

decode(encode(INT64_MAX + 5))
Comment 7 Steve Block 2011-05-04 04:07:56 PDT
Comment on attachment 92057 [details]
Patch

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

> Source/WebCore/storage/IDBLevelDBCoding.cpp:195
> +        ret = ret | (static_cast<int64_t>(c) << shift);

Use |=

> Source/WebCore/storage/IDBLevelDBCoding.cpp:227
> +        foundInt |= static_cast<int64_t>(*p & 0x7f) << shift;

This might be easier to read if you use a local 'char c' like in decodeInt.

> Source/WebCore/storage/IDBLevelDBCoding.cpp:229
>      } while (*p++ & 128);

Would '0x80' be easier to read than '128', since you use hex for the other mask? - throughout.

> Source/WebKit/chromium/tests/IDBLevelDBCodingTest.cpp:34
> +#if ENABLE(LEVELDB)

Should these be above this block of headers?

> Source/WebKit/chromium/tests/IDBLevelDBCodingTest.cpp:46
> +    for (int i = 0; i < 256; ++i) {

I don't know about Chromium style, but I thought that usually one tests only the 'interesting' values, rather than doing all by brute force, so it's clear what we're testing? Maybe 0, 1, 255 here?

> Source/WebKit/chromium/tests/IDBLevelDBCodingTest.cpp:47
> +        unsigned char c = i;

Wwhy not loop on a char to avoid the local?

> Source/WebKit/chromium/tests/IDBLevelDBCodingTest.cpp:86
> +    EXPECT_EQ(static_cast<size_t>(1), encodeInt(1).size());

What should encodeInt(0)do? Should we add a test? Same for encodeVarInt.

> Source/WebKit/chromium/tests/IDBLevelDBCodingTest.cpp:97
> +    for (int i = 0; i < 1000; i++) {

Same comment about brute force

> Source/WebKit/chromium/tests/IDBLevelDBCodingTest.cpp:164
> +    EXPECT_EQ(static_cast<size_t>(2), encodeString("a").size());

encodeString("") ?

> Source/WebKit/chromium/tests/IDBLevelDBCodingTest.cpp:167
> +    EXPECT_EQ(static_cast<size_t>(4), encodeString(testStringB).size());

It looks like there's only a single encodeString() function, and it takes 'const String&'. So the conversion from 8 and 16 bit char arrays to a 16 bit String is done by the String constructor, right? Given this, is there any point in testing both 8 and 16 bit here? If so, I think you should use explicit calls to String(). Same comment for 'WithLength'.

> Source/WebKit/chromium/tests/IDBLevelDBCodingTest.cpp:245
> +    EXPECT_EQ(static_cast<size_t>(8), encodeDouble(3.14).size());

encodeDouble(0.0) ?

> Source/WebKit/chromium/tests/IDBLevelDBCodingTest.cpp:266
> +    EXPECT_FALSE(p);

EXPECT_EQ(0, p)? Same comment below.
Comment 8 Hans Wennborg 2011-05-04 07:07:45 PDT
Created attachment 92236 [details]
Patch
Comment 9 Hans Wennborg 2011-05-04 07:08:07 PDT
(In reply to comment #7)
> (From update of attachment 92057 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92057&action=review

Thanks very much for the review Steve.

> 
> > Source/WebCore/storage/IDBLevelDBCoding.cpp:195
> > +        ret = ret | (static_cast<int64_t>(c) << shift);
> 
> Use |=
Done.

> 
> > Source/WebCore/storage/IDBLevelDBCoding.cpp:227
> > +        foundInt |= static_cast<int64_t>(*p & 0x7f) << shift;
> 
> This might be easier to read if you use a local 'char c' like in decodeInt.
Done. (I assume you mean for *p, right?)

> 
> > Source/WebCore/storage/IDBLevelDBCoding.cpp:229
> >      } while (*p++ & 128);
> 
> Would '0x80' be easier to read than '128', since you use hex for the other mask? - throughout.
Yes, that's probably better. Done.

> 
> > Source/WebKit/chromium/tests/IDBLevelDBCodingTest.cpp:34
> > +#if ENABLE(LEVELDB)
> 
> Should these be above this block of headers?
Yes, that makes sense. Done.

> 
> > Source/WebKit/chromium/tests/IDBLevelDBCodingTest.cpp:46
> > +    for (int i = 0; i < 256; ++i) {
> 
> I don't know about Chromium style, but I thought that usually one tests only the 'interesting' values, rather than doing all by brute force, so it's clear what we're testing? Maybe 0, 1, 255 here?
Yes, makes sense. Done.

> 
> > Source/WebKit/chromium/tests/IDBLevelDBCodingTest.cpp:47
> > +        unsigned char c = i;
> 
> Wwhy not loop on a char to avoid the local?
Because a char would always be less than 256, and the loop would never finish. But that loop is gone now anyway.

> 
> > Source/WebKit/chromium/tests/IDBLevelDBCodingTest.cpp:86
> > +    EXPECT_EQ(static_cast<size_t>(1), encodeInt(1).size());
> 
> What should encodeInt(0)do? Should we add a test? Same for encodeVarInt.
Yes, we should add a test. Done.

> 
> > Source/WebKit/chromium/tests/IDBLevelDBCodingTest.cpp:97
> > +    for (int i = 0; i < 1000; i++) {
> 
> Same comment about brute force
Done.

> 
> > Source/WebKit/chromium/tests/IDBLevelDBCodingTest.cpp:164
> > +    EXPECT_EQ(static_cast<size_t>(2), encodeString("a").size());
> 
> encodeString("") ?
Done.

> 
> > Source/WebKit/chromium/tests/IDBLevelDBCodingTest.cpp:167
> > +    EXPECT_EQ(static_cast<size_t>(4), encodeString(testStringB).size());
> 
> It looks like there's only a single encodeString() function, and it takes 'const String&'. So the conversion from 8 and 16 bit char arrays to a 16 bit String is done by the String constructor, right? Given this, is there any point in testing both 8 and 16 bit here?

I want to test that encodeInt can also handle Strings where some characters are using more than the low eight bits.

> If so, I think you should use explicit calls to String(). Same comment for 'WithLength'.

Done.

> 
> > Source/WebKit/chromium/tests/IDBLevelDBCodingTest.cpp:245
> > +    EXPECT_EQ(static_cast<size_t>(8), encodeDouble(3.14).size());
> 
> encodeDouble(0.0) ?
Done.

> 
> > Source/WebKit/chromium/tests/IDBLevelDBCodingTest.cpp:266
> > +    EXPECT_FALSE(p);
> 
> EXPECT_EQ(0, p)? Same comment below.

Done. Fixed throughout.
Comment 10 Steve Block 2011-05-04 07:15:58 PDT
Comment on attachment 92236 [details]
Patch

r=me
Comment 11 Hans Wennborg 2011-05-05 01:36:49 PDT
Committed r85844: <http://trac.webkit.org/changeset/85844>