Bug 40835

Summary: Adds support for throwing behavior of SerializedScriptValue to be specified in the IDL.
Product: WebKit Reporter: Marcus Bulach <bulach>
Component: New BugsAssignee: 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 Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch abarth: review-

Description Marcus Bulach 2010-06-18 08:31:11 PDT
Adds support for throwing behavior of SerializedScriptValue to be specified in the IDL.
Comment 1 Marcus Bulach 2010-06-18 08:36:24 PDT
Created attachment 59113 [details]
Patch
Comment 2 WebKit Review Bot 2010-06-18 08:38:53 PDT
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.
Comment 3 Marcus Bulach 2010-06-18 08:39:51 PDT
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!
Comment 4 Early Warning System Bot 2010-06-18 08:56:54 PDT
Attachment 59113 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3267314
Comment 5 Jeremy Orlow 2010-06-18 11:19:10 PDT
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 6 Jeremy Orlow 2010-06-18 11:22:04 PDT
Comment on attachment 59113 [details]
Patch

Removing from review queue until there are layout tests.
Comment 7 Marcus Bulach 2010-06-21 12:50:41 PDT
Created attachment 59274 [details]
Patch
Comment 8 WebKit Review Bot 2010-06-21 12:54:35 PDT
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.
Comment 9 Marcus Bulach 2010-06-21 12:56:27 PDT
(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.
Comment 10 Early Warning System Bot 2010-06-21 12:59:45 PDT
Attachment 59274 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3334542
Comment 11 WebKit Review Bot 2010-06-21 16:44:12 PDT
Attachment 59274 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3281563
Comment 12 Marcus Bulach 2010-06-22 02:32:31 PDT
Comment on attachment 59274 [details]
Patch

ops, wait a bit, need to fix the JS generator.. will upload a new patch shortly.
Comment 13 Marcus Bulach 2010-06-22 05:46:01 PDT
Created attachment 59362 [details]
Patch
Comment 14 Marcus Bulach 2010-06-22 05:48:37 PDT
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
Comment 15 WebKit Review Bot 2010-06-22 05:50:10 PDT
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.
Comment 16 Early Warning System Bot 2010-06-22 06:04:53 PDT
Attachment 59362 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3341605
Comment 17 Marcus Bulach 2010-06-22 06:07:15 PDT
Comment on attachment 59362 [details]
Patch

not there yet... :( let me fix the new issue.
Comment 18 WebKit Review Bot 2010-06-22 06:47:03 PDT
Attachment 59362 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/3282546
Comment 19 Marcus Bulach 2010-06-22 12:38:23 PDT
Created attachment 59405 [details]
Patch
Comment 20 Marcus Bulach 2010-06-22 12:40:17 PDT
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.
Comment 21 WebKit Review Bot 2010-06-22 12:43:55 PDT
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 22 Jeremy Orlow 2010-06-24 07:48:02 PDT
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.
Comment 23 Marcus Bulach 2010-06-28 03:24:31 PDT
Created attachment 59888 [details]
Patch
Comment 24 WebKit Review Bot 2010-06-28 03:26:46 PDT
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.
Comment 25 Marcus Bulach 2010-06-28 03:28:23 PDT
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 26 Jeremy Orlow 2010-06-28 22:19:38 PDT
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
Comment 27 Marcus Bulach 2010-06-29 10:17:05 PDT
Created attachment 60033 [details]
Patch
Comment 28 WebKit Review Bot 2010-06-29 10:20:22 PDT
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.
Comment 29 Marcus Bulach 2010-06-29 10:29:59 PDT
(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..
Comment 30 Jeremy Orlow 2010-06-29 14:45:01 PDT
(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.)
Comment 31 Marcus Bulach 2010-06-30 05:32:54 PDT
Created attachment 60111 [details]
Patch
Comment 32 WebKit Review Bot 2010-06-30 05:34:19 PDT
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.
Comment 33 Marcus Bulach 2010-06-30 05:37:00 PDT
(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 34 Jeremy Orlow 2010-07-01 00:40:05 PDT
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.
Comment 35 Marcus Bulach 2010-07-01 06:56:10 PDT
Created attachment 60241 [details]
Patch
Comment 36 WebKit Review Bot 2010-07-01 06:58:04 PDT
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.
Comment 37 Marcus Bulach 2010-07-01 07:01:53 PDT
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.
Comment 38 Jeremy Orlow 2010-07-01 07:14:38 PDT
(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 39 Jeremy Orlow 2010-07-02 00:33:12 PDT
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
Comment 40 Marcus Bulach 2010-07-28 04:29:47 PDT
Created attachment 62810 [details]
Patch
Comment 41 Marcus Bulach 2010-07-28 04:31:02 PDT
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
Comment 42 WebKit Review Bot 2010-07-28 04:37:46 PDT
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 43 Adam Barth 2011-01-12 14:19:50 PST
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?