[Webkit-unassigned] [Bug 39490] Indexed Database component is missing IDBObjectStoreRequest interface

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=39490





--- Comment #6 from Andrei Popescu <andreip at google.com>  2010-05-24 10:55:51 PST ---
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.