[Webkit-unassigned] [Bug 103484] IndexedDB: Implement IndexedDB bindings for JSC

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=103484


Kentaro Hara <haraken at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #176820|review?                     |review-
               Flag|                            |




--- Comment #6 from Kentaro Hara <haraken at chromium.org>  2012-12-02 21:17:17 PST ---
(From update of attachment 176820)
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(visitor);
> +}

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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.