Bug 81127

Created attachment 131871 [details] Patch

This is the codegen I think we want to support transfer lists. This will hopefully be useful for postMessage as well, where that appears. Please tell me where I can do better with the perl! It's been a while, and I fully believe there are better ways to do some of what I want. You'll notice the test files are a bit messy. I made another change which just changes the test IDL and results, without adding implementation, and diffed against that, this is a lot more focused. Let me know if you'd like that checked in separately to make this easier to read.

Comment on attachment 131871 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131871&action=review I think this generally goes in the right direction. Some comments below. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1521 > + # I think there's a better way to do this, but my perl fu is weak. :-( My perl-fu is weak too but this code has one advantage for me: I can understand it :) > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1599 > + next; # weak sauce! do we have runtime errors? IDL error: TransferList refs a nonexistent param Hmm IDLParser.pm suggests you go die("Error message") > Source/WebCore/bindings/scripts/test/JS/JSTestSerializedScriptValueInterface.cpp:285 > + RefPtr<SerializedScriptValue> data(SerializedScriptValue::create(exec, MAYBE_MISSING_PARAMETER(exec, 0, DefaultIsUndefined))); No transfer list for JSC yet? Either implement or file a separate bug to get it implemented. > Source/WebCore/bindings/scripts/test/TestSerializedScriptValueInterface.idl:32 > + Constructor(in DOMString hello, in [TransferList=transferList] SerializedScriptValue data, in [Optional] Array transferList), Test for multiple SSV+transferList pairs? I know we do not have a use case for such yet but if we implement the attribute we should better get it right.

Comment on attachment 131871 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131871&action=review This looks good to me, but Kentaro might have some thoughts. >> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1599 >> + next; # weak sauce! do we have runtime errors? IDL error: TransferList refs a nonexistent param > > Hmm IDLParser.pm suggests you go > die("Error message") Yeah, die is appropriate here. > Source/WebCore/bindings/scripts/IDLAttributes.txt:100 > +TransferList=* Please consider adding some documentation about how to use this feature to https://trac.webkit.org/wiki/WebKitIDL after this patch lands.

Attachment 131871 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestSerializedScriptValueInterface.cpp:68: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestSerializedScriptValueInterface.cpp:73: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/bindings/scripts/test/V8/V8TestSerializedScriptValueInterface.cpp:177: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 3 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 131871 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131871&action=review Looks good to me, except for several Nits I and other reviewers pointed out. >> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1521 >> + # I think there's a better way to do this, but my perl fu is weak. :-( > > My perl-fu is weak too but this code has one advantage for me: I can understand it :) Yeah, this code is readable. This is good. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1547 > + my @allParameterNames = (); > + foreach my $parameter (@{$function->parameters}) { > + push(@allParameterNames, $parameter->name); > + } Nit: Shall we move this code into the 'if ($parameter->extendedAttributes->{"TransferList"})' block? > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1597 > + $paramTransferListIndex = GetIndexOf($paramTransferListName, @allParameterNames); Nit FYI: Perl-er would write '$paramTransferListIndex = grep { $allParameterNames[$_] eq $paramTransferListName } 0..$#allParameterNames'. But GetIndexOf() is better for readability. >> Source/WebCore/bindings/scripts/test/JS/JSTestSerializedScriptValueInterface.cpp:285 >> + RefPtr<SerializedScriptValue> data(SerializedScriptValue::create(exec, MAYBE_MISSING_PARAMETER(exec, 0, DefaultIsUndefined))); > > No transfer list for JSC yet? > Either implement or file a separate bug to get it implemented. Same comment.

Comment on attachment 131871 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131871&action=review >> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1547 >> + } > > Nit: Shall we move this code into the 'if ($parameter->extendedAttributes->{"TransferList"})' block? I was just passing in $function->parameters to the GetIndexOf function, but it wasn't working at all. I still don't understand why. Maybe it was still while I was fighting the array parameter... I agree this is kind of ugly sitting here, and I would like to believe there's a neat way to do this all at once. Something like python filters? >> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1597 >> + $paramTransferListIndex = GetIndexOf($paramTransferListName, @allParameterNames); > > Nit FYI: Perl-er would write '$paramTransferListIndex = grep { $allParameterNames[$_] eq $paramTransferListName } 0..$#allParameterNames'. But GetIndexOf() is better for readability. Would that combine nicely with @($function->parameters)? I have forgotten much of the perl I once knew, so it's like I can almost remember that this should be easy. :-/ >> Source/WebCore/bindings/scripts/IDLAttributes.txt:100 >> +TransferList=* > > Please consider adding some documentation about how to use this feature to https://trac.webkit.org/wiki/WebKitIDL after this patch lands. Definitely -- it's on my list. Thanks for noting that here in the bug thread, though. >> Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestSerializedScriptValueInterface.cpp:68 >> + if (data != NULL) { > > Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] These are code-genned, but I don't think it's in my change. Shall I hunt it down? >>> Source/WebCore/bindings/scripts/test/JS/JSTestSerializedScriptValueInterface.cpp:285 >>> + RefPtr<SerializedScriptValue> data(SerializedScriptValue::create(exec, MAYBE_MISSING_PARAMETER(exec, 0, DefaultIsUndefined))); >> >> No transfer list for JSC yet? >> Either implement or file a separate bug to get it implemented. > > Same comment. I'll keep this change on V8 and file a separate bug. >> Source/WebCore/bindings/scripts/test/TestSerializedScriptValueInterface.idl:32 >> + Constructor(in DOMString hello, in [TransferList=transferList] SerializedScriptValue data, in [Optional] Array transferList), > > Test for multiple SSV+transferList pairs? > I know we do not have a use case for such yet but if we implement the attribute we should better get it right. oy! Good edge case. Would such a function signature even arise, though?

> >> Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestSerializedScriptValueInterface.cpp:68 > >> + if (data != NULL) { > > > > Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] > > These are code-genned, but I don't think it's in my change. Shall I hunt it down? It's a yak that we should shave one day, but that day doesn't need to be today. :)

Comment on attachment 131871 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131871&action=review >>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1547 >>> + my @allParameterNames = (); >>> + foreach my $parameter (@{$function->parameters}) { >>> + push(@allParameterNames, $parameter->name); >>> + } >> >> Nit: Shall we move this code into the 'if ($parameter->extendedAttributes->{"TransferList"})' block? > > I was just passing in $function->parameters to the GetIndexOf function, but it wasn't working at all. I still don't understand why. Maybe it was still while I was fighting the array parameter... I agree this is kind of ugly sitting here, and I would like to believe there's a neat way to do this all at once. Something like python filters? Ah, I just meant, you can move the following whole code: 1544 my @allParameterNames = (); 1545 foreach my $parameter (@{$function->parameters}) { 1546 push(@allParameterNames, $parameter->name); 1547 } into the if block, because the code is used by the if block only. >>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1597 >>> + $paramTransferListIndex = GetIndexOf($paramTransferListName, @allParameterNames); >> >> Nit FYI: Perl-er would write '$paramTransferListIndex = grep { $allParameterNames[$_] eq $paramTransferListName } 0..$#allParameterNames'. But GetIndexOf() is better for readability. > > Would that combine nicely with @($function->parameters)? I have forgotten much of the perl I once knew, so it's like I can almost remember that this should be easy. :-/ No. @allParameterNames is an array of all names in @{$function->parameters}. If you do not want to introduce @allParameterNames, the code would look like $paramTransferListIndex = grep { ${$function->parameters}[$_]->name eq $paramTransferListName } 0..$#{$function->parameters}; which is far from readable.

(In reply to comment #7) > (From update of attachment 131871 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=131871&action=review > >> Source/WebCore/bindings/scripts/test/TestSerializedScriptValueInterface.idl:32 > >> + Constructor(in DOMString hello, in [TransferList=transferList] SerializedScriptValue data, in [Optional] Array transferList), > > > > Test for multiple SSV+transferList pairs? > > I know we do not have a use case for such yet but if we implement the attribute we should better get it right. > > oy! Good edge case. Would such a function signature even arise, though? Who knows? However if we ship a generator with support of an attribute on an argument, we should support attribute on multiple arguments. If we do not want to support specifying attribute on multiple arguments, let's have an attribute on a method instead.

Comment on attachment 131871 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131871&action=review >>>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1547 >>>> + } >>> >>> Nit: Shall we move this code into the 'if ($parameter->extendedAttributes->{"TransferList"})' block? >> >> I was just passing in $function->parameters to the GetIndexOf function, but it wasn't working at all. I still don't understand why. Maybe it was still while I was fighting the array parameter... I agree this is kind of ugly sitting here, and I would like to believe there's a neat way to do this all at once. Something like python filters? > > Ah, I just meant, you can move the following whole code: > > 1544 my @allParameterNames = (); > 1545 foreach my $parameter (@{$function->parameters}) { > 1546 push(@allParameterNames, $parameter->name); > 1547 } > > into the if block, because the code is used by the if block only. Yeah, the code to do it in-place looked kind of nasty. Moved this block in. It is definitely more readable. Can't shake the feeling that there's a good alternative, but I can't produce it. :-/ >>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1599 >>> + next; # weak sauce! do we have runtime errors? IDL error: TransferList refs a nonexistent param >> >> Hmm IDLParser.pm suggests you go >> die("Error message") > > Yeah, die is appropriate here. Fixed up. Thanks.

Created attachment 132084 [details] Patch

Attachment 132084 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestSerializedScriptValueInterface.cpp:68: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestSerializedScriptValueInterface.cpp:73: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/bindings/scripts/test/V8/V8TestSerializedScriptValueInterface.cpp:118: messagePortArray_transferList is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebCore/bindings/scripts/test/V8/V8TestSerializedScriptValueInterface.cpp:119: arrayBufferArray_transferList is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebCore/bindings/scripts/test/V8/V8TestSerializedScriptValueInterface.cpp:165: messagePortArray_transferList is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebCore/bindings/scripts/test/V8/V8TestSerializedScriptValueInterface.cpp:166: arrayBufferArray_transferList is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebCore/bindings/scripts/test/V8/V8TestSerializedScriptValueInterface.cpp:177: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 7 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 132089 [details] Patch

Attachment 132089 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestSerializedScriptValueInterface.cpp:68: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestSerializedScriptValueInterface.cpp:73: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestSerializedScriptValueInterface.cpp:91: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestSerializedScriptValueInterface.cpp:96: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestSerializedScriptValueInterface.cpp:101: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestSerializedScriptValueInterface.cpp:106: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/bindings/scripts/test/V8/V8TestSerializedScriptValueInterface.cpp:118: messagePortArray_transferList is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebCore/bindings/scripts/test/V8/V8TestSerializedScriptValueInterface.cpp:119: arrayBufferArray_transferList is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebCore/bindings/scripts/test/V8/V8TestSerializedScriptValueInterface.cpp:144: messagePortArray_tx is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebCore/bindings/scripts/test/V8/V8TestSerializedScriptValueInterface.cpp:145: arrayBufferArray_tx is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebCore/bindings/scripts/test/V8/V8TestSerializedScriptValueInterface.cpp:162: messagePortArray_txx is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebCore/bindings/scripts/test/V8/V8TestSerializedScriptValueInterface.cpp:163: arrayBufferArray_txx is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebCore/bindings/scripts/test/V8/V8TestSerializedScriptValueInterface.cpp:210: messagePortArray_transferList is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebCore/bindings/scripts/test/V8/V8TestSerializedScriptValueInterface.cpp:211: arrayBufferArray_transferList is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Source/WebCore/bindings/scripts/test/V8/V8TestSerializedScriptValueInterface.cpp:222: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 15 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.

The second patch adds the multi-arg test Dmitry asked for. Good thing, since it shook loose a bug. :-) The tangential diffs in the test files are now quite large. Sorry about that. :-/ They are all mechanical except the changes in V8TestSerializedScriptInterface.cpp I used "_<<argname>>" as a suffix for the intermediate args to cut the C++ compiler some slack, but that makes the lint checker unhappy. Should I just omit the "_"?

(In reply to comment #16) > The second patch adds the multi-arg test Dmitry asked for. Good thing, since it shook loose a bug. :-) Right, I thought I see the bug in your codegen ;)) Asking for a test is the polite way of pointing it out :))))). > > The tangential diffs in the test files are now quite large. Sorry about that. :-/ They are all mechanical except the changes in V8TestSerializedScriptInterface.cpp > > I used "_<<argname>>" as a suffix for the intermediate args to cut the C++ compiler some slack, but that makes the lint checker unhappy. Should I just omit the "_"? Let's not add more style errors to our codegen. I suggest removing the underscore and capitalizing parameter name (messagePortArrayArg1).

Comment on attachment 132089 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132089&action=review > Source/WebCore/bindings/scripts/test/CPP/WebDOMTestSerializedScriptValueInterface.cpp:133 > + impl()->acceptTransferList(WebCore::SerializedScriptValue::create(WTF::String(data)), toWebCore(transferList)); Hmm I thought about this further - now I am not sure about these CPP bindings? Do they even compile? What type toWebCore(transferList) has here?

> Hmm I thought about this further - now I am not sure about these CPP bindings? Do they even compile? What type toWebCore(transferList) has here? Let's not worry too much about the CPP bindings. These tests files don't need to compile. They exist for visual diffing.

(In reply to comment #19) > > Hmm I thought about this further - now I am not sure about these CPP bindings? Do they even compile? What type toWebCore(transferList) has here? > > Let's not worry too much about the CPP bindings. These tests files don't need to compile. They exist for visual diffing. Well, ideally the idl should produce something the compiles for C++. I do not see how this signature can make sense for C++ or ObjC. AFAIU, we do not really have a precedent for that in WebKit today - all methods that _accept SSV as a parameterhave custom bindings (correct me if I am wrong). Anyway, looks like we do not have to fix this for this patch, but I am eager to see how this pans out in real-life use-case.

I wouldn't spend any time worrying about the CPP bindings.

(In reply to comment #17) > (In reply to comment #16) > > The second patch adds the multi-arg test Dmitry asked for. Good thing, since it shook loose a bug. :-) > > Right, I thought I see the bug in your codegen ;)) Asking for a test is the polite way of pointing it out :))))). :-) Feel free to directly point out any bugs you see. > > > > The tangential diffs in the test files are now quite large. Sorry about that. :-/ They are all mechanical except the changes in V8TestSerializedScriptInterface.cpp > > > > I used "_<<argname>>" as a suffix for the intermediate args to cut the C++ compiler some slack, but that makes the lint checker unhappy. Should I just omit the "_"? > > Let's not add more style errors to our codegen. I suggest removing the underscore and capitalizing parameter name (messagePortArrayArg1). Done.

Created attachment 132128 [details] Patch

Attachment 132128 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestSerializedScriptValueInterface.cpp:68: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestSerializedScriptValueInterface.cpp:73: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestSerializedScriptValueInterface.cpp:91: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestSerializedScriptValueInterface.cpp:96: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestSerializedScriptValueInterface.cpp:101: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestSerializedScriptValueInterface.cpp:106: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/bindings/scripts/test/V8/V8TestSerializedScriptValueInterface.cpp:222: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 7 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #22) > (In reply to comment #17) > > (In reply to comment #16) > > > The second patch adds the multi-arg test Dmitry asked for. Good thing, since it shook loose a bug. :-) > > > > Right, I thought I see the bug in your codegen ;)) Asking for a test is the polite way of pointing it out :))))). > > :-) Feel free to directly point out any bugs you see. Sorry I didn't mean to sound arrogant - I'd say I suspected there was a bug... If I was 100% sure I would have pointed it directly..

Comment on attachment 132128 [details] Patch This looks good to me. I defer to Perl experts for perl review.

> This looks good to me. I defer to Perl experts for perl review. Thanks. This looks good to me. If folks have more comments, we can iterate from here. Hopefully this will unblock the WebIntents work though.

Comment on attachment 132128 [details] Patch Clearing flags on attachment: 132128 Committed r110972: <http://trac.webkit.org/changeset/110972>

All reviewed patches have been landed. Closing bug.