Implement memory based storage for IndexedDB
Created attachment 59838 [details] Patch
This is a big patch. I can split it up some to make it more easily landable, but I think the best I can do is 2 patches that'd put in "dead code" to be used by the third. (IDBKey, then IDBKeyTree, then the rest.) A LOT of this patch is boilerplate (easy to review) or build file changes (I basically had to re-do the IDB files in the xcode project since what's there doesn't currently build anymore and I couldn't figure out what was wrong). Darin: please look at the WebKit layer. Dimitri/Nate: please look at the bindings changes (especially the code generator). Marcus/Andrei: Please look at the IDB specific stuff. I think anyone can review the rest. The AVLTree stuff is surprisingly straightforward.
Actually, I think it should make it substantially easier to review if split in two.
Oops...forgot Darin. But nothing to do here until the other is ready to land.
Created attachment 59854 [details] Patch
It's still big, but over half of it is build file changes and most of the rest is really straightforward stuff. I don't think it can really be split up much further.
Oh, and this depends on 41250.
Er...52.
Created attachment 59989 [details] Patch
> idb-objectstore-request.js > > function openSuccess() > { > debug("openSuccess():"); Is that a tab? > function createSuccess() > { > debug("createSuccess():"); ditto > function addSuccess() > { > debug("addSuccess():"); ditto > IDBObjectStoreRequest.cpp > > IDBObjectStoreRequest::modify Is it me or is the IDBObjectStore IDL out of date? What we have is: add(), addOrModify(), modify(), remove() and what we should have, according to the spec, is: add(), put(), get(), remove() In fact, we decided in http://lists.w3.org/Archives/Public/public-webapps/2010AprJun/0512.html to drop the ModifyOnly semantics altogether, right? > IDBObjectStoreImpl.cpp > > void IDBObjectStoreImpl::get(PassRefPtr<IDBKey> key, PassRefPtr<IDBCallbacks> callbacks) > { > RefPtr<SerializedScriptValue> value = m_tree->get(key); > > callbacks->onSuccess(value.get()); > } Maybe I am wrong, but I have the same question as in the other patch. We're not really transferring any ownership and all we really need is the raw pointer. Do we need the RefPtr here or can we just use raw pointers? > 90 bool hasValue = m_tree->get(key); Umm...doesn't get() return a PassRefPtr? > IDBObjectStoreProxy.h Why did you remove 'virtual'? Is that a WebKit style rule? I thought it's useful to flag that those methods are virtual...
(In reply to comment #10) > > idb-objectstore-request.js > > > > function openSuccess() > > { > > debug("openSuccess():"); > > Is that a tab? > > > function createSuccess() > > { > > debug("createSuccess():"); > > ditto > > > function addSuccess() > > { > > debug("addSuccess():"); > > ditto > > > IDBObjectStoreRequest.cpp > > > > IDBObjectStoreRequest::modify > > Is it me or is the IDBObjectStore IDL out of date? What we have is: > > add(), addOrModify(), modify(), remove() > > and what we should have, according to the spec, is: > > add(), put(), get(), remove() > > In fact, we decided in > > http://lists.w3.org/Archives/Public/public-webapps/2010AprJun/0512.html > > to drop the ModifyOnly semantics altogether, right? This was up to date when I started. Given that the spec is a moving target and this patch is blocking people and the backend (put with a enum) will stay the same, I figured it was best to move forward with what was there and then do a follow up patch once things stabilized. I guess they've stabilized so I can do this now, but if everything else looks good before I do that, I strongly feel that we should get this patch in first and then change it in a subsequent patch. No one's shipping this code yet, so it's not a big deal. It'll also make it clearer (to me and reviewers) about what's changing so we can make sure no bits of add/addOrModify/modify are left. > > IDBObjectStoreImpl.cpp > > > > void IDBObjectStoreImpl::get(PassRefPtr<IDBKey> key, PassRefPtr<IDBCallbacks> callbacks) > > { > > RefPtr<SerializedScriptValue> value = m_tree->get(key); > > > > callbacks->onSuccess(value.get()); > > } > > Maybe I am wrong, but I have the same question as in the other patch. We're not really transferring > any ownership and all we really need is the raw pointer. Do we need the RefPtr here or can we just use raw pointers? In the Request code, I think we either have to take PassRefPtrs or we need to make the code generators call .get() on themselves. It's probably possible, but probably best for a second patch (this one is already so big). For these, yeah...I guess we could make them take pointers since the caller should keep them from going away in the mean time. > > 90 bool hasValue = m_tree->get(key); > > Umm...doesn't get() return a PassRefPtr? Yeah. So that becomes true if the value exists or false if not. Which is all I care about here. > > IDBObjectStoreProxy.h > > Why did you remove 'virtual'? Is that a WebKit style rule? I thought it's useful to flag that those methods are virtual... You omitted virtual on a bunch of these and I was going to fix it, but then I realized there's really no need. No one will be subclassing anything other than the interface. And the interface is virtual. It is webkit practice to make nothing virtual that doesn't need to be, so this seemed prudent.
Comment on attachment 59989 [details] Patch LayoutTests/storage/indexeddb/script-tests/idb-objectstore-request.js:15 + debug("openSuccess():"); nit: indentation LayoutTests/storage/indexeddb/script-tests/idb-objectstore-request.js:30 + debug("createSuccess():"); nit: indentation LayoutTests/storage/indexeddb/script-tests/idb-objectstore-request.js:46 + debug("addSuccess():"); ditto WebCore/storage/IDBObjectStoreImpl.cpp:77 + callbacks->onError(IDBDatabaseError::create(IDBDatabaseException::UNKNOWN_ERR, "A key was supplied for an objectStore that has a keyPath.")); it might be nice to define a helper method named notifyError that takes the callbacks, exception type, and string. WebKit/chromium/public/WebIDBKey.h:42 + WEBKIT_API static WebIDBKey createInvalid(); nit: please add a new line after this line to help readability WebKit/chromium/public/WebIDBKey.h:62 + /* Types not in WebCore::IDBKey */ nit: use C++ style comment WebKit/chromium/public/WebIDBKey.h:67 + WEBKIT_API WebString string() const; // Only valid for StringType. nit: maybe these methods should be named toString and toNumber or asString and asNumber? WebKit/chromium/public/WebIDBObjectStore.h:61 + // we do with the data in Chromium (for now) is pass it through, it's not really worth the effort at the moment. hmm... it would be better to make the API have enums. in the future, the embedder might have need to interpret these values, and when that happens people often do not bother to go back and cleanup the API to expose a proper enum type. WebKit/chromium/src/WebIDBKey.cpp:39 + COMPILE_ASSERT(int(WebIDBKey::NullType) == int(IDBKey::NullType), dummyNullType); please move these to AssertMatchingEnums.cpp
(In reply to comment #12) > WebCore/storage/IDBObjectStoreImpl.cpp:77 > + callbacks->onError(IDBDatabaseError::create(IDBDatabaseException::UNKNOWN_ERR, "A key was supplied for an objectStore that has a keyPath.")); > it might be nice to define a helper method named notifyError that takes the callbacks, exception type, and string. It doesn't seem like that'd do more than save a few characters of typing (onError and IDBDatabaseError::) since the rest would still be required for the call. Given that it'd be adding a layer of indirection, I'm not sure this would actually be a win. > WebKit/chromium/public/WebIDBKey.h:42 > + WEBKIT_API static WebIDBKey createInvalid(); > nit: please add a new line after this line to help readability > > WebKit/chromium/public/WebIDBKey.h:62 > + /* Types not in WebCore::IDBKey */ > nit: use C++ style comment > > WebKit/chromium/public/WebIDBKey.h:67 > + WEBKIT_API WebString string() const; // Only valid for StringType. > nit: maybe these methods should be named toString and toNumber or asString and asNumber? Well, they're just getters so I'm not sure if either of those really make sense. > WebKit/chromium/public/WebIDBObjectStore.h:61 > + // we do with the data in Chromium (for now) is pass it through, it's not really worth the effort at the moment. > hmm... it would be better to make the API have enums. in the future, > the embedder might have need to interpret these values, and when that > happens people often do not bother to go back and cleanup the API to > expose a proper enum type. In the latest spec, there are only 2 states (add/addOrModify) so I've changed this to just be a bool. > WebKit/chromium/src/WebIDBKey.cpp:39 > + COMPILE_ASSERT(int(WebIDBKey::NullType) == int(IDBKey::NullType), dummyNullType); > please move these to AssertMatchingEnums.cpp
Created attachment 60208 [details] Patch
Adding another potential reviewer.
Comment on attachment 60208 [details] Patch r=me, with a few minor comments. please keep an eye on the bots after landing this patch. WebCore/storage/IDBObjectStoreRequest.idl:37 + [CallWith=ScriptExecutionContext] IDBRequest put(in SerializedScriptValue value, in [Optional] IDBKey key); are you missing the declaration for remove()? WebKit/chromium/public/WebIDBKey.h:45 + WebIDBKey(int32_t number) { assign(number); } int32_t --> unsigned long ? WebKit/chromium/src/WebIDBKey.cpp:31 + #include "IDBKey.h" move inside ENABLE(INDEXED_DB)
(In reply to comment #16) > (From update of attachment 60208 [details]) > r=me, with a few minor comments. please keep an eye on the bots after landing this patch. > > WebCore/storage/IDBObjectStoreRequest.idl:37 > + [CallWith=ScriptExecutionContext] IDBRequest put(in SerializedScriptValue value, in [Optional] IDBKey key); > are you missing the declaration for remove()? Oops! > WebKit/chromium/public/WebIDBKey.h:45 > + WebIDBKey(int32_t number) { assign(number); } > int32_t --> unsigned long ? int32 is what the bindings use
Committed r63064: <http://trac.webkit.org/changeset/63064>
http://trac.webkit.org/changeset/63064 might have broken Chromium Win Release