[webkit-reviews] review denied: [Bug 85298] IndexedDB: Store key paths in IDBKeyPath type instead of String : [Attachment 139691] Patch

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has denied Joshua Bell
<jsbell at chromium.org>'s request for review:
Bug 85298: IndexedDB: Store key paths in IDBKeyPath type instead of String
https://bugs.webkit.org/show_bug.cgi?id=85298

Attachment 139691: Patch
https://bugs.webkit.org/attachment.cgi?id=139691&action=review

------- Additional Comments from Kentaro Hara <haraken at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=139691&action=review


> Source/WebCore/bindings/v8/Dictionary.cpp:427
> +#if ENABLE(INDEXED_DATABASE)

Let's remove #if. This is a general method and we can use the method in other
code.

> Source/WebCore/bindings/v8/Dictionary.cpp:439
> +	   v8::Local<v8::Value> indexedValue =
v8Array->Get(v8::Integer::New(i));

v8::Integer::New(i) => v8::Uint32::New(i)

> Source/WebCore/bindings/v8/Dictionary.h:86
> +#if ENABLE(INDEXED_DATABASE)

Let's remove #if.

> Source/WebCore/bindings/v8/custom/V8IDBAnyCustom.cpp:79
> +    case IDBAny::StringType:
> +	   return v8String(impl->string());

Shouldn't this be v8StringOrNull()? Previously [TreatReturnedNullStringAs=Null]
was specified in keyPath. If you want to keep the behavior, I guess that you
need to use v8StringOrNull() instead of v8String(). In any case, please add a
test case where keyPath returns a null string.