Bug 39490 – Indexed Database component is missing IDBObjectStoreRequest interface

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Andrei Popescu]

Indexed Database component is missing IDBObjectStoreRequest interface. Patch coming soon.

[Andrei Popescu]

Created attachment 56716 [details]
Patch

[Andrei Popescu]

Created attachment 56718 [details]
Patch

[Jeremy Orlow]

Comment on attachment 56718 [details]
Patch

WebCore/storage/IDBObjectStoreRequest.idl:43
 +          readonly attribute DOMString keyPath;
add a fixme for indexNames + check for it in a layout test that documents the failure (otherwise it's too easy for things like this to slip through the cracks)

WebCore/storage/IDBObjectStoreRequest.h:37
 +  #include <wtf/UnusedParam.h>
You aren't using this.

WebCore/storage/IDBObjectStoreRequest.h:59
 +      RefPtr<IDBObjectStore> m_store;
I'd name this m_objectStore.  (I had been naming stuff m_idbObjectStore, but I guess that's a bit excessive.)

WebCore/storage/IDBObjectStoreRequest.h:49
 +      ~IDBObjectStoreRequest() { }
I'd put a newline after this...and I think a lot of classes do

WebCore/storage/IDBObjectStoreRequest.cpp:49
 +  IDBObjectStoreRequest::IDBObjectStoreRequest(PassRefPtr<IDBObjectStore> idbStore) : m_store(idbStore)
Put  : m_objectStore(objectStore) on the next line

WebCore/storage/IDBObjectStoreRequest.h:54
 +      IDBObjectStoreRequest();
Why do you need this?

WebCore/storage/IDBObjectStoreRequest.cpp:45
 +  IDBObjectStoreRequest::IDBObjectStoreRequest()
I've been putting all of these first in the file.  I know it's out of order from what's in the .h, but it's kind of convention in a lot of the files (including the other IDB ones).

WebCore/storage/IDBObjectStoreRequest.h:57
 +      String m_name;
This data should be in the shared IDBObjectStore.h file and not here.  This class should only have the m_objectStore variable.

WebCore/storage/IDBObjectStoreRequest.cpp:35
 +  String IDBObjectStoreRequest::name() const
These could have been inline it seems...but either way, they should be in IDBObjectStore not ...Request.

WebCore/storage/IDBObjectStore.h:42
 +      // TODO: Implement.
FIXME

And you'll need to do it in this patch actually.

WebCore/storage/IDBObjectStoreRequest.idl:36
 +          [Custom] IDBRequest get(in any key);
Avoid custom functions at all cost.

This should work as is for JSC because it makes it a SerializedScriptValue.  As for V8, I thought there was some attribute you could add that hints it should be a serialized script value.  If not, you should add it.  Most changes to the code generator are easier than you might guess.

WebCore/bindings/js/JSIDBObjectStoreRequestCustom.cpp:44
 +  JSC::JSValue JSIDBObjectStoreRequest::remove(JSC::ExecState* exec, const JSC::ArgList& args)
None of this stuff should be necessary.
WebCore/ChangeLog:12
 +          * DerivedSources.cpp:
You forgot VIsual Studio.  ...and Android and CMake (which I forgot previously..but we should start updating).

[Jeremy Orlow]

Oh yeah!  And you need layout tests.  (Or at least to solemnly swear to add them as soon as it's possible to do so.)

[Andrei Popescu]

Created attachment 56902 [details]
Patch

[Andrei Popescu]

Thanks for the review Jeremy!

(In reply to comment #3)
> (From update of attachment 56718 [details])
> WebCore/storage/IDBObjectStoreRequest.idl:43
>  +          readonly attribute DOMString keyPath;
> add a fixme for indexNames + check for it in a layout test that documents the failure (otherwise it's too easy for things like this to slip through the cracks)
>

Added FIXME.

As for the layout tests, I would need to have IndexedDatabase::open() actually working and returning a valid IDBDatabase object. This is because object stores are created via IDBDatabase::createObjectStore(). Right now,  IndexedDatabase::open() just invokes the onerror handler. I know you are working on an implementation of this method, so I will wait until you check it in and then rebase this change. For now, I would be grateful if you could have a look at the rest of the code.

 
> WebCore/storage/IDBObjectStoreRequest.h:37
>  +  #include <wtf/UnusedParam.h>
> You aren't using this.
> 

Removed.

> WebCore/storage/IDBObjectStoreRequest.h:59
>  +      RefPtr<IDBObjectStore> m_store;
> I'd name this m_objectStore.  (I had been naming stuff m_idbObjectStore, but I guess that's a bit excessive.)
>

Done.
 
> WebCore/storage/IDBObjectStoreRequest.h:49
>  +      ~IDBObjectStoreRequest() { }
> I'd put a newline after this...and I think a lot of classes do
>

Done.
 
> WebCore/storage/IDBObjectStoreRequest.cpp:49
>  +  IDBObjectStoreRequest::IDBObjectStoreRequest(PassRefPtr<IDBObjectStore> idbStore) : m_store(idbStore)
> Put  : m_objectStore(objectStore) on the next line
> 

Done.

> WebCore/storage/IDBObjectStoreRequest.h:54
>  +      IDBObjectStoreRequest();
> Why do you need this?
>

I don't. Removed.
 
> WebCore/storage/IDBObjectStoreRequest.cpp:45
>  +  IDBObjectStoreRequest::IDBObjectStoreRequest()
> I've been putting all of these first in the file.  I know it's out of order from what's in the .h, but it's kind of convention in a lot of the files (including the other IDB ones).
>

Moved to the top.
 
> WebCore/storage/IDBObjectStoreRequest.h:57
>  +      String m_name;
> This data should be in the shared IDBObjectStore.h file and not here.  This class should only have the m_objectStore variable.
>

Moved.
 
> WebCore/storage/IDBObjectStoreRequest.cpp:35
>  +  String IDBObjectStoreRequest::name() const
> These could have been inline it seems...but either way, they should be in IDBObjectStore not ...Request.
>

Done, and they're inline.
 
> WebCore/storage/IDBObjectStore.h:42
>  +      // TODO: Implement.
> FIXME
> 

Fixed.

> 
> WebCore/storage/IDBObjectStoreRequest.idl:36
>  +          [Custom] IDBRequest get(in any key);
> Avoid custom functions at all cost.
>

Avoided.
 
> This should work as is for JSC because it makes it a SerializedScriptValue.  As for V8, I thought there was some attribute you could add that hints it should be a serialized script value.  If not, you should add it.  Most changes to the code generator are easier than you might guess.
>

Yep, used SerializedScriptValue and got rid of [Custom]. Thanks for the tip.
 
> WebCore/bindings/js/JSIDBObjectStoreRequestCustom.cpp:44
>  +  JSC::JSValue JSIDBObjectStoreRequest::remove(JSC::ExecState* exec, const JSC::ArgList& args)
> None of this stuff should be necessary.

Removed.

> WebCore/ChangeLog:12
>  +          * DerivedSources.cpp:
> You forgot VIsual Studio.  ...and Android and CMake (which I forgot previously..but we should start updating).

Added.


So my plan is to:

- Wait for your change to implement IndexedDatabase::open()
- Update my change and implement IDBDatabase::createObjectStore() and add layout tests.
- While I am waiting, I will look into adding a 'weak pointer' to the ScriptExecutionContext to be used by IDBRequest, as discussed.

How does this sound?

Thanks,
Andrei

[Jeremy Orlow]

Comment on attachment 56902 [details]
Patch

WebCore/storage/IDBAny.h:84
 +  
extra new line

WebCore/storage/IDBObjectStore.cpp:28
 +  #include "config.h"
new line between */ and this

WebCore/storage/IDBObjectStore.cpp:2
 +   * Copyright (C) 2010 Google Inc. All rights reserved.
For new files, lets use this: http://webkit.org/coding/bsd-license.html

WebCore/storage/IDBObjectStore.h:2
 +   * Copyright (C) 2010 Google Inc. All rights reserved.
ditto

WebCore/storage/IDBObjectStore.h:39
 +  // This class is shared by IDBObjectStoreRequest (async) and IDBObjectStoreSync (sync).
not sure if this is necessary.  We document this relationship for IndexedDatabase so, if anything, maybe we should point the reader in that direction?

WebCore/storage/IDBObjectStore.h:44
 +          return adoptRef(new IDBObjectStore);
It doesn't matter, but () seems prettier maybe?  (In some cases, () implies that it should be 0 initialized so it seems like a good habit?)

WebCore/storage/IDBObjectStore.h:46
 +      virtual ~IDBObjectStore() { }
Don't think this needs to be virtual.  (If you copied this from elsewhere, you can probably change it there too if you want.)

WebCore/storage/IDBObjectStoreRequest.cpp:2
 +   * Copyright (C) 2010 Google Inc. All rights reserved.
ditto...ditto

WebCore/storage/IDBObjectStoreRequest.cpp:41
 +      : m_objectStore(idbStore)
Nit, order the variables the same in the constructor and the class

WebCore/storage/IDBObjectStoreRequest.h:64
 +      PassRefPtr<IDBRequest> addOrModify(PassRefPtr<SerializedScriptValue> value, PassRefPtr<SerializedScriptValue> key);
Will it compile if you make "0" a default value for the key parameter?

WebCore/storage/IDBObjectStoreRequest.idl:37
 +          IDBRequest add(in SerializedScriptValue value, in [Optional] SerializedScriptValue key);
Oh, so optional does work now.  Cool!

[Jeremy Orlow]

Comment on attachment 56902 [details]
Patch

Your plan looks good.  In fact, now that I look at this, I guess it might make the most sense for you to put this in as is and then hook it up in a subsequent patch.  It's your choice to do so if you want.

r=me (if you fix the nits)

[Andrei Popescu]

(In reply to comment #8)
> (From update of attachment 56902 [details])
> Your plan looks good.  In fact, now that I look at this, I guess it might make the most sense for you to put this in as is and then hook it up in a subsequent patch.  It's your choice to do so if you want.
> 
> r=me (if you fix the nits)

Thanks, I will fix the nits and re-upload. Still not a committer yet so you will have to help me land this :)

Andrei

[Andrei Popescu]

Created attachment 57028 [details]
Patch

[Andrei Popescu]

(In reply to comment #7)
> (From update of attachment 56902 [details])
> WebCore/storage/IDBAny.h:84
>  +  
> extra new line
> 

Removed.

> WebCore/storage/IDBObjectStore.cpp:28
>  +  #include "config.h"
> new line between */ and this
>

Added.
 
> WebCore/storage/IDBObjectStore.cpp:2
>  +   * Copyright (C) 2010 Google Inc. All rights reserved.
> For new files, lets use this: http://webkit.org/coding/bsd-license.html
>

Hmm, there was a 3rd clause in my license, otherwise it was the same. Removed.

 
> WebCore/storage/IDBObjectStore.h:2
>  +   * Copyright (C) 2010 Google Inc. All rights reserved.
> ditto
> 
> WebCore/storage/IDBObjectStore.h:39
>  +  // This class is shared by IDBObjectStoreRequest (async) and IDBObjectStoreSync (sync).
> not sure if this is necessary.  We document this relationship for IndexedDatabase so, if anything, maybe we should point the reader in that direction?
>

I removed it, not sure the comment added any value.
 
> WebCore/storage/IDBObjectStore.h:44
>  +          return adoptRef(new IDBObjectStore);
> It doesn't matter, but () seems prettier maybe?  (In some cases, () implies that it should be 0 initialized so it seems like a good habit?)
>

Ok.
 
> WebCore/storage/IDBObjectStore.h:46
>  +      virtual ~IDBObjectStore() { }
> Don't think this needs to be virtual.  (If you copied this from elsewhere, you can probably change it there too if you want.)
> 
> WebCore/storage/IDBObjectStoreRequest.cpp:2
>  +   * Copyright (C) 2010 Google Inc. All rights reserved.
> ditto...ditto
> 
> WebCore/storage/IDBObjectStoreRequest.cpp:41
>  +      : m_objectStore(idbStore)
> Nit, order the variables the same in the constructor and the class
>


Hmm, but they are in order already...objectStore is first in the header. Or am I misunderstanding you?

 
> WebCore/storage/IDBObjectStoreRequest.h:64
>  +      PassRefPtr<IDBRequest> addOrModify(PassRefPtr<SerializedScriptValue> value, PassRefPtr<SerializedScriptValue> key);
> Will it compile if you make "0" a default value for the key parameter?
> 

Yes, done.

Thanks,
Andrei

[Jeremy Orlow]

r=me

[Andrei Popescu]

Created attachment 57098 [details]
Patch

[Andrei Popescu]

Ok, I rebased the patch against TOT, this should land w/o any conflicts. Also fixes the linking error introduced in the other patch.

[Jeremy Orlow]

Committed r60240: <http://trac.webkit.org/changeset/60240>