[webkit-reviews] review denied: [Bug 40835] Adds support for throwing behavior of SerializedScriptValue to be specified in the IDL. : [Attachment 60111] Patch

[bugzilla-daemon at webkit.org]

Jeremy Orlow <jorlow at chromium.org> has denied Marcus Bulach
<bulach at chromium.org>'s request for review:
Bug 40835: Adds support for throwing behavior of SerializedScriptValue to be
specified in the IDL.
https://bugs.webkit.org/show_bug.cgi?id=40835

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

------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
WebCore/bindings/js/SerializedScriptValue.h:179
 +	    static PassRefPtr<SerializedScriptValue>
createWithoutThrowing(JSC::ExecState* exec, JSC::JSValue value);
don't need exec or value here...or below

WebCore/bindings/js/SerializedScriptValue.h:183
 +	    static PassRefPtr<SerializedScriptValue> create(JSC::ExecState*
exec, JSC::JSValue value, ExceptionCode* ec);
don't need ec

WebCore/bindings/scripts/CodeGeneratorJS.pm:2234
 +	    push(@implContent, "\n" .
NativeSerializedScriptValueToJSValue($function->signature, $functionString,
@{$function->raisesExceptions})) if $function->signature->type eq
"SerializedScriptValue";
what about any?  be sure to test for this if it's true that you should have had
"any"?

WebCore/bindings/scripts/CodeGeneratorJS.pm:2473
 +	if ($type eq "SerializedScriptValue" or $type eq "any") {
Do you actually need this?  Can't you assume it?

WebCore/bindings/scripts/test/JS/JSTestObj.cpp:832
 +	SerializedScriptValue* serializedScriptValue =
imp->serializedValueReturnRaises(ec);
Wouldn't this need to be a PassRefPtr?

Does anything even use this code yet?  If not, then maybe we shouldn't test for
it and/or worry about it.

WebCore/bindings/scripts/test/JS/JSTestObj.cpp:833
 +	JSC::JSValue result = serializedScriptValue && !ec ?
serializedScriptValue->deserialized() : jsNull();
if ec is set, we should return undefined.  Otherwise null.