Bug 81127

Summary: Add TransferList IDL modifier, with support in V8 code gen.
Product: WebKit Reporter: Greg Billock <gbillock>
Component: New BugsAssignee: Greg Billock <gbillock>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dslomov, haraken, japhet, ojan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 80200    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Greg Billock
Reported 2012-03-14 09:45:45 PDT
Add TransferList IDL modifier, with support in V8 code gen.
Attachments
Patch (34.13 KB, patch)
2012-03-14 09:46 PDT, Greg Billock
no flags
Patch (34.43 KB, patch)
2012-03-15 11:28 PDT, Greg Billock
no flags
Patch (42.44 KB, patch)
2012-03-15 11:38 PDT, Greg Billock
no flags
Patch (42.52 KB, patch)
2012-03-15 14:59 PDT, Greg Billock
no flags
Greg Billock
Comment 1 2012-03-14 09:46:32 PDT
Greg Billock
Comment 2 2012-03-14 09:49:04 PDT
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.
Dmitry Lomov
Comment 3 2012-03-14 10:15:39 PDT
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.
Adam Barth
Comment 4 2012-03-14 12:32:17 PDT
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.
WebKit Review Bot
Comment 5 2012-03-14 14:36:24 PDT
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.
Kentaro Hara
Comment 6 2012-03-14 17:37:50 PDT
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.
Greg Billock
Comment 7 2012-03-14 18:14:18 PDT
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?
Adam Barth
Comment 8 2012-03-14 18:17:02 PDT
> >> 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. :)
Kentaro Hara
Comment 9 2012-03-14 18:24:47 PDT
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.
Dmitry Lomov
Comment 10 2012-03-14 19:17:03 PDT
(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.
Greg Billock
Comment 11 2012-03-15 11:27:45 PDT
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.
Greg Billock
Comment 12 2012-03-15 11:28:01 PDT
WebKit Review Bot
Comment 13 2012-03-15 11:32:12 PDT
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.
Greg Billock
Comment 14 2012-03-15 11:38:39 PDT
WebKit Review Bot
Comment 15 2012-03-15 11:42:34 PDT
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.
Greg Billock
Comment 16 2012-03-15 11:43:19 PDT
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 "_"?
Dmitry Lomov
Comment 17 2012-03-15 12:12:15 PDT
(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).
Dmitry Lomov
Comment 18 2012-03-15 12:16:45 PDT
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?
Adam Barth
Comment 19 2012-03-15 13:44:06 PDT
> 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.
Dmitry Lomov
Comment 20 2012-03-15 13:56:04 PDT
(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.
Adam Barth
Comment 21 2012-03-15 14:11:57 PDT
I wouldn't spend any time worrying about the CPP bindings.
Greg Billock
Comment 22 2012-03-15 14:56:15 PDT
(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.
Greg Billock
Comment 23 2012-03-15 14:59:19 PDT
WebKit Review Bot
Comment 24 2012-03-15 15:02:52 PDT
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.
Dmitry Lomov
Comment 25 2012-03-15 15:08:53 PDT
(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..
Dmitry Lomov
Comment 26 2012-03-15 15:10:10 PDT
Comment on attachment 132128 [details] Patch This looks good to me. I defer to Perl experts for perl review.
Adam Barth
Comment 27 2012-03-15 15:50:22 PDT
> 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.
WebKit Review Bot
Comment 28 2012-03-16 01:56:31 PDT
Comment on attachment 132128 [details] Patch Clearing flags on attachment: 132128 Committed r110972: <http://trac.webkit.org/changeset/110972>
WebKit Review Bot
Comment 29 2012-03-16 01:56:37 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.