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

[bugzilla-daemon at webkit.org]

Kentaro Hara <haraken at chromium.org> has denied 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 176820: Patch
https://bugs.webkit.org/attachment.cgi?id=176820&action=review

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


The approach looks reasonable. I hope JSC experts also take a look.

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:130
> +	   if (currentValue.isString() && keyPathElements[i] == "length")
> +	       return jsNumber(currentValue.toString(exec)->length());
> +	   if (!currentValue.isObject())
> +	       return jsUndefined();

This behavior looks different from V8's one.

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:144
> +

Don't you need to set up a new scope here?

  APIEntryShim entryShim(exec);

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:147
> +    jsValue = getNthValueOnKeyPath(exec, jsValue, keyPathElements,
> +	   keyPathElements.size());

Nit: you don't need to wrap the line.

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:148
> +    if (jsValue.isUndefined())

This should be jsValue.isEmpty()?

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:204
> +    ExecState* exec = requestState->exec();
> +    APIEntryShim entryShim(exec);

You can use DOMRequestState::Scope.

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:208
> +    if (!jsValue.isObject())
> +	   return false;

Remove this.

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:211
> +    JSValue parent = ensureNthValueOnKeyPath(exec, asObject(jsValue),
> +	   keyPathElements, keyPathElements.size() - 1);

asObject(jsValue) => jsValue.

Nit: You don't need to wrap the line.

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:212
> +    if (parent.isUndefined())

This should be parent.isEmpty()?

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:216
> +    if (!set(exec, parent, keyPathElements.last(),
> +	   toJS(exec, jsCast<JSDOMGlobalObject*>(exec->lexicalGlobalObject()),
key.get())))

Nit: Don't wrap the line.

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

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:260
> +    JSC::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.

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

Nit: don't wrap the line.

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:268
> +    ExecState* exec = requestState->exec();
> +    APIEntryShim entryShim(exec);

You can use DOMRequestState::Scope.

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:278
> +    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.

> Source/WebCore/bindings/js/IDBBindingUtilities.cpp:280
> +    return ScriptValue(exec->globalData(),
> +	   toJS(exec, jsCast<JSDOMGlobalObject*>(exec->lexicalGlobalObject()),
key.get()));

Nit: don't wrap the line.

> Source/WebCore/bindings/js/JSIDBAnyCustom.cpp:-88
> -    case IDBAny::SerializedScriptValueType:
> -	   return idbAny->serializedScriptValue()->deserialize(exec,
globalObject);

Why can you remove this?

> Source/WebCore/bindings/js/JSIDBAnyCustom.cpp:88
> +    case IDBAny::StringType:
> +	   return jsStringWithCache(exec, idbAny->string());

Just to confirm: Is there any reason why you moved this code from above to
here?

> Source/WebCore/bindings/js/JSIDBOpenDBRequestCustom.cpp:48
> +void JSIDBOpenDBRequest::visitChildren(JSCell* cell, JSC::SlotVisitor&
visitor)
> +{
> +    JSIDBOpenDBRequest* jsRequest = jsCast<JSIDBOpenDBRequest*>(cell);
> +    ASSERT_GC_OBJECT_INHERITS(jsRequest, &s_info);
> +    COMPILE_ASSERT(StructureFlags & OverridesVisitChildren,
OverridesVisitChildrenWithoutSettingFlag);
> +    ASSERT(jsRequest->structure()->typeInfo().overridesVisitChildren());
> +    Base::visitChildren(jsRequest, visitor);
> +   
static_cast<IDBOpenDBRequest*>(jsRequest->impl())->visitJSEventListeners(visito
r);
> +}

Why do you need this? Event listeners are already cared by JS bindings, aren't
they?

> Source/WebCore/bindings/js/JSIDBVersionChangeRequestCustom.cpp:47
> +void JSIDBVersionChangeRequest::visitChildren(JSCell* cell,
JSC::SlotVisitor& visitor)
>  {
> -    // FIXME: Implement this.
> +    JSIDBVersionChangeRequest* jsRequest =
jsCast<JSIDBVersionChangeRequest*>(cell);
> +    ASSERT_GC_OBJECT_INHERITS(jsRequest, &s_info);
> +    COMPILE_ASSERT(StructureFlags & OverridesVisitChildren,
OverridesVisitChildrenWithoutSettingFlag);
> +    ASSERT(jsRequest->structure()->typeInfo().overridesVisitChildren());
> +    Base::visitChildren(jsRequest, visitor);
> +   
static_cast<IDBVersionChangeRequest*>(jsRequest->impl())->visitJSEventListeners
(visitor);
>  }

Ditto.