Bug 41250

Summary: Implement IDBObjectStore.get/set/remove
Product: WebKit Reporter: Jeremy Orlow <jorlow>
Component: New BugsAssignee: Jeremy Orlow <jorlow>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, andreip, bulach, dglazkov, dumi, eric, fishd, japhet, steveblock, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on: 41252    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch dumi: review+

Description Jeremy Orlow 2010-06-26 09:34:48 PDT
Implement memory based storage for IndexedDB
Comment 1 Jeremy Orlow 2010-06-26 09:46:23 PDT
Created attachment 59838 [details]
Patch
Comment 2 Jeremy Orlow 2010-06-26 09:53:47 PDT
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.
Comment 3 Jeremy Orlow 2010-06-26 10:15:08 PDT
Actually, I think it should make it substantially easier to review if split in two.
Comment 4 Jeremy Orlow 2010-06-26 10:38:36 PDT
Oops...forgot Darin.  But nothing to do here until the other is ready to land.
Comment 5 Jeremy Orlow 2010-06-27 06:02:02 PDT
Created attachment 59854 [details]
Patch
Comment 6 Jeremy Orlow 2010-06-27 06:03:18 PDT
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.
Comment 7 Jeremy Orlow 2010-06-27 06:03:43 PDT
Oh, and this depends on 41250.
Comment 8 Jeremy Orlow 2010-06-27 06:03:55 PDT
Er...52.
Comment 9 Jeremy Orlow 2010-06-28 22:57:49 PDT
Created attachment 59989 [details]
Patch
Comment 10 Andrei Popescu 2010-06-29 08:44:54 PDT
> 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...
Comment 11 Jeremy Orlow 2010-06-29 14:55:25 PDT
(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 12 Darin Fisher (:fishd, Google) 2010-06-29 16:43:29 PDT
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
Comment 13 Jeremy Orlow 2010-06-30 23:56:47 PDT
(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
Comment 14 Jeremy Orlow 2010-07-01 00:20:09 PDT
Created attachment 60208 [details]
Patch
Comment 15 Jeremy Orlow 2010-07-01 17:34:14 PDT
Adding another potential reviewer.
Comment 16 Dumitru Daniliuc 2010-07-02 02:24:56 PDT
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)
Comment 17 Jeremy Orlow 2010-07-05 07:37:09 PDT
(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
Comment 18 Jeremy Orlow 2010-07-12 03:01:06 PDT
Committed r63064: <http://trac.webkit.org/changeset/63064>
Comment 19 WebKit Review Bot 2010-07-12 04:28:09 PDT
http://trac.webkit.org/changeset/63064 might have broken Chromium Win Release