[webkit-reviews] review granted: [Bug 96181] [WK2][WTR] CodeGeneratorTestRunner could keep original copyright. : [Attachment 164484] patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Kangil Han
<kangil.han at samsung.com>'s request for review:
Bug 96181: [WK2][WTR] CodeGeneratorTestRunner could keep original copyright.
https://bugs.webkit.org/show_bug.cgi?id=96181

Attachment 164484: patch
https://bugs.webkit.org/attachment.cgi?id=164484&action=review

------- Additional Comments from Daniel Bates <dbates at webkit.org>
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.