[webkit-reviews] review denied: [Bug 124581] Modify webkitdirs to reuse checkForArgumentAndRemoveFromARGV : [Attachment 217313] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has denied Nick Diego Yamane (diegoyam)
<nick.yamane at openbossa.org>'s request for review:
Bug 124581: Modify webkitdirs to reuse checkForArgumentAndRemoveFromARGV
https://bugs.webkit.org/show_bug.cgi?id=124581

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

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=217313&action=review


This patch looks good. I'm r-'ing this patch because of the lack of a test.

> Tools/Scripts/webkitdirs.pm:393
> +    if (checkForArgumentAndRemoveFromARGVGettingValue('--sdk', \$sdk)) {

Nit: ' => "

That is, we use double quotes for string literals unless we explicitly don't
want string interpolation.

> Tools/Scripts/webkitdirs.pm:395
> +    } elsif (checkForArgumentAndRemoveFromARGV('--device')) {

Ditto.

> Tools/Scripts/webkitdirs.pm:397
> +    } elsif (checkForArgumentAndRemoveFromARGV('--sim') ||

Ditto.

> Tools/Scripts/webkitdirs.pm:398
> +	   checkForArgumentAndRemoveFromARGV('--simulator')) {

Ditto.

> Tools/Scripts/webkitdirs.pm:614
> +    if (checkForArgumentAndRemoveFromARGV('--debug')) {

Ditto.

> Tools/Scripts/webkitdirs.pm:616
> +    } elsif(checkForArgumentAndRemoveFromARGV('--release')) {

Ditto.

> Tools/Scripts/webkitdirs.pm:618
> +    } elsif (checkForArgumentAndRemoveFromARGV('--profile') ||
checkForArgumentAndRemoveFromARGV('--profiling')) {

Ditto.

> Tools/Scripts/webkitdirs.pm:653
> +    if (checkForArgumentAndRemoveFromARGV('--32-bit')) {

Ditto.

> Tools/Scripts/webkitdirs.pm:881
> +    my $removeOffset = 0;
>      foreach my $index (@indicesToRemove) {
> -	   splice(@$arrayRef, $index, 1);
> +	   splice(@$arrayRef, $index - $removeOffset++, 1);

Can we write a test for this change?

You may find it beneficial to look at the tests in
<http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitperl/> when writing
such a test, including
<http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitperl/VCSUtils_unittes
t/removeEOL.pl>.