Summary: | Indexed Database component is missing IDBObjectStoreRequest interface | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andrei Popescu <andreip> | ||||||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | jorlow | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
URL: | http://dev.w3.org/2006/webapi/WebSimpleDB/ | ||||||||||||||
Attachments: |
|
Description
Andrei Popescu
2010-05-21 08:23:37 PDT
Created attachment 56716 [details]
Patch
Created attachment 56718 [details]
Patch
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).
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.) Created attachment 56902 [details]
Patch
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 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! 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)
(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 Created attachment 57028 [details]
Patch
(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 r=me Created attachment 57098 [details]
Patch
Ok, I rebased the patch against TOT, this should land w/o any conflicts. Also fixes the linking error introduced in the other patch. Committed r60240: <http://trac.webkit.org/changeset/60240> |