[webkit-reviews] review denied: [Bug 45110] Implement JSC features required for IndexedDB : [Attachment 120549] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Donggwan Kim
<donggwan.kim at samsung.com>'s request for review:
Bug 45110: Implement JSC features required for IndexedDB
https://bugs.webkit.org/show_bug.cgi?id=45110

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

------- Additional Comments from Sam Weinig <sam at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=120549&action=review


This of course needs a changelog.

> Source/WebCore/bindings/js/OptionsObject.h:58
> +class OptionsObject {
> +public:
> +    OptionsObject();
> +    OptionsObject(JSC::ExecState*, const JSC::JSValue& options);
> +
> +    OptionsObject& operator=(const OptionsObject&);
> +
> +    bool get(const String&, bool&) const;
> +    bool get(const String&, int32_t&) const;
> +    bool get(const String&, double&) const;
> +    bool get(const String&, String&) const;
> +    bool get(const String&, unsigned short&) const;
> +    bool get(const String&, unsigned&) const;
> +    bool get(const String&, unsigned long long&) const;
> +
> +    bool getWithUndefinedOrNullCheck(const String&, String&) const;
> +
> +    ~OptionsObject();
> +
> +private:
> +    bool getKey(const String& key, JSC::JSValue&) const;
> +
> +    JSC::ExecState* m_exec;
> +    JSC::JSValue m_options;
> +};

The JSC bindings have a class called JSDictionary which has much of the same
functionality has this, I would rather we didn't duplicate this code.

> Source/WebCore/bindings/js/SerializedScriptValue.h:77
>      String toString();
> +    String toWireString();

It would probably make sense to add a comment explaining what the differences
between toString and toWireString are.

>> Source/WebCore/storage/IDBKeyPathBackendImpl.cpp:51
>> +#if USE(JSC)
> 
> WebCore proper shouldn't have any JSC specific code.	All the JSC specific
code should be in the bindings.

Indeed, why does JSC need special cased code here?

> Source/WebCore/storage/IDBTransactionBackendImpl.h:98
> +    ScriptExecutionContext* m_scriptExecutionContext;

What ensures the lifetime of the ScriptExecutionContext here?