[Webkit-unassigned] [Bug 62780] Migrate SQLite backing store to LevelDB backing store for Indexeddb.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=62780





--- Comment #7 from Hans Wennborg <hans at chromium.org>  2011-06-21 03:49:03 PST ---
(From update of attachment 97872)
Hi Greg,

Thanks again for working on this. It's a tricky area.

My main worry is that the patch confuses backing stores and databases, and fileIdentifiers and uniqueIdentifiers. See comments inline, and ping me if there's anything I can clarify.


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

> LayoutTests/storage/indexeddb/migrate-basics.html:154
> +    shouldBeTrue("(window.store.index('ExampleIndex') != undefined)");

we also need to check that the index actually contains anything

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:95
> +    IDBBackingStoreMap::iterator it2 = m_backingStoreMap.find(uniqueIdentifier);

This is a regression of what I fixed in Bug 62382. It is important that m_backingStoreMap is keyed *per origin* (fileIdentifier) rather than per database (uniqueIdentifier).

The m_backingStoreMap's purpose is to cache open backing stores, so that we only keep a single connection to each backing store. If a LevelDB database is opened more than once simultaneously, it will cause corruption.

This comment applies throughout the patch.

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:104
> +            // LayoutTests: SQLite backing store may not exist on disk but may exist in cache.

Nice catch; I hadn't thought of this.

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:117
> +            backingStore = IDBSQLiteBackingStore::open(securityOrigin.get(), dataDir, maximumSize, uniqueIdentifier, this);

Again, this seems to undo the patch from Bug 62382. Is this deliberate, or a merge thing?

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:120
> +            backingStore = IDBLevelDBBackingStore::open(securityOrigin.get(), dataDir, maximumSize, uniqueIdentifier, this);

Ditto.

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:148
> +        if (!toObjStoreNames.contains(fromObjStoreNames[i])) {

I think what Eric meant by his "early continue" comment is that you could turn this into

if (toObjStoreNames.contains(fromObjStoreNames[i]))
    continue;

// do migration here...

I.e. the functionality is the same, it's just a style thing, reducing the nesting of blocks.

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:159
> +                keyPath = true;

Maybe "bool hasKeyPath = !fromKeyPaths[i].isEmpty();" is simpler. Also, I'm not sure what this is used for, see comment below.

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:181
> +                ASSERT(keyPath);

If I understand correctly, this asserts that the object store has a key path. I don't see why that would hold? It's prefectly ok for an object store not to have a key path, and have indexes.

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:184
> +                    break;

maybe return false?

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:208
> +    toDatabaseExistedBefore = IDBLevelDBBackingStore::backingStoreExists(securityOrigin, dataDir);

this seems to check that the "to origin" exists, which isn't the same as that the "to database" exists in that origin.

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:232
> +    // Open "To" database. First find out if it already exists -- this will determine if

the "First find out if it already exists" comment doesn't seem to apply here.. you're just opening the backing store?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.