[webkit-reviews] review denied: [Bug 96614] Need to clear exception in JSDictionary for operations that might have. : [Attachment 179158] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Michael Pruett <michael at 68k.org>'s
request for review:
Bug 96614: Need to clear exception in JSDictionary for operations that might
have.
https://bugs.webkit.org/show_bug.cgi?id=96614

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=179158&action=review


This is OK if we can’t fix the Dictionary class. But I think it would have been
OK to fix Dictionary too. It’s just that it was wrong to change JSDictionary.

review- because the bindings have multiple small mistakes in them

> Source/WebCore/bindings/js/JSIDBDatabaseCustom.cpp:52
> +    ExceptionCode ec = 0;

This should be moved down to the end of the function where it’s used rather
than declaring it way up here.

> Source/WebCore/bindings/js/JSIDBDatabaseCustom.cpp:54
> +    JSValue nameValue = exec->argument(0);
> +    String name = nameValue.toString(exec)->value(exec);

There's no reason to put nameValue into a local variable. This can all be done
on one line.

> Source/WebCore/bindings/js/JSIDBDatabaseCustom.cpp:58
> +    JSValue optionsValue = MAYBE_MISSING_PARAMETER(exec, 1,
DefaultIsUndefined);

This is overkill. The argument function already does this without requiring any
macro help. The way to write this is:

    JSValue optionsValue = exec->argument(1);

> Source/WebCore/bindings/js/JSIDBDatabaseCustom.cpp:60
> +    if (!optionsValue.isUndefinedOrNull() && !optionsValue.isObject())
> +	   return throwTypeError(exec, "Not an object.");

Is this type check really needed? Are there tests that cover this? Does the
specification ask for it? Normally in JavaScript you can try to get the
properties of anything, and there's no need to require that a JavaScript value
be an object just to get its properties.

> Source/WebCore/bindings/js/JSIDBDatabaseCustom.cpp:62
> +    JSDictionary options(exec, optionsValue.getObject());

It's not idiomatic to use JSDictionary here. Why not just use JavaScriptCore
functions directly on optionsValue?

> Source/WebCore/bindings/js/JSIDBDatabaseCustom.cpp:67
> +    if (options.isValid() && options.get("keyPath", keyPathScriptValue)) {
> +	   JSValue keyPathValue = keyPathScriptValue.jsValue();

This should just be:

    JSValue keyPathValue = optionsValue.get(exec, PropertyName("keyPath"));

Or something along those lines. JSDictionary does not add value here.

> Source/WebCore/bindings/js/JSIDBDatabaseCustom.cpp:68
> +	   if (isJSArray(keyPathValue))

It’s surprising that a JavaScript array behaves differently than a non-array
JavaScript object with the same properties as the array. That’s not a
conventional distinction made in JavaScript in most cases. I guess IDB is
unusual in that respect? Is that really what’s specified?

Do we have test cases that pass a non-array object that has array-like
properties to test this?

> Source/WebCore/bindings/js/JSIDBDatabaseCustom.cpp:82
> +    bool autoIncrement = false;
> +    if (options.isValid())
> +	   options.get("autoIncrement", autoIncrement);
> +
> +    if (exec->hadException())
> +	   return jsUndefined();

This should just be:

    JSValue autoIncrementValue = optionsValue.get(exec,
PropertyName("autoIncrement"));
    if (exec->hadException())
	return jsUndefined();

    bool autoIncrement = autoIncrementValue.toBoolean(exec);
    if (exec->hadException())
	return jsUndefined();

Again, JSDictionary does not help us.

> Source/WebCore/bindings/js/JSIDBDatabaseCustom.cpp:84
> +    JSValue result = toJS(exec, globalObject(),
WTF::getPtr(impl()->createObjectStore(name, keyPath, autoIncrement, ec)));

Please use get() instead of WTF::getPtr. WTF::getPtr just exists so the
generator can make code that works correctly without knowing if the return
value from createObjectStore is a PassRefPtr or a raw pointer.

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:51
> +    ExceptionCode ec = 0;

Same comment about moving the ec definition down to where it's used.

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:54
> +    ScriptExecutionContext* context =
jsCast<JSDOMGlobalObject*>(exec->lexicalGlobalObject())->scriptExecutionContext
();
> +    if (!context)
> +	   return jsUndefined();

We don’t want to throw an exception or log to the console in this case? Just
fail silently?

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:57
> +    JSValue nameValue = exec->argument(0);
> +    String name = nameValue.toString(exec)->value(exec);

There's no reason to put nameValue into a local variable. This can all be done
on one line.

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:62
> +    IDBKeyPath keyPath;

Conversion of a JSValue into an IDBKeyPath is complex enough that I’d like
these two classes to share a helper function that does it.

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:63
> +    if (keyPathValue.isObject() && isJSArray(keyPathValue))

The isObject() check here is not needed or helpful. The isJSArray function will
return false for any non-object value.

It’s surprising that a JavaScript array behaves differently than a non-array
JavaScript object with the same properties as the array. That’s not a
conventional distinction made in JavaScript in most cases. I guess IDB is
unusual in that respect? Is that really what’s specified?

Do we have test cases that pass a non-array object that has array-like
properties to test this?

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:70
> +    JSValue optionsValue = MAYBE_MISSING_PARAMETER(exec, 2,
DefaultIsUndefined);

As in a similar case above, this should just be:

    JSValue optionsValue = exec->argument(2);

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:72
> +    if (!optionsValue.isUndefinedOrNull() && !optionsValue.isObject())
> +	   return throwTypeError(exec, "Not an object.");

Is this type check really needed? Are there tests that cover this? Does the
specification ask for it? Normally in JavaScript you can try to get the
properties of anything, and there's no need to require that a JavaScript value
be an object just to get its properties.

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:74
> +    JSDictionary options(exec, optionsValue.getObject());

No need to use JSDictionary here.

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:78
> +    bool unique = false;
> +    if (options.isValid())
> +	   options.get("unique", unique);

This should just be:

    JSValue uniqueValue = unique.get(exec, PropertyName("unique"));
    if (exec->hadException())
	return jsUndefined();

    bool unique = uniqueValue.toBoolean(exec);
    if (exec->hadException())
	return jsUndefined();

JSDictionary does not help us.

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:85
> +    bool multiEntry = false;
> +    if (options.isValid())
> +	   options.get("multiEntry", multiEntry);
> +
> +    if (exec->hadException())
> +	   return jsUndefined();

This should just be:

    JSValue multiEntryValue = multiEntry.get(exec, PropertyName("multiEntry"));

    if (exec->hadException())
	return jsUndefined();

    bool multiEntry = multiEntryValue.toBoolean(exec);
    if (exec->hadException())
	return jsUndefined();

JSDictionary does not help us.

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:87
> +    JSValue result = toJS(exec, globalObject(),
WTF::getPtr(impl()->createIndex(context, name, keyPath, unique, multiEntry,
ec)));

Please use get() instead of WTF::getPtr. WTF::getPtr just exists so the
generator can make code that works correctly without knowing if the return
value from createObjectStore is a PassRefPtr or a raw pointer.