Hooks IDBKeyPath with IDBObjectStorage::put.

[Marcus Bulach]

Hooks IDBKeyPath with IDBObjectStorage::put.

[Marcus Bulach]

Created attachment 64873 [details]
Patch

[Jeremy Orlow]

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...

[Marcus Bulach]

Created attachment 64952 [details]
Patch

[Marcus Bulach]

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?

[Jeremy Orlow]

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

[Jeremy Orlow]

Andrei, can you please take this over and finish it?

[Marcus Bulach]

Created attachment 64961 [details]
Patch

[Andrei Popescu]

(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?

[Jeremy Orlow]

Comment on attachment 64961 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 64961 [details]
Patch

Clearing flags on attachment: 64961

Committed r65893: <http://trac.webkit.org/changeset/65893>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.