[webkit-reviews] review denied: [Bug 40835] Adds support for throwing behavior of SerializedScriptValue to be specified in the IDL. : [Attachment 59888] 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 59888: Patch
https://bugs.webkit.org/attachment.cgi?id=59888&action=review

------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
LayoutTests/storage/indexeddb/script-tests/idb-add-serialized-script-value.js:8

 +	verifyErrorEvent(event);
Assert that we're raising a serialization error?  Or...make a fixme to do this,
since it probably doesn't work right today.  :-)

LayoutTests/storage/indexeddb/script-tests/idb-add-serialized-script-value.js:1
8
 +	var a = {};
Maybe make a function in the shared script for this? 
createUnserializableValue()  ?

LayoutTests/storage/indexeddb/script-tests/idb-add-serialized-script-value.js:2
3
 +	    testFailed("objStore.add threw an unexpected exception: " + err);
And then notify done + return?

WebCore/bindings/js/SerializedScriptValue.cpp:972
 +	PassRefPtr<SerializedScriptValue> serializedValue = new
SerializedScriptValue(SerializedScriptValueData::serialize(exec, value));
This should be a RefPtr...later return serializedValue.release();

WebCore/bindings/js/SerializedScriptValue.cpp:990
 +	} else {
No {}'s on the second half

WebCore/bindings/scripts/test/TestPODObj.idl:13
 +   * 3.  Neither the name of Apple Computer, Inc. ("Apple") nor the names of
Just use a 2 clause license.

WebCore/bindings/scripts/test/TestPODObj.idl:2
 +   * Copyright (C) 2009 Google Inc. All rights reserved.
2010

WebCore/bindings/scripts/test/TestPODObj.idl:34
 +		     attribute [ConvertNullToNullString] DOMString stringAttr;
Not sure this indenting is necessary.

WebCore/bindings/scripts/test/TestObj.idl:65
 +	    // Test various forms of SerializedScriptValue
period at end.

WebCore/bindings/scripts/test/JS/JSTestObj.cpp:798
 +	RefPtr<SerializedScriptValue> serializedArg =
SerializedScriptValue::create(exec, exec->argument(0), &ec);
Hmm...do we need to generate bail out code for if ec is !0?

WebCore/bindings/scripts/test/JS/JSTestObj.cpp:828
 +	JSC::JSValue result = imp->serializedValueReturnRaises(ec) ?
imp->serializedValueReturnRaises(ec)->deserialize(exec,
castedThis->globalObject()) : jsNull();
Hu?  This seems very not right.  :-)
At least make sure the bindings generator kills itself rather than emit stuff
like this.

WebCore/bindings/scripts/test/JS/JSTestObj.cpp:842
 +	JSC::JSValue result = imp->serializedValueReturnNoRaise() ?
imp->serializedValueReturnNoRaise()->deserialize(exec,
castedThis->globalObject()) : jsNull();
ditto