[webkit-reviews] review granted: [Bug 103484] IndexedDB: Implement IndexedDB bindings for JSC : [Attachment 177628] Patch

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has granted Michael Pruett
<michael at 68k.org>'s request for review:
Bug 103484: IndexedDB: Implement IndexedDB bindings for JSC
https://bugs.webkit.org/show_bug.cgi?id=103484

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

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


Looks OK. This will behave consistently with V8's one.

> > > Source/WebCore/bindings/js/IDBBindingUtilities.cpp:228
> > > +    ExecState* exec = requestState->exec();
> > > +    APIEntryShim entryShim(exec);
> > 
> > You can use DOMRequestState::Scope.
> > 
> > BTW, V8 binding isn't creating a scope of DOMRequestState::Scope, which
looks wrong.
>
> I've removed all APIEntryShim scopes from IDBBindingUtilities.cpp. The V8
implementation of  IDBBindingUtilities.cpp assumes that the proper
DOMRequestState scope has already been established  in the core IDB code, and
the same now holds true for the JSC implementation.

OK, then why does V8 binding need to create 'v8::HandleScope handleScope;' in
IDBBindingUtilities.cpp ? (e.g. createIDBKeyFromScriptValueAndKeyPath())

BTW, maybe I'm wrong but do we really need DOMRequestState? What we need is
just to keep a ScriptExecutionContext, isn't it?

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:87
> +	       unsigned length = array->length();

Nit: unsigned => size_t

The same comment for all unsigneds in this patch.

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:247
> +    return canInjectNthValueOnKeyPath(exec, scriptValue.jsValue(),
> +	   keyPathElements, keyPathElements.size() - 1);

Nit: Don't wrap the line.