Bug 41250 – Implement IDBObjectStore.get/set/remove

• Component: New Bugs
• Status: RESOLVED FIXED

[Jeremy Orlow]

Implement memory based storage for IndexedDB

[Jeremy Orlow]

Created attachment 59838 [details]
Patch

[Jeremy Orlow]

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.

[Jeremy Orlow]

Actually, I think it should make it substantially easier to review if split in two.

[Jeremy Orlow]

Oops...forgot Darin.  But nothing to do here until the other is ready to land.

[Jeremy Orlow]

Created attachment 59854 [details]
Patch

[Jeremy Orlow]

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.

[Jeremy Orlow]

Oh, and this depends on 41250.

[Jeremy Orlow]

Er...52.

[Jeremy Orlow]

Created attachment 59989 [details]
Patch

[Andrei Popescu]

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

[Jeremy Orlow]

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

[Darin Fisher (:fishd, Google)]

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

[Jeremy Orlow]

(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

[Jeremy Orlow]

Created attachment 60208 [details]
Patch

[Jeremy Orlow]

Adding another potential reviewer.

[Dumitru Daniliuc]

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)

[Jeremy Orlow]

(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

[Jeremy Orlow]

Committed r63064: <http://trac.webkit.org/changeset/63064>

[WebKit Review Bot]

http://trac.webkit.org/changeset/63064 might have broken Chromium Win Release