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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted 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 179326: Patch
https://bugs.webkit.org/attachment.cgi?id=179326&action=review

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


> Source/WebCore/bindings/js/JSIDBDatabaseCustom.cpp:54
> +    if (exec->argumentCount() < 1)
> +	   return throwError(exec, createNotEnoughArgumentsError(exec));
> +    String name = exec->argument(0).toString(exec)->value(exec);
> +    if (exec->hadException())
> +	   return jsUndefined();

I suggest breaking this into two paragraphs. The argument count check seems
separate from extracting the first argument.

> Source/WebCore/bindings/js/JSIDBDatabaseCustom.cpp:62
> +    if (optionsValue.isObject()) {

Why do we have this check? I believe we’ll get the same behavior without it
since the get calls will return undefined values, and I would prefer that we
omit it to streamline the code, and move the definitions of the variables
closer to the code that sets them up.

> Source/WebCore/bindings/js/JSIDBDatabaseCustom.cpp:71
> +	   if (!keyPathValue.isUndefinedOrNull())
> +	       keyPath = idbKeyPathFromValue(exec, keyPathValue);
> +
> +	   if (exec->hadException())
> +	       return jsUndefined();

I suggest nesting the second if statement inside the first to make it clear the
relationship between the function that might have created an exception and the
need for the if statement for the early return.

> Source/WebCore/bindings/js/JSIDBDatabaseCustom.cpp:74
> +	   JSValue autoIncrementValue = optionsValue.get(exec, Identifier(exec,
"autoIncrement"));
> +	   autoIncrement = autoIncrementValue.toBoolean(exec);

This would read better without the autoIncrementValue local variable.

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:60
> +    IDBKeyPath keyPath = idbKeyPathFromValue(exec, exec->argument(1));

This will convert undefined to the string "undefined" and null to the string
"null". I assume that’s the behavior we want.

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:70
> +    if (optionsValue.isObject()) {

Why do we have this check? I believe we’ll get the same behavior without it
since the get calls will return undefined values, and I would prefer that we
omit it to streamline the code.

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:72
> +	   JSValue uniqueValue = optionsValue.get(exec, Identifier(exec,
"unique"));
> +	   unique = uniqueValue.toBoolean(exec);

This would read better without the uniqueValue local variable.

> Source/WebCore/bindings/js/JSIDBObjectStoreCustom.cpp:77
> +	   JSValue multiEntryValue = optionsValue.get(exec, Identifier(exec,
"multiEntry"));
> +	   multiEntry = multiEntryValue.toBoolean(exec);

This would read better without the multiEntryValue local variable.