Summary: | IndexedDB: Introduce skeleton for LevelDB backend | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hans Wennborg <hans> | ||||||||
Component: | New Bugs | Assignee: | Hans Wennborg <hans> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | andreip, dgrogan, steveblock | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Other | ||||||||||
OS: | OS X 10.5 | ||||||||||
Attachments: |
|
Description
Hans Wennborg
2011-04-05 04:33:48 PDT
Created attachment 88202 [details]
Patch
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? > 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? 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. Created attachment 88265 [details]
Patch
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. (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. LGTM 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. 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. (In reply to comment #10) > Whose comments are these? They're Steve Block's piped through me. Sorry for the confusion :) (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. Created attachment 88991 [details]
Patch
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' 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. Committed r83443: <http://trac.webkit.org/changeset/83443> |