Bug 54457 – IndexedDB: Inject auto increment keys via key path on insertion

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Hans Wennborg]

IndexedDB: Inject auto increment keys via key path on insertion

[Hans Wennborg]

Created attachment 82448 [details]
Patch

[Hans Wennborg]

Please review.

[Jeremy Orlow]

Comment on attachment 82448 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82448&action=review

> Source/WebCore/bindings/v8/IDBBindingUtilities.cpp:40
> +v8::Handle<v8::Value> toV8(IDBKey*);

Instead, include V8IDBKey.h

> Source/WebCore/bindings/v8/IDBBindingUtilities.cpp:56
> +namespace {

do you need this?  shouldn't using static and/or template be enough?

> Source/WebCore/bindings/v8/IDBBindingUtilities.cpp:69
> +bool setValue(T indexOrName, v8::Handle<v8::Value>& v8Object, v8::Handle<v8::Value>& v8Value)

seems like the object should come first

> Source/WebCore/bindings/v8/IDBBindingUtilities.cpp:74
> +        return false; // Never overwrite.

More to the point, we simply shouldn't have been called if something was there...right?

> Source/WebCore/bindings/v8/IDBBindingUtilities.cpp:98
> +v8::Handle<v8::Value> getNthValueOnKeyPath(v8::Handle<v8::Value>& v8Value, const Vector<IDBKeyPathElement>& keyPath, size_t n)

variable names aren't great.  Maybe rootValue, keyPathElements, and something else for n

> Source/WebCore/bindings/v8/IDBBindingUtilities.cpp:123
> +PassRefPtr<IDBKey> createIDBKeyFromSerializedValueAndKeyPath(PassRefPtr<SerializedScriptValue> value, const Vector<IDBKeyPathElement>& keyPath)

Could you take a bit of time to see if it's possible to make the code in this file a bit more clear?  I don't have any specific ideas and it's likely you can't do much better, but this is a pretty intense chunk of code.

> Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:205
> +PassRefPtr<IDBKey> IDBObjectStoreBackendImpl::selectKeyForPut(IDBObjectStoreBackendImpl* objectStore, RefPtr<SerializedScriptValue>& value, IDBKey* key, PutMode putMode, IDBCallbacks* callbacks)

in/out params should be last

> Source/WebKit/chromium/public/WebKitClient.h:39
> +#include "WebSerializedScriptValue.h"

remove the forward declare below

> Source/WebKit/chromium/src/PlatformBridge.cpp:552
> +    return webKitClient()->injectIDBKeyIntoSerializedValue(WebIDBKey(key), WebSerializedScriptValue(value), WebString(keyPath));

none of these casts should be necessary, i thought

[Hans Wennborg]

(In reply to comment #3)
> > Source/WebCore/bindings/v8/IDBBindingUtilities.cpp:40
> > +v8::Handle<v8::Value> toV8(IDBKey*);
> 
> Instead, include V8IDBKey.h
Done.

> > Source/WebCore/bindings/v8/IDBBindingUtilities.cpp:56
> > +namespace {
> 
> do you need this?  shouldn't using static and/or template be enough?
But I'd like to give LocalContext internal linkage too, and I can't do that with static. Not that it matters much.

> > Source/WebCore/bindings/v8/IDBBindingUtilities.cpp:69
> > +bool setValue(T indexOrName, v8::Handle<v8::Value>& v8Object, v8::Handle<v8::Value>& v8Value)
> 
> seems like the object should come first
Done.

> > Source/WebCore/bindings/v8/IDBBindingUtilities.cpp:74
> > +        return false; // Never overwrite.
> 
> More to the point, we simply shouldn't have been called if something was there...right?
Right. Asserting not reached instead.

> > Source/WebCore/bindings/v8/IDBBindingUtilities.cpp:98
> > +v8::Handle<v8::Value> getNthValueOnKeyPath(v8::Handle<v8::Value>& v8Value, const Vector<IDBKeyPathElement>& keyPath, size_t n)
> 
> variable names aren't great.  Maybe rootValue, keyPathElements, and something else for n
Done.

> > Source/WebCore/bindings/v8/IDBBindingUtilities.cpp:123
> > +PassRefPtr<IDBKey> createIDBKeyFromSerializedValueAndKeyPath(PassRefPtr<SerializedScriptValue> value, const Vector<IDBKeyPathElement>& keyPath)
> 
> Could you take a bit of time to see if it's possible to make the code in this file a bit more clear?  I don't have any specific ideas and it's likely you can't do much better, but this is a pretty intense chunk of code.
Done. Not sure if I made it better or worse :) Extracted some code into separate functions.

> > Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:205
> > +PassRefPtr<IDBKey> IDBObjectStoreBackendImpl::selectKeyForPut(IDBObjectStoreBackendImpl* objectStore, RefPtr<SerializedScriptValue>& value, IDBKey* key, PutMode putMode, IDBCallbacks* callbacks)
> 
> in/out params should be last
Done.

> > Source/WebKit/chromium/public/WebKitClient.h:39
> > +#include "WebSerializedScriptValue.h"
> 
> remove the forward declare below
Done.

> > Source/WebKit/chromium/src/PlatformBridge.cpp:552
> > +    return webKitClient()->injectIDBKeyIntoSerializedValue(WebIDBKey(key), WebSerializedScriptValue(value), WebString(keyPath));
> 
> none of these casts should be necessary, i thought
Ah, you're right. Removing same cast from the function above, that's where I copied the pattern :)

[Hans Wennborg]

Created attachment 82620 [details]
Patch

[Jeremy Orlow]

Comment on attachment 82620 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82620&action=review

r=me

> Source/WebCore/bindings/v8/IDBBindingUtilities.cpp:71
> +    if (object->Has(indexOrName))

Simply assert?

> Source/WebCore/bindings/v8/IDBBindingUtilities.cpp:89
> +bool set(v8::Handle<v8::Value>& object, const IDBKeyPathElement& keyPathElement, const v8::Handle<v8::Value>& v8Value)

make the param names and ordering consistent between these two?

[Hans Wennborg]

Committed r78721: <http://trac.webkit.org/changeset/78721>