[webkit-reviews] review denied: [Bug 39490] Indexed Database component is missing IDBObjectStoreRequest interface : [Attachment 56718] Patch

[bugzilla-daemon at webkit.org]

Jeremy Orlow <jorlow at chromium.org> has denied Andrei Popescu
<andreip at google.com>'s request for review:
Bug 39490: Indexed Database component is missing IDBObjectStoreRequest
interface
https://bugs.webkit.org/show_bug.cgi?id=39490

Attachment 56718: Patch
https://bugs.webkit.org/attachment.cgi?id=56718&action=review

------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
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).