Bug 59692 – IndexedDB: Unit tests for LevelDB key coding functions

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Hans Wennborg]

IndexedDB: Unit tests for LevelDB key coding functions

[Hans Wennborg]

Created attachment 91476 [details]
Patch

[David Grogan]

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?

[Hans Wennborg]

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

[Hans Wennborg]

Created attachment 92057 [details]
Patch

[Hans Wennborg]

Steve, would you like to take a look?

[David Grogan]

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

[Steve Block]

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.

[Hans Wennborg]

Created attachment 92236 [details]
Patch

[Hans Wennborg]

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

[Steve Block]

Comment on attachment 92236 [details]
Patch

r=me

[Hans Wennborg]

Committed r85844: <http://trac.webkit.org/changeset/85844>