[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 #8 from Greg Simon <gregsimon at chromium.org>  2011-06-21 06:52:58 PST ---
(In reply to comment #7)
> (From update of attachment 97872 [details])
> 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.

The separation of backing store and backends was present before this patch. I believe this is the correct way to do it because the two are related to each other (the backend has a pointer to the backingstore, which are both going to be different depending on leveldb or sqlite. Ideally there would not be a different between backing store and backend. 

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

The backend keeps a pointer to a backing store so they are related even though they are separate C++ objects. This is an important fact in the migration code which is why I keep them together.

> 
> 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;

Ah yes I missed that.

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

I guess am misunderstanding the spec then because it mentions indexes using a keypath. I can remove the assertion.

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

Hmm. Yes, right -- the name needs to be in there too.

The purpose of this extra check is because if you call extractMetaData on an emptydatabase it seems to corrupt the store and you can't populate it anymore. OTOH, calling setDatabaseMetaData more than once increments the databaseId each time. THis was not a problem until we decided we would do partial/merge migrations. I suspect we should *not* do merge/partial migrations, which was the logic captured in patch 61000.

> > 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?

See above comment about extractMetaData.

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