[webkit-reviews] review denied: [Bug 39490] Indexed Database component is missing IDBObjectStoreRequest interface : [Attachment 56718] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri May 21 10:30:43 PDT 2010
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).
More information about the webkit-reviews
mailing list