Bug 96181

Created attachment 162954 [details] patch

Comment on attachment 162954 [details] patch Attachment 162954 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13792295

Comment on attachment 162954 [details] patch Attachment 162954 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13799151

Comment on attachment 162954 [details] patch Attachment 162954 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13794221

Comment on attachment 162954 [details] patch Attachment 162954 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13797330

Created attachment 163026 [details] patch Try it!

@weinig : review please? :-)

Comment on attachment 163026 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=163026&action=review > Tools/WebKitTestRunner/InjectedBundle/Bindings/CodeGeneratorTestRunner.pm:120 > my ($filename) = @_; $filename is unused, so it probably should be removed. (Also in where _fileHeaderString is called.) $filename is the name of the header file, not the IDL file.

Created attachment 163596 [details] patch Thanks for the review. Done!

The latest patch looks good to me.

Comment on attachment 163596 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=163596&action=review > Tools/WebKitTestRunner/InjectedBundle/Bindings/CodeGeneratorTestRunner.pm:127 > - * Copyright (C) 2010 Apple Inc. All rights reserved. > + * Copyright (C) 2010, 2011, 2012 Apple Inc. All rights reserved. I don't think anyone but Apple is allowed to update the Apple copyright.

(In reply to comment #11) > (From update of attachment 163596 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=163596&action=review > > > Tools/WebKitTestRunner/InjectedBundle/Bindings/CodeGeneratorTestRunner.pm:127 > > - * Copyright (C) 2010 Apple Inc. All rights reserved. > > + * Copyright (C) 2010, 2011, 2012 Apple Inc. All rights reserved. > > I don't think anyone but Apple is allowed to update the Apple copyright. Oh.. sorry for any inconvenience from this. I've just referred to copyright in TestRunner.cpp I will make a new patch to keep it as it is. :-)

Created attachment 163976 [details] patch Done!

> I don't think anyone but Apple is allowed to update the Apple copyright. When you copy code, you _have to_ update the copyright. Apple or an other owner. It does not apply here, but I wanted to make this clear since it is a frequent mistake.

Comment on attachment 163976 [details] patch It seems more appropriate to associate the path to the idl file with the CodeGeneratorTestRunner instance instead of storing this information in a file-level scope variable. We should also consider caching the license block that we parse from the idl file so that we don't read and parse the file twice for each call to GenerateInterface(). I need to run, but I'll comment further on this patch later tonight.

Comment on attachment 163976 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=163976&action=review Please excuse my tardy response as yesterday (09/13) was hectic. > Tools/WebKitTestRunner/InjectedBundle/Bindings/CodeGeneratorTestRunner.pm:31 > +my $idlFilePath = ""; This variable would be better suited as a instance variable. See how we define codeGenerator and outputDir in new() (below) for an example of how to define an instance variable. > Tools/WebKitTestRunner/InjectedBundle/Bindings/CodeGeneratorTestRunner.pm:43 > + $idlFilePath = shift; It's weird to use a file-level variable for this. See my remark (above) for more details. > Tools/WebKitTestRunner/InjectedBundle/Bindings/CodeGeneratorTestRunner.pm:86 > +sub _getCopyright Maybe a more descriptive name for this function would be _parseLicenseBlock, _parseLicenseBlockFromFile, or _parseCopyright? > Tools/WebKitTestRunner/InjectedBundle/Bindings/CodeGeneratorTestRunner.pm:88 > + my ($self) = @_; This variable ($self) is unused. I suggest that we make this a non-member function that takes exactly one argument, the path to the IDL file it should parse, say $path. > Tools/WebKitTestRunner/InjectedBundle/Bindings/CodeGeneratorTestRunner.pm:92 > + my $first = 1; > + my $copyRightMode = 0; We can eliminate the need for these variables by structuring the code in this function such that specialize the handling of the first line in the file. See my remark for lines 104-111 (below) for more details. > Tools/WebKitTestRunner/InjectedBundle/Bindings/CodeGeneratorTestRunner.pm:93 > + my $copyRight = ""; Nit: $copyRight => $copyright (since "copyright" is one word) > Tools/WebKitTestRunner/InjectedBundle/Bindings/CodeGeneratorTestRunner.pm:96 > + $copyRight = $copyRight . $line; Nit: I would write this as: $copyRight .= $line; > Tools/WebKitTestRunner/InjectedBundle/Bindings/CodeGeneratorTestRunner.pm:98 > + close($fileHandle) or die "Failed to close $idlFilePath after reading: $!"; Nit: It's unusual to anticipate that close() will fail for a file. We tend to omit die() when calling close() throughout our Perl code. > Tools/WebKitTestRunner/InjectedBundle/Bindings/CodeGeneratorTestRunner.pm:111 > + if ($first) { > + if (0 != index($line, "/*")) { > + last; > + } > + $first = 0; > + $copyRightMode = 1; > + $copyRight = $copyRight . $line; > + } For your consideration, I suggest that we hoist this code outside of the while loop and structure this code to use an early return style. This will make it more straightforward to follow the control flow of this function. As a side effect of hoisting this code outside of the while loop, we can remove the variables $first and $copyRightMode as the states they represent are now encoded in the control flow of the function. That is, we handle the first state, $first = 1, when we check that the first line of the file starts with "/*". And the second state, $copyRightMode = 1, is encoded by the position of the while-loop, which is after handling the if conditional for the first state. > Tools/WebKitTestRunner/InjectedBundle/Bindings/CodeGeneratorTestRunner.pm:118 > sub _fileHeaderString Instead of modifying this function to call _getCopyright() I propose that we add a member function, say _licenseBlock(), that has the form: sub _licenseBlock() { my ($self) = @_; if ($self->{licenseBlock}) { return $self->{licenseBlock} } my $licenseBlock = _getCopyright($self->{idlFilePath}) || _defaultLicenseBlock(); $self->{licenseBlock} = $licenseBlock; return $licenseBlock; } where _defaultLicenseBlock() is identical to _fileHeaderString() (as it was before this patch was applied) except it doesn't accept any arguments (i.e. we remove the line "my ($filename) = @_"). Then we can modify _generateHeaderFile() and _generateImplementationFile() to call _licenseBlock(). For completeness, I chose to cache the return value of _getCopyright() in _licenseBlock() so as to avoid reading the IDL file twice: once when generating the header file in _generateHeaderFile() and once when generating the implementation file in _generateImplementationFile(). I also chose to define a function for the default license block, _defaultLicenseBlock(), instead of defining a file-level variable for it so as to avoid constructing a string on startup that may not be used. > Tools/WebKitTestRunner/InjectedBundle/Bindings/CodeGeneratorTestRunner.pm:123 > + if ($copyright ne "") { > + return $copyright; > + } Nit: Notice that _getCopyright() always returns a string, the truth value of the empty string is false, and the truth value of a non-empty string is true. It's sufficient to test the boolean truth value of $copyright instead of performing a string comparison operation (ne) and write: if ($copyright) { return $copyright; }

Comment on attachment 163976 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=163976&action=review I inadvertently didn't add my remarks about the correctness of this function for some abnormal inputs. Maybe we shouldn't expect such input? >> Tools/WebKitTestRunner/InjectedBundle/Bindings/CodeGeneratorTestRunner.pm:86 >> +sub _getCopyright > > Maybe a more descriptive name for this function would be _parseLicenseBlock, _parseLicenseBlockFromFile, or _parseCopyright? 1. Consider an IDL file that begins with: /* Copyright (C) 2012 Apple Inc. All rights reserved. */ Then _getCopyright() returns the empty string. But it should return the string: "/* Copyright (C) 2012 Apple Inc. All rights reserved. */". 2. Consider an IDL file that begins with: /* * Copyright (C) 2012 Apple Inc. All rights reserved. */ module WTR { ... } (Notice that there is IDL markup on the same line as "*/") Then _getCopyright() returns the string: "/* * Copyright (C) 2012 Apple Inc. All rights reserved. */ module WTR {" But it should return the string: "/* * Copyright (C) 2012 Apple Inc. All rights reserved. */"

Created attachment 164484 [details] patch I've modified code as you have suggested. Hope this would meet your expectations. :-)

Comment on attachment 164484 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=164484&action=review Thanks for updating the patch! I noticed correctness issues on line 84 and 89. I also noticed some minor nits. > Tools/WebKitTestRunner/InjectedBundle/Bindings/CodeGeneratorTestRunner.pm:82 > + my ($copyright, $curData, $readCount, $lastData); Maybe better names for $curData would be: $currentCharacter? or $currentChar? Maybe a better name for $lastData would be: $previousCharacter? or $previousChar? > Tools/WebKitTestRunner/InjectedBundle/Bindings/CodeGeneratorTestRunner.pm:83 > + $readCount = read $fileHandle, $curData, 2; For your consideration, I suggest that we define a variable, maybe lengthOfStartSentinel = length("/*") (can we come up with a better name?), and then reference this variable instead of hardcoding the number 2. I also suggest that we use parentheses around the argument list for read() so as to make this line easier to interpret as a function call: $readCount = read($fileHandle, $curData, lengthOfStartSentinel) > Tools/WebKitTestRunner/InjectedBundle/Bindings/CodeGeneratorTestRunner.pm:84 > + return "" if ($readCount < 2 && $curData ne "/*"); && => || It is good form to close the file before returning from this function. If you find it inconvenient to call close()- or you feel it makes the code less readable to call close()- before returning from this function then one idea is that we could repurpose this function to parse the license block given a file handle and define a function called _parseLicenseBlockFromFile() that opens the specified file and calls _parseLicenseBlock() then closes the file. This would have the form: sub _parseLicenseBlockFromFile { my ($path) = @_; open my $fileHandle, "<", $path or die "Failed to open $path for reading: $!"; my $licenseBlock = _parseLicenseBlock($fileHandle); close($fileHandle); return $licenseBlock; } sub _parseLicenseBlock { my ($fileHandle) = @_; my ($copyright, $curData, $readCount, $lastData); $readCount = read $fileHandle, $curData, 2; ... } Then we can return from _parseLicenseBlock() without calling close() since _parseLicenseBlockFromFile() will do it. > Tools/WebKitTestRunner/InjectedBundle/Bindings/CodeGeneratorTestRunner.pm:87 > + while (($readCount = read $fileHandle, $curData, 1) != 0) { Nit: It's unnecessary to check "!=" as zero has a truth value of false. We can write the while statement as: while ($readCount = read $fileHandle, $curData, 1) For your consideration, I suggest that we use parentheses around the argument list for read() so as to make it easier to interpret the call to read() as a function call: while ($readCount = read($fileHandle, $curData, 1)) > Tools/WebKitTestRunner/InjectedBundle/Bindings/CodeGeneratorTestRunner.pm:89 > + return $copyright if $curData eq "/" && $lastData eq "*"; It is good form to call close() before returning from this function. See my remark on line 84 for an idea on how we can can eliminate the need to call close() by moving the responsibility to another function. > Tools/WebKitTestRunner/InjectedBundle/Bindings/CodeGeneratorTestRunner.pm:130 > + return $self->{licenseBlock} if ($self->{licenseBlock}); Nit: The parentheses aren't necessary. > Tools/WebKitTestRunner/InjectedBundle/Bindings/CodeGeneratorTestRunner.pm:132 > + my $licenseBlock = _parseLicenseBlock($$self{idlFilePath}) || _defaultLicenseBlock(); It's kind of weird that we alternate between using the infix dereference operator ("->") and the scalar deference operator ($) in this function for accessing instance variables. We should pick one convention and stick with it.

Created attachment 164648 [details] patch Thanks for great review Daniel! All Done!

Comment on attachment 164648 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=164648&action=review > Tools/WebKitTestRunner/InjectedBundle/Bindings/CodeGeneratorTestRunner.pm:84 > + return "" if ($readCount < 2 || $currentCharacter ne "/*"); 2 => $lengthOfStartSentinel

Comment on attachment 164648 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=164648&action=review >> Tools/WebKitTestRunner/InjectedBundle/Bindings/CodeGeneratorTestRunner.pm:84 >> + return "" if ($readCount < 2 || $currentCharacter ne "/*"); > > 2 => $lengthOfStartSentinel The name $currentCharacter is somewhat disingenuous here since it contains two characters. Maybe buffer would better describe this variable? Maybe this is overkill...since we decided to compute the length of "/*" instead of hardcoding the number 2 then we shouldn't hardcode the string literal "/*". Instead we should define a variable, say $startSentinel = "/*", and then define $lengthOfStartSentinel in terms of it (i.e. $lengthOfStartSentinel = length($startSentinel)). Regardless, these are very minor points. The patch looks reasonable as-is.

Created attachment 164659 [details] patch Done, thanks!

Comment on attachment 164659 [details] patch Clearing flags on attachment: 164659 Committed r128966: <http://trac.webkit.org/changeset/128966>

All reviewed patches have been landed. Closing bug.