Bug 57827 – IndexedDB: Introduce skeleton for LevelDB backend

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

[Hans Wennborg]

IndexedDB: Introduce skeleton for LevelDB backend

[Hans Wennborg]

Created attachment 88202 [details]
Patch

[Hans Wennborg]

This adds an empty implementation of the LevelDB backend, and makes all necessary changes to the interfaces around it.

It's basically https://bugs.webkit.org/show_bug.cgi?id=57372 with the guts ripped out; I think it will be easier to land in pieces like this.

Andrei: can you take a look?

[Andrei Popescu]

> Source/WebCore/storage/IDBBackingStore.h:56
> +    class ObjectStoreRecordHandle : public RefCounted<IDBBackingStore> {

"Handle" seems like the wrong concept for this: this isn't a handle to a resource or something like that, it's just a row identifier. Maybe just call it RecordIdentifier? Also, why is this deriving from RefCounted<IDBBackingStore>?

> Source/WebCore/storage/IDBSQLiteBackingStore.cpp:428
> +    SQLiteRecordHandle(int64_t id) : m_id(id) {}

space between { } ?

> Source/WebCore/storage/IDBSQLiteBackingStore.cpp:-473
> -    ASSERT_UNUSED(ok, ok);

Umm, why delete all this? Now your method doesn't do anything anymore.

> Source/WebCore/storage/IDBSQLiteBackingStore.cpp:523
> +        RefPtr<SQLiteRecordHandle> recordHandle = adoptRef(new SQLiteRecordHandle(objectStoreDataId));

I don't think you should do this? Why not add a create() factory method to SQLiteRecordHandle?

> Source/WebCore/storage/IDBSQLiteBackingStore.cpp:595
> +    SQLiteStatement query(m_db, "DELETE FROM IndexData WHERE objectStoreDataId = ? AND indexId = ?");

This change seems unrelated? Was there a problem here before?

[Hans Wennborg]

Thank you very much for the feedback.

(In reply to comment #3)
> > Source/WebCore/storage/IDBBackingStore.h:56
> > +    class ObjectStoreRecordHandle : public RefCounted<IDBBackingStore> {
> 
> "Handle" seems like the wrong concept for this: this isn't a handle to a resource or something like that, it's just a row identifier. Maybe just call it RecordIdentifier?
Sounds good. Done.

> Also, why is this deriving from RefCounted<IDBBackingStore>?
Oops, that's not intentional :) Fixed.

> 
> > Source/WebCore/storage/IDBSQLiteBackingStore.cpp:428
> > +    SQLiteRecordHandle(int64_t id) : m_id(id) {}
> 
> space between { } ?
Done.

> 
> > Source/WebCore/storage/IDBSQLiteBackingStore.cpp:-473
> > -    ASSERT_UNUSED(ok, ok);
> 
> Umm, why delete all this? Now your method doesn't do anything anymore.
The method still deletes the object store data, it just doesn't delete any index data.

Index data is deleted in deleteIndexDataForRecord(), which I've added a call to in IDBObjectStoreBackendImpl::deleteFunction.

> 
> > Source/WebCore/storage/IDBSQLiteBackingStore.cpp:523
> > +        RefPtr<SQLiteRecordHandle> recordHandle = adoptRef(new SQLiteRecordHandle(objectStoreDataId));
> 
> I don't think you should do this? Why not add a create() factory method to SQLiteRecordHandle?
You're right. Done.

> 
> > Source/WebCore/storage/IDBSQLiteBackingStore.cpp:595
> > +    SQLiteStatement query(m_db, "DELETE FROM IndexData WHERE objectStoreDataId = ? AND indexId = ?");
> 
> This change seems unrelated? Was there a problem here before?

The semantics of the function is changing from deleting all index data for a particular record (which is not practical in the LevelDB backend) to deleting index data for a record from a specific index.

[Hans Wennborg]

Created attachment 88265 [details]
Patch

[Andrei Popescu]

Thanks Hans. One more comment:

> Source/WebCore/storage/IDBLevelDBBackingStore.cpp:33
> +#include <leveldb/db.h>

Just checking: can we actually do this here? Looking at SQLite, we have a wrapper that lives in platform/sql. In general, code that depends on various third-party libraries should probably live in platform?

> Source/WebCore/storage/IDBLevelDBBackingStore.h:36
> +class DB;

Ditto.

[Hans Wennborg]

(In reply to comment #6)
> Thanks Hans. One more comment:
> 
> > Source/WebCore/storage/IDBLevelDBBackingStore.cpp:33
> > +#include <leveldb/db.h>
> 
> Just checking: can we actually do this here? Looking at SQLite, we have a wrapper that lives in platform/sql. In general, code that depends on various third-party libraries should probably live in platform?

There seems to be examples of both wrapped and non-wrapped libraries.

I was looking at libxml, which is used in e.g. Source/WebCore/dom/XMLDocumentParser.h, where the library is included and used directly.

I can see a point in wrapping the library to make the interface more WebKit-like if it were to be used widely within the code, but since it's just us using it at the moment, and it's localized to these two files, I would prefer to use the library directly.

We can always wrap it later if we decide that's better.

[Andrei Popescu]

LGTM

[Hans Wennborg]

Comment on attachment 88265 [details]
Patch

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

> Source/WebCore/WebCore.gyp/WebCore.gyp:2
> +# Copyright (C) 2011 Google Inc. All rights reserved.

Probably no need to update the year here.

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:73
> +void IDBFactoryBackendImpl::open(const String& name, PassRefPtr<IDBCallbacks> callbacks, PassRefPtr<SecurityOrigin> securityOrigin, Frame*, const String& dataDir, int64_t maximumSize, BackingStoreType backingStoreType)

look into having BackingStoreType in RuntimeEnabledFlags instead?

> Source/WebCore/storage/IDBLevelDBBackingStore.cpp:177
> +PassRefPtr<IDBBackingStore::Cursor> IDBLevelDBBackingStore::openIndexKeyCursor(int64_t databaseId, int64_t objectStoreId, int64_t indexId, const IDBKeyRange* range, IDBCursor::Direction direction)

we'll probably get warnings about un-used parameters on some compilers

> Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:232
> +    RefPtr<IDBBackingStore::ObjectStoreRecordIdentifier> recordIdentifier;

instead of passing reference to RefPtr, just pass in the raw pointer and write to it. We'll need to have an "invalid" state for ObjectStoreRecordIdentifier.

> Source/WebCore/storage/IDBSQLiteBackingStore.cpp:462
> +void IDBSQLiteBackingStore::clearObjectStore(int64_t databaseId, int64_t objectStoreId)

warnings? ignore parameters that we ignore

> Source/WebCore/storage/IDBSQLiteBackingStore.cpp:525
> +        if (!callback.callback(*recordIdentifier, value))

Pass const ptr to RecordIdentifier rather than reference.

[David Grogan]

Whose comments are these?

(In reply to comment #9)
> (From update of attachment 88265 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88265&action=review
> 
> > Source/WebCore/WebCore.gyp/WebCore.gyp:2
> > +# Copyright (C) 2011 Google Inc. All rights reserved.
> 
> Probably no need to update the year here.
> 
> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:73
> > +void IDBFactoryBackendImpl::open(const String& name, PassRefPtr<IDBCallbacks> callbacks, PassRefPtr<SecurityOrigin> securityOrigin, Frame*, const String& dataDir, int64_t maximumSize, BackingStoreType backingStoreType)
> 
> look into having BackingStoreType in RuntimeEnabledFlags instead?
> 
> > Source/WebCore/storage/IDBLevelDBBackingStore.cpp:177
> > +PassRefPtr<IDBBackingStore::Cursor> IDBLevelDBBackingStore::openIndexKeyCursor(int64_t databaseId, int64_t objectStoreId, int64_t indexId, const IDBKeyRange* range, IDBCursor::Direction direction)
> 
> we'll probably get warnings about un-used parameters on some compilers
> 
> > Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:232
> > +    RefPtr<IDBBackingStore::ObjectStoreRecordIdentifier> recordIdentifier;
> 
> instead of passing reference to RefPtr, just pass in the raw pointer and write to it. We'll need to have an "invalid" state for ObjectStoreRecordIdentifier.
> 
> > Source/WebCore/storage/IDBSQLiteBackingStore.cpp:462
> > +void IDBSQLiteBackingStore::clearObjectStore(int64_t databaseId, int64_t objectStoreId)
> 
> warnings? ignore parameters that we ignore
> 
> > Source/WebCore/storage/IDBSQLiteBackingStore.cpp:525
> > +        if (!callback.callback(*recordIdentifier, value))
> 
> Pass const ptr to RecordIdentifier rather than reference.

[Hans Wennborg]

(In reply to comment #10)
> Whose comments are these?

They're Steve Block's piped through me. Sorry for the confusion :)

[Hans Wennborg]

(In reply to comment #9)
> (From update of attachment 88265 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88265&action=review
> 
> > Source/WebCore/WebCore.gyp/WebCore.gyp:2
> > +# Copyright (C) 2011 Google Inc. All rights reserved.
> 
> Probably no need to update the year here.
Reverted.

> 
> > Source/WebCore/storage/IDBFactoryBackendImpl.cpp:73
> > +void IDBFactoryBackendImpl::open(const String& name, PassRefPtr<IDBCallbacks> callbacks, PassRefPtr<SecurityOrigin> securityOrigin, Frame*, const String& dataDir, int64_t maximumSize, BackingStoreType backingStoreType)
> 
> look into having BackingStoreType in RuntimeEnabledFlags instead?

Hmm, I'd rather not do that in this patch. It would require a two-sided patch, so if we want to do it, I think it's better with a separate patch.

Also, we haven't quite figured out how we want to test using the different back-ends from DumpRenderTree. Maybe being able to choose back-end like this, rather than just setting the RuntimeEnabledFlags, will help.

> 
> > Source/WebCore/storage/IDBLevelDBBackingStore.cpp:177
> > +PassRefPtr<IDBBackingStore::Cursor> IDBLevelDBBackingStore::openIndexKeyCursor(int64_t databaseId, int64_t objectStoreId, int64_t indexId, const IDBKeyRange* range, IDBCursor::Direction direction)
> 
> we'll probably get warnings about un-used parameters on some compilers
Done. I've just stripped the parameter names.

> 
> > Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:232
> > +    RefPtr<IDBBackingStore::ObjectStoreRecordIdentifier> recordIdentifier;
> 
> instead of passing reference to RefPtr, just pass in the raw pointer and write to it. We'll need to have an "invalid" state for ObjectStoreRecordIdentifier.
Done.

> 
> > Source/WebCore/storage/IDBSQLiteBackingStore.cpp:462
> > +void IDBSQLiteBackingStore::clearObjectStore(int64_t databaseId, int64_t objectStoreId)
> 
> warnings? ignore parameters that we ignore
Done.

> 
> > Source/WebCore/storage/IDBSQLiteBackingStore.cpp:525
> > +        if (!callback.callback(*recordIdentifier, value))
> 
> Pass const ptr to RecordIdentifier rather than reference.
Done.

[Hans Wennborg]

Created attachment 88991 [details]
Patch

[Steve Block]

Comment on attachment 88991 [details]
Patch

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

Please file a bug to track looking into the use of RuntimeEnabledFeatures for the backend type, to avoid passing the type param right through the call stack.

> Source/WebCore/storage/IDBBackingStore.h:61
> +    virtual PassRefPtr<ObjectStoreRecordIdentifier> invalidRecordIdentifier() = 0;

I think this method should probably be named 'createInvalidRecordIdentifier'

[Hans Wennborg]

Thanks for the review, Steve.

(In reply to comment #14)
> (From update of attachment 88991 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88991&action=review
> 
> Please file a bug to track looking into the use of RuntimeEnabledFeatures for the backend type, to avoid passing the type param right through the call stack.
Filed Bug 58237.

> 
> > Source/WebCore/storage/IDBBackingStore.h:61
> > +    virtual PassRefPtr<ObjectStoreRecordIdentifier> invalidRecordIdentifier() = 0;
> 
> I think this method should probably be named 'createInvalidRecordIdentifier'
Done.

[Hans Wennborg]

Committed r83443: <http://trac.webkit.org/changeset/83443>