Bug 111233 – IndexedDB: Add IDBBackingStoreTest

• Component: New Bugs
• Status: RESOLVED FIXED

[Alec Flett]

IndexedDB: Add IDBBackingStoreTest

[Alec Flett]

Created attachment 191064 [details]
Patch

[Alec Flett]

jsbell / dgrogan - mind taking a look? This will let us do all sorts of things, like programmatically corrupt the DB in different ways beyond just LevelDB corruption.

Note that the headers are weirdly spaced but this is the only way that the webkit-style checker would allow them to be.

[Joshua Bell]

Comment on attachment 191064 [details]
Patch

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

> Source/WebKit/chromium/ChangeLog:13
> +        (WebCore):

Delete these generic lines since they don't add value to the ChangeLog.

> Source/WebKit/chromium/tests/IDBBackingStoreTest.cpp:47
> +        String pathBase;

Add comment that empty pathBase == in-memory database?

(I may refactor to be an explicit IDBBackingStore::openInMemory so that decision is made in IDBFactory, but that's uncertain.)

> Source/WebKit/chromium/tests/IDBBackingStoreTest.cpp:59
> +protected:

Blank line before/no blank after?

> Source/WebKit/chromium/tests/IDBBackingStoreTest.cpp:73
> +

Extra blank line?

> Source/WebKit/chromium/tests/IDBBackingStoreTest.cpp:101
> +        bool ok = m_backingStore->putRecord(&transaction1, 1UL << 35, 1UL << 39, *m_key1.get(), SharedBuffer::create(m_value1.data(), m_value1.size()), &record);

Use consts for 1UL<<35, 1UL<<39 ?

> Source/WebKit/chromium/tests/IDBBackingStoreTest.cpp:111
> +        bool ok = m_backingStore->getRecord(&transaction2, 1UL << 35, 1UL << 39, *m_key1.get(), resultValue);

... since they're repeated here.

> Source/WebKit/chromium/tests/IDBBackingStoreTest.cpp:123
> +    int64_t intVersion = 9;

const for all of these?

> Source/WebKit/chromium/tests/IDBBackingStoreTest.cpp:135
> +        bool ok = m_backingStore->createIDBDatabaseMetaData("db1", version, intVersion, databaseId);

constant for "db1" ?

> Source/WebKit/chromium/tests/IDBBackingStoreTest.cpp:142
> +        ok = m_backingStore->createObjectStore(&transaction, databaseId, objectStoreId, "objectStore1", objectStoreKeyPath, autoIncrement);

EXPECT_TRUE(ok) after this?

> Source/WebKit/chromium/tests/IDBBackingStoreTest.cpp:147
> +

Spacing looks weird here.

> Source/WebKit/chromium/tests/IDBBackingStoreTest.cpp:154
> +        bool ok = m_backingStore->getIDBDatabaseMetaData("db1", &database, found);

EXPECT_TRUE(found) ?

> Source/WebKit/chromium/tests/IDBBackingStoreTest.cpp:157
> +        // database.name is not filled in by the implementation.

Huh... I wonder why we even have it as a property in IDBMetadata? Ah well.

> Source/WebKit/chromium/tests/IDBBackingStoreTest.cpp:166
> +        EXPECT_EQ("objectStore1", objectStore.name);

EXPECT_EQ's for the other object store properties?

> Source/WebKit/chromium/tests/IDBBackingStoreTest.cpp:173
> +

Lots of extra whitespace here?

[Alec Flett]

Created attachment 191270 [details]
Patch

[Alec Flett]

jsbell - ready for another review, all comments addressed.

[Joshua Bell]

Comment on attachment 191270 [details]
Patch

lgtm

[Adam Barth]

Comment on attachment 191270 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBBackingStore.cpp:344
> +    // Only null in tests.

Can we make this comment into an ASSERT?

[Alec Flett]

abarth@ - is there an existing way to do test-only asserts beyond plumbing a boolean value in from our tests + non-test code? I couldn't find any examples in existing code.

[Adam Barth]

You can check whether

WebKit::Platform()->unitTestSupport()

is non-null:

ASSERT(m_factory || WebKit::Platform()->unitTestSupport());

You might need to wrap that in a PLATFORM(CHROMIUM) ifdef.

[Alec Flett]

Created attachment 191358 [details]
Patch

(thanks..) update to detect webkit unit tests

[WebKit Review Bot]

Comment on attachment 191358 [details]
Patch

Attachment 191358 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16930052

[Alec Flett]

Created attachment 191535 [details]
Patch

[Alec Flett]

abarth@ - r? 

(I had to use WebKit::Platform::current() instead of WebKit::Platform())

[Adam Barth]

Comment on attachment 191535 [details]
Patch

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

> Source/WebKit/chromium/tests/IDBBackingStoreTest.cpp:32
> +#include "config.h"
> +
> +#include "IDBBackingStore.h"
> +
> +#include "SharedBuffer.h"
> +
> +#include <gtest/gtest.h>

So many blank lines!  :)

[Alec Flett]

Comment on attachment 191535 [details]
Patch

(those blank lines are soley to make the webkit-style checker happy)

[WebKit Review Bot]

Comment on attachment 191535 [details]
Patch

Clearing flags on attachment: 191535

Committed r144830: <http://trac.webkit.org/changeset/144830>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.