Summary: | Hooks IDBKeyPath with IDBObjectStorage::put. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Marcus Bulach <bulach> | ||||||||
Component: | New Bugs | Assignee: | Andrei Popescu <andreip> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | andreip, bulach, commit-queue, fishd, jorlow | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Other | ||||||||||
OS: | OS X 10.5 | ||||||||||
Attachments: |
|
Description
Marcus Bulach
2010-08-19 10:51:02 PDT
Created attachment 64873 [details]
Patch
Comment on attachment 64873 [details]
Patch
WebCore/bindings/v8/IDBBindingUtilities.h:42
+ void createIDBKeysFromSerializedValuesAndKeyPath(const Vector<PassRefPtr<SerializedScriptValue>, 0> values, const String& keyPath, Vector<PassRefPtr<IDBKey>, 0>& keys);
Why a Vector<, 0>?
WebCore/bindings/v8/IDBBindingUtilities.h:42
+ void createIDBKeysFromSerializedValuesAndKeyPath(const Vector<PassRefPtr<SerializedScriptValue>, 0> values, const String& keyPath, Vector<PassRefPtr<IDBKey>, 0>& keys);
remove the double space before the second const
WebCore/storage/IDBKeyPathBackendImpl.cpp:35
+ // FIXME: Implement this method once JSC support WireFormat for SerializedScriptValue.
supports (with an s)
WebCore/storage/IDBObjectStoreBackendImpl.cpp:83
+ RefPtr<SerializedScriptValue> originalValue = value;
Change value to prpValue and at the beginning of the function do RefPtr<> value = prpValue; Then you don't need any of this stuff.
WebCore/ChangeLog:5
+ Hooks IDBKeyPath with IDBObjectStorage::put.
This is not terribly descriptive. :-)
You forgot the URL
Please add more detail
WebKit/chromium/public/WebIDBKey.h:42
+ WebIDBKey() { }
Hmm...can we avoid this? The idea is that you'd use the proper create constructor depending on what you wanted to do.
WebCore/storage/IDBObjectStoreBackendImpl.h:38
+ struct IDBKeyPathElement;
Is this still needed?
Looks pretty good...
Created attachment 64952 [details]
Patch
thanks Jeremy! all comments addressed, another look please? (In reply to comment #2) > (From update of attachment 64873 [details]) > WebCore/bindings/v8/IDBBindingUtilities.h:42 > + void createIDBKeysFromSerializedValuesAndKeyPath(const Vector<PassRefPtr<SerializedScriptValue>, 0> values, const String& keyPath, Vector<PassRefPtr<IDBKey>, 0>& keys); > Why a Vector<, 0>? sorry, this file shouldn't be here, removed. (but to answer your question: Forward.h doesn't declare the default value for inlineCapacity, so we need to provide here, or include the real header). > > WebCore/bindings/v8/IDBBindingUtilities.h:42 > + void createIDBKeysFromSerializedValuesAndKeyPath(const Vector<PassRefPtr<SerializedScriptValue>, 0> values, const String& keyPath, Vector<PassRefPtr<IDBKey>, 0>& keys); > remove the double space before the second const file removed (but fixed somewhere else). > > WebCore/storage/IDBKeyPathBackendImpl.cpp:35 > + // FIXME: Implement this method once JSC support WireFormat for SerializedScriptValue. > supports (with an s) Done. > > WebCore/storage/IDBObjectStoreBackendImpl.cpp:83 > + RefPtr<SerializedScriptValue> originalValue = value; > Change value to prpValue and at the beginning of the function do RefPtr<> value = prpValue; Then you don't need any of this stuff. A-ha, I knew there was a simpler way. thanks! > > WebCore/ChangeLog:5 > + Hooks IDBKeyPath with IDBObjectStorage::put. > This is not terribly descriptive. :-) > You forgot the URL > Please add more detail I tried to add more details, let me know if it's any clearer. > > WebKit/chromium/public/WebIDBKey.h:42 > + WebIDBKey() { } > Hmm...can we avoid this? The idea is that you'd use the proper create constructor depending on what you wanted to do. WebVector :( it requires a public trivial constructor even for declaring an empty vector, not sure how to avoid this.. > > WebCore/storage/IDBObjectStoreBackendImpl.h:38 > + struct IDBKeyPathElement; > Is this still needed? removed. > > > Looks pretty good... thanks! another look please? Comment on attachment 64952 [details]
Patch
WebKit/chromium/src/ChromiumBridge.cpp:505
+ void ChromiumBridge::createIDBKeysFromSerializedValuesAndKeyPath(const Vector<PassRefPtr<SerializedScriptValue> >& values, const String& keyPath, Vector<PassRefPtr<IDBKey> >& keys)
RefPtr
WebCore/storage/IDBObjectStoreBackendImpl.cpp:139
+ Vector<PassRefPtr<SerializedScriptValue> > values;
s/PassRefPtr/RefPtr/
Otherwise LGTM
Andrei, can you please take this over and finish it? Created attachment 64961 [details]
Patch
(In reply to comment #6) > Andrei, can you please take this over and finish it? Looks like the latest patch is good to go, can you please have a look and r+ it if you're happy? Comment on attachment 64961 [details]
Patch
r=me
Comment on attachment 64961 [details] Patch Clearing flags on attachment: 64961 Committed r65893: <http://trac.webkit.org/changeset/65893> All reviewed patches have been landed. Closing bug. |