Summary: | Adds support for throwing behavior of SerializedScriptValue to be specified in the IDL. | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Marcus Bulach <bulach> | ||||||||||||||||||||
Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||
Status: | UNCONFIRMED --- | ||||||||||||||||||||||
Severity: | Normal | CC: | andreip, bulach, gustavo, jorlow, webkit.review.bot, xan.lopez | ||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | Other | ||||||||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||||||||
Attachments: |
|
Description
Marcus Bulach
2010-06-18 08:31:11 PDT
Created attachment 59113 [details]
Patch
Attachment 59113 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:177: Extra space before ( in function call [whitespace/parens] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:179: Extra space before ( in function call [whitespace/parens] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:180: _g_serialized_arg is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:181: Extra space before ( in function call [whitespace/parens] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:188: Extra space before ( in function call [whitespace/parens] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:191: g_res is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:204: Extra space before ( in function call [whitespace/parens] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:206: g_res is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
Total errors found: 8 in 18 files
If any of these errors are false positives, please file a bug against check-webkit-style.
jorlow: I took over your patch (thanks!), fixed a couple of nits, and WebKitTools/Scripts/run-bindings-tests seems to be happy: "All tests passed!". would you mind taking another look? also, please, let me know if there's any other test I should run, or anybody else that should be cc:d on this bug. thanks! Attachment 59113 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/3267314 Can you please write some layout tests to verify that we are doing the right thing now (and verify we weren't before)? checking PopStateEvent.init throws and that IDBObjectStore.put doesn't (and a FIXME to verify that it does throw an onError) is probably enough I took a glance and it looks good to me, but since I wrote a bunch of the original code, someone else should review this. Comment on attachment 59113 [details]
Patch
Removing from review queue until there are layout tests.
Created attachment 59274 [details]
Patch
Attachment 59274 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:181: converted_serialized_arg is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:192: g_res is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:207: g_res is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
Total errors found: 3 in 25 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #6) > (From update of attachment 59113 [details]) > Removing from review queue until there are layout tests. thanks Jeremy! added layout tests for both PopStateEvent and IDBObjectStore, another look please? feel free to add anyone else who should review this patch. Attachment 59274 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/3334542 Attachment 59274 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/3281563 Comment on attachment 59274 [details]
Patch
ops, wait a bit, need to fix the JS generator.. will upload a new patch shortly.
Created attachment 59362 [details]
Patch
fixed the JS generator, and added a TestPODObj.idl to exercise the binding generator for interfaces with [PODType=] attribute.. another look please? (In reply to comment #13) > Created an attachment (id=59362) [details] > Patch Attachment 59362 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
Last 3072 characters of output:
our identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:116: prop_id is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:122: Non-label code inside switch statements should be indented. [whitespace/indent] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:133: webkit_dom_test_pod_obj_get_property is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:133: prop_id is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:139: Non-label code inside switch statements should be indented. [whitespace/indent] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:155: webkit_dom_test_pod_obj_class_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:157: Declaration has space between type name and * in GObjectClass *gobjectClass [whitespace/declaration] [3]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:182: webkit_dom_test_pod_obj_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.h:27: Alphabetical sorting problem. [build/include_order] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.h:39: parent_instance is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.h:43: parent_class is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.h:47: Extra space before ( in function call [whitespace/parens] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:181: converted_serialized_arg is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:192: g_res is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:207: g_res is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObjPrivate.h:21: #ifndef header guard has wrong style, please use: WebKitDOMTestPODObjPrivate_h [build/header_guard] [5]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObjPrivate.h:26: Alphabetical sorting problem. [build/include_order] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObjPrivate.h:28: Code inside a namespace should not be indented. [whitespace/indent] [4]
Total errors found: 30 in 38 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 59362 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/3341605 Comment on attachment 59362 [details]
Patch
not there yet... :( let me fix the new issue.
Attachment 59362 [details] did not build on gtk: Build output: http://webkit-commit-queue.appspot.com/results/3282546 Created attachment 59405 [details]
Patch
ok, I managed to compile Qt and run the tests locally, let's hope it makes the bots just as happy... if everything (except the style of autogenerated code) is green, would you mind another look please? thanks! (In reply to comment #17) > (From update of attachment 59362 [details]) > not there yet... :( let me fix the new issue. Attachment 59405 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
Last 3072 characters of output:
our identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:116: prop_id is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:122: Non-label code inside switch statements should be indented. [whitespace/indent] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:133: webkit_dom_test_pod_obj_get_property is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:133: prop_id is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:139: Non-label code inside switch statements should be indented. [whitespace/indent] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:155: webkit_dom_test_pod_obj_class_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:157: Declaration has space between type name and * in GObjectClass *gobjectClass [whitespace/declaration] [3]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:182: webkit_dom_test_pod_obj_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.h:27: Alphabetical sorting problem. [build/include_order] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.h:39: parent_instance is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.h:43: parent_class is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.h:47: Extra space before ( in function call [whitespace/parens] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:181: converted_serialized_arg is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:192: g_res is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:207: g_res is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObjPrivate.h:21: #ifndef header guard has wrong style, please use: WebKitDOMTestPODObjPrivate_h [build/header_guard] [5]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObjPrivate.h:26: Alphabetical sorting problem. [build/include_order] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObjPrivate.h:28: Code inside a namespace should not be indented. [whitespace/indent] [4]
Total errors found: 30 in 40 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 59405 [details]
Patch
WebCore/bindings/js/SerializedScriptValue.cpp:987
+ PassRefPtr<SerializedScriptValue> serializedValue = new SerializedScriptValue(SerializedScriptValueData::serialize(exec, value));
RefPtr not PassRefPtr
WebCore/bindings/js/SerializedScriptValue.cpp:994
+ return serializedValue;
.release() here.
WebCore/bindings/js/SerializedScriptValue.cpp:981
+ PassRefPtr<SerializedScriptValue> serializedValue = SerializedScriptValue::create(exec, value, &ec);
Don't you need to set the dom exception here as well?
Also don't use PassRefPtr within a function.
WebCore/bindings/scripts/CodeGeneratorJS.pm:1719
+ push(@implContent, " podImp = " . JSValueToNative($attribute->signature, "value", 0) . ";\n");
Use booleans.
WebCore/bindings/scripts/CodeGeneratorJS.pm:1721
+ push(@implContent, " podImp.set$implSetterFunctionName(" . JSValueToNative($attribute->signature, "value", 0) . ");\n");
ditto (and elsewhere)
LayoutTests/storage/indexeddb/script-tests/idb-add-serialized-script-value.js:1
+ description("Test IndexedDB's openCursor.");
Update the description.
LayoutTests/storage/indexeddb/resources/shared.js:52
+ testFailed("Success function called unexpectedly: (" + event.code + ") " + event.message);
There will be no code/message.
LayoutTests/fast/loader/stateobjects/initpopstateevent-expected.txt:5
+ PASS serialized script value threw an exception due to a cycle.
Put a message in the beginning saying it'll print "COMPLETE" when done and then add code to do that.
Created attachment 59888 [details]
Patch
Attachment 59888 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
Last 3072 characters of output:
switch statements should be indented. [whitespace/indent] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:133: webkit_dom_test_pod_obj_get_property is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:133: prop_id is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:139: Non-label code inside switch statements should be indented. [whitespace/indent] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:155: webkit_dom_test_pod_obj_class_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:157: Declaration has space between type name and * in GObjectClass *gobjectClass [whitespace/declaration] [3]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:182: webkit_dom_test_pod_obj_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.h:27: Alphabetical sorting problem. [build/include_order] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.h:39: parent_instance is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.h:43: parent_class is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.h:47: Extra space before ( in function call [whitespace/parens] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:181: converted_serialized_arg is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:192: g_res is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:198: Extra space between WebKitDOMSerializedScriptValue* and res [whitespace/declaration] [3]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:207: g_res is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:208: Extra space between WebKitDOMSerializedScriptValue* and res [whitespace/declaration] [3]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObjPrivate.h:21: #ifndef header guard has wrong style, please use: WebKitDOMTestPODObjPrivate_h [build/header_guard] [5]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObjPrivate.h:26: Alphabetical sorting problem. [build/include_order] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObjPrivate.h:28: Code inside a namespace should not be indented. [whitespace/indent] [4]
Total errors found: 33 in 40 files
If any of these errors are false positives, please file a bug against check-webkit-style.
thanks jeremy! all comments addressed, plus one clarification below. another look please? (In reply to comment #22) > (From update of attachment 59405 [details]) > WebCore/bindings/js/SerializedScriptValue.cpp:987 > + PassRefPtr<SerializedScriptValue> serializedValue = new SerializedScriptValue(SerializedScriptValueData::serialize(exec, value)); > RefPtr not PassRefPtr Done. > > WebCore/bindings/js/SerializedScriptValue.cpp:994 > + return serializedValue; > .release() here. Done. > > WebCore/bindings/js/SerializedScriptValue.cpp:981 > + PassRefPtr<SerializedScriptValue> serializedValue = SerializedScriptValue::create(exec, value, &ec); > Don't you need to set the dom exception here as well? Not really, this is just calling the factory below (which sets dom exception). > Also don't use PassRefPtr within a function. Done. > > WebCore/bindings/scripts/CodeGeneratorJS.pm:1719 > + push(@implContent, " podImp = " . JSValueToNative($attribute->signature, "value", 0) . ";\n"); > Use booleans. Done. > > WebCore/bindings/scripts/CodeGeneratorJS.pm:1721 > + push(@implContent, " podImp.set$implSetterFunctionName(" . JSValueToNative($attribute->signature, "value", 0) . ");\n"); > ditto (and elsewhere) Done. > > LayoutTests/storage/indexeddb/script-tests/idb-add-serialized-script-value.js:1 > + description("Test IndexedDB's openCursor."); > Update the description. ouch, done. > > LayoutTests/storage/indexeddb/resources/shared.js:52 > + testFailed("Success function called unexpectedly: (" + event.code + ") " + event.message); > There will be no code/message. removed. > > LayoutTests/fast/loader/stateobjects/initpopstateevent-expected.txt:5 > + PASS serialized script value threw an exception due to a cycle. > Put a message in the beginning saying it'll print "COMPLETE" when done and then add code to do that. done. Comment on attachment 59888 [details]
Patch
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:18
+ var a = {};
Maybe make a function in the shared script for this? createUnserializableValue() ?
LayoutTests/storage/indexeddb/script-tests/idb-add-serialized-script-value.js:23
+ 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
Created attachment 60033 [details]
Patch
Attachment 60033 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
Last 3072 characters of output:
bject/WebKitDOMTestPODObj.cpp:145: Non-label code inside switch statements should be indented. [whitespace/indent] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:161: webkit_dom_test_pod_obj_constructed is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:168: webkit_dom_test_pod_obj_class_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:170: Declaration has space between type name and * in GObjectClass *gobjectClass [whitespace/declaration] [3]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:196: webkit_dom_test_pod_obj_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:211: Extra space between return and WEBKIT_DOM_TEST_POD_OBJ [whitespace/declaration] [3]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:212: Use 0 instead of NULL. [readability/null] [5]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.h:27: Alphabetical sorting problem. [build/include_order] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.h:39: parent_instance is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.h:43: parent_class is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.h:47: Extra space before ( in function call [whitespace/parens] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:192: converted_serialized_arg is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:204: g_res is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:210: Extra space between WebKitDOMSerializedScriptValue* and res [whitespace/declaration] [3]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:220: g_res is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:221: Extra space between WebKitDOMSerializedScriptValue* and res [whitespace/declaration] [3]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObjPrivate.h:21: #ifndef header guard has wrong style, please use: WebKitDOMTestPODObjPrivate_h [build/header_guard] [5]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObjPrivate.h:26: Alphabetical sorting problem. [build/include_order] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObjPrivate.h:28: Code inside a namespace should not be indented. [whitespace/indent] [4]
Total errors found: 36 in 40 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #26) > (From update of attachment 59888 [details]) > 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. :-) yeah, there was a FIXME further down where I'm short-circuiting this :) but good point, added another one here. > > LayoutTests/storage/indexeddb/script-tests/idb-add-serialized-script-value.js:18 > + var a = {}; > Maybe make a function in the shared script for this? createUnserializableValue() ? done. > > LayoutTests/storage/indexeddb/script-tests/idb-add-serialized-script-value.js:23 > + testFailed("objStore.add threw an unexpected exception: " + err); > And then notify done + return? done. > > WebCore/bindings/js/SerializedScriptValue.cpp:972 > + PassRefPtr<SerializedScriptValue> serializedValue = new SerializedScriptValue(SerializedScriptValueData::serialize(exec, value)); > This should be a RefPtr...later return serializedValue.release(); done. > > WebCore/bindings/js/SerializedScriptValue.cpp:990 > + } else { > No {}'s on the second half done. > > 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. sigh, I'll keep this in mind when copying from somewhere else next time.. thanks! > > WebCore/bindings/scripts/test/TestPODObj.idl:2 > + * Copyright (C) 2009 Google Inc. All rights reserved. > 2010 copy the whole license from somewhere else. > > WebCore/bindings/scripts/test/TestPODObj.idl:34 > + attribute [ConvertNullToNullString] DOMString stringAttr; > Not sure this indenting is necessary. removed. > > WebCore/bindings/scripts/test/TestObj.idl:65 > + // Test various forms of SerializedScriptValue > period at end. done. > > 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? no idea.. :( won't the setDOMException() below take care of it? > 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. fixed. getting imp->serializedValueReturn[Raises|NoRaise]() as a temp, and then testing it (plus ec, if available), and returning jsNull() instead. note that this code would call the function twice, but wasn't intrinsically wrong AFAICT, since the function that returns the serializedValue should be const anyway.. oh, the joys of perl. :) would please double check the generator bits as well? thanks! > > WebCore/bindings/scripts/test/JS/JSTestObj.cpp:842 > + JSC::JSValue result = imp->serializedValueReturnNoRaise() ? imp->serializedValueReturnNoRaise()->deserialize(exec, castedThis->globalObject()) : jsNull(); > ditto fixed as above.. (In reply to comment #29) > > 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? > no idea.. :( won't the setDOMException() below take care of it? Well, we don't want to run the webCore method unless we haven't hit an error at this point. See what Chromium does. (Not sure if this is what you did or not...won't have time to look until tonight.) Created attachment 60111 [details]
Patch
Attachment 60111 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
Last 3072 characters of output:
bject/WebKitDOMTestPODObj.cpp:145: Non-label code inside switch statements should be indented. [whitespace/indent] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:161: webkit_dom_test_pod_obj_constructed is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:168: webkit_dom_test_pod_obj_class_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:170: Declaration has space between type name and * in GObjectClass *gobjectClass [whitespace/declaration] [3]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:196: webkit_dom_test_pod_obj_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:211: Extra space between return and WEBKIT_DOM_TEST_POD_OBJ [whitespace/declaration] [3]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:212: Use 0 instead of NULL. [readability/null] [5]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.h:27: Alphabetical sorting problem. [build/include_order] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.h:39: parent_instance is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.h:43: parent_class is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.h:47: Extra space before ( in function call [whitespace/parens] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:192: converted_serialized_arg is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:204: g_res is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:210: Extra space between WebKitDOMSerializedScriptValue* and res [whitespace/declaration] [3]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:220: g_res is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:221: Extra space between WebKitDOMSerializedScriptValue* and res [whitespace/declaration] [3]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObjPrivate.h:21: #ifndef header guard has wrong style, please use: WebKitDOMTestPODObjPrivate_h [build/header_guard] [5]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObjPrivate.h:26: Alphabetical sorting problem. [build/include_order] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObjPrivate.h:28: Code inside a namespace should not be indented. [whitespace/indent] [4]
Total errors found: 36 in 40 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #30) > (In reply to comment #29) > > > 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? > > no idea.. :( won't the setDOMException() below take care of it? > > Well, we don't want to run the webCore method unless we haven't hit an error at this point. See what Chromium does. (Not sure if this is what you did or not...won't have time to look until tonight.) got it. the generated code now sets the exception code and returns immediately on exception, without calling the webcore method in the latest patch. Comment on attachment 60111 [details]
Patch
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.
Created attachment 60241 [details]
Patch
Attachment 60241 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
Last 3072 characters of output:
bject/WebKitDOMTestPODObj.cpp:145: Non-label code inside switch statements should be indented. [whitespace/indent] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:161: webkit_dom_test_pod_obj_constructed is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:168: webkit_dom_test_pod_obj_class_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:170: Declaration has space between type name and * in GObjectClass *gobjectClass [whitespace/declaration] [3]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:196: webkit_dom_test_pod_obj_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:211: Extra space between return and WEBKIT_DOM_TEST_POD_OBJ [whitespace/declaration] [3]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:212: Use 0 instead of NULL. [readability/null] [5]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.h:27: Alphabetical sorting problem. [build/include_order] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.h:39: parent_instance is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.h:43: parent_class is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.h:47: Extra space before ( in function call [whitespace/parens] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:192: converted_serialized_arg is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:204: g_res is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:210: Extra space between WebKitDOMSerializedScriptValue* and res [whitespace/declaration] [3]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:220: g_res is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:221: Extra space between WebKitDOMSerializedScriptValue* and res [whitespace/declaration] [3]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObjPrivate.h:21: #ifndef header guard has wrong style, please use: WebKitDOMTestPODObjPrivate_h [build/header_guard] [5]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObjPrivate.h:26: Alphabetical sorting problem. [build/include_order] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObjPrivate.h:28: Code inside a namespace should not be indented. [whitespace/indent] [4]
Total errors found: 36 in 40 files
If any of these errors are false positives, please file a bug against check-webkit-style.
thanks jeremy! another look please? (In reply to comment #34) > (From update of attachment 60111 [details]) > 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 done > > WebCore/bindings/js/SerializedScriptValue.h:183 > + static PassRefPtr<SerializedScriptValue> create(JSC::ExecState* exec, JSC::JSValue value, ExceptionCode* ec); > don't need ec done. > > 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"? right, testing for both SerializedScriptValue and 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? good point.. changed so that the caller tests it. > > WebCore/bindings/scripts/test/JS/JSTestObj.cpp:832 > + SerializedScriptValue* serializedScriptValue = imp->serializedValueReturnRaises(ec); > Wouldn't this need to be a PassRefPtr? done. > > Does anything even use this code yet? If not, then maybe we shouldn't test for it and/or worry about it. well, I think it's better to have it now rather than worry about it later? as in, as far as i can tell it's a valid case and we want to make sure the generator will "do the right thing"? > > 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. done. (In reply to comment #37) > > Does anything even use this code yet? If not, then maybe we shouldn't test for it and/or worry about it. > well, I think it's better to have it now rather than worry about it later? as in, as far as i can tell it's a valid case and we want to make sure the generator will "do the right thing"? For better or for worse, what the generator supports is generally built up on demand. This means that often when you add something to an IDL you have to add support into the generators. Will look at the patch tomorrow. Sorry for these long cycles, but it's midnight here and I'm pretty tired. Comment on attachment 60241 [details]
Patch
WebCore/bindings/scripts/test/JS/JSTestObj.cpp:833
+ JSC::JSValue result = ec ? jsUndefined() : serializedScriptValue ? serializedScriptValue->deserialized() : jsNull();
Personally, I think this is cleaner:
if (ec) {
setDOMException
return jsUndefined;
}
return serializedScriptVlaue->deserilized()
WebCore/bindings/scripts/CodeGeneratorJS.pm:2475
+ $ret = $ret . " serializedScriptValue ? serializedScriptValue->deserialized() : jsNull();\n";
you get double spaces between a few items here...not that big of a deal tho
r=me
Created attachment 62810 [details]
Patch
sorry about the delay, this one went under my radar. both comments addressed, another look please? (In reply to comment #39) > (From update of attachment 60241 [details]) > WebCore/bindings/scripts/test/JS/JSTestObj.cpp:833 > + JSC::JSValue result = ec ? jsUndefined() : serializedScriptValue ? serializedScriptValue->deserialized() : jsNull(); > Personally, I think this is cleaner: > > if (ec) { > setDOMException > return jsUndefined; > } > return serializedScriptVlaue->deserilized() > > WebCore/bindings/scripts/CodeGeneratorJS.pm:2475 > + $ret = $ret . " serializedScriptValue ? serializedScriptValue->deserialized() : jsNull();\n"; > you get double spaces between a few items here...not that big of a deal tho > > > > r=me Attachment 62810 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
Last 3072 characters of output:
bject/WebKitDOMTestPODObj.cpp:145: Non-label code inside switch statements should be indented. [whitespace/indent] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:161: webkit_dom_test_pod_obj_constructed is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:168: webkit_dom_test_pod_obj_class_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:170: Declaration has space between type name and * in GObjectClass *gobjectClass [whitespace/declaration] [3]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:196: webkit_dom_test_pod_obj_init is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:211: Extra space between return and WEBKIT_DOM_TEST_POD_OBJ [whitespace/declaration] [3]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.cpp:212: Use 0 instead of NULL. [readability/null] [5]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.h:27: Alphabetical sorting problem. [build/include_order] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.h:39: parent_instance is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.h:43: parent_class is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObj.h:47: Extra space before ( in function call [whitespace/parens] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:194: converted_serialized_arg is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:206: g_res is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:212: Extra space between WebKitDOMSerializedScriptValue* and res [whitespace/declaration] [3]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:222: g_res is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:223: Extra space between WebKitDOMSerializedScriptValue* and res [whitespace/declaration] [3]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObjPrivate.h:21: #ifndef header guard has wrong style, please use: WebKitDOMTestPODObjPrivate_h [build/header_guard] [5]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObjPrivate.h:26: Alphabetical sorting problem. [build/include_order] [4]
WebCore/bindings/scripts/test/GObject/WebKitDOMTestPODObjPrivate.h:28: Code inside a namespace should not be indented. [whitespace/indent] [4]
Total errors found: 36 in 40 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 62810 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=62810&action=review Looks reasonable. Some cleanup mentioned below. The only serious problem is what looks like a memory leak. > WebCore/bindings/js/SerializedScriptValue.cpp:977 > + RefPtr<SerializedScriptValue> serializedValue = new SerializedScriptValue(SerializedScriptValueData::serialize(exec, value)); Isn't this a memory leak? We need to call adoptRef, right? > WebCore/bindings/js/SerializedScriptValue.cpp:991 > + RefPtr<SerializedScriptValue> serializedValue = new SerializedScriptValue(SerializedScriptValueData::serialize(exec, value)); Same here. adoptRef? > WebCore/bindings/scripts/CodeGeneratorJS.pm:2491 > + $ret = $ret . " PassRefPtr<SerializedScriptValue> serializedScriptValue = $value;\n"; We don't usually store PassRefPtrs on the stack. We usually us RefPtr and then release. > WebCore/bindings/scripts/test/V8/V8TestObj.cpp:610 > + if (UNLIKELY(ec)) > + goto fail; That's pretty nasty code. Also, the indent is wrong here. > WebCore/bindings/scripts/test/V8/V8TestObj.cpp:637 > + { > + RefPtr<SerializedScriptValue> result = imp->serializedValueReturnRaises(ec); > + if (UNLIKELY(ec)) > + goto fail; > + return result.release()->deserialize(); > + } Another bad indent. Why do we need these { } brackets? > WebCore/bindings/v8/SerializedScriptValue.cpp:1031 > + didThrow = true; Why are we using didThrow instead of the ec, like we usually do? |