[webkit-reviews] review denied: [Bug 124537] Build script should fail when any invalid option is passed to it : [Attachment 217317] 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 124537: Build script should fail when any invalid option is passed to it
https://bugs.webkit.org/show_bug.cgi?id=124537

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

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


> Tools/ChangeLog:11
> +	   NOTICE: 'build-webkit' pass through command-line options only
> +	   for xcode builds as it has its customized options/features
mechanism.

This sentence doesn't read well. Maybe:

Note, build-webkit will not error out when it encounters an unrecognized
command line option when building the Mac or iOS port. For these ports,
unrecognized options will be passed through so that Xcode can process them.

> Tools/ChangeLog:15
> +	   options which has to be added to the 'options' hashtable so that

Nit: hashtable => hash table

> Tools/ChangeLog:17
> +	   GetOpt::Long::GetOptions will be aware of them. That's is by the
> +	   new function 'initializePrivateOptions' added into webkitdirs
module.

This sentence doesn't read well. Moreover, I suggest you make this sentence a
function-specific remark for initializePrivateOptions() (i.e. move the sentence
to line 32 in the change log entry), maybe:

(initializePrivateOptions): Added; used internally to expose command line
options for non-{iOS, Mac} ports.

> Tools/ChangeLog:25
> +	   (determineIsWK2):
> +	   (isWK2):
> +	   (determineShouldUpdateEfl):
> +	   (shouldUpdateEfl):
> +	   (determineShouldUpdateGtk):
> +	   (shouldUpdateGtk):
> +	   (shouldUseGuardMalloc):
> +	   (determineShouldUseGuardMalloc):

Please add the remark "Added" to these change log lines just as you did for
initializePrivateOptions() (below).

This will make it straightforward to determine when a function was added by
searching for the functions name in Trac or SVN/Git history.

> Tools/ChangeLog:29
> +	   In adition, this patch standardizes the way command-line options are


Nit: adition => addition

> Tools/ChangeLog:32
> +	   (initilizePrivateOptions): Added.

Nit: initilizePrivateOptions => initializePrivateOptions

That is, the word "initialize" is misspelled in the name of this function.

> Tools/Scripts/build-webkit:133
> +    initilizePrivateOptions(\%options);

Nit: initilizePrivateOptions => initializePrivateOptions

That is, the word "initialize" is misspelled in the name of this function.

> Tools/Scripts/webkitdirs.pm:105
>  my $shouldUseXPCServiceForWebProcess;
>  my $shouldUseGuardMalloc;
>  my $xcodeVersion;
> +my $shouldUpdateEfl;
> +my $shouldUpdateGtk;

We should sort this list of declarations and insert these variable declarations
in sorted order.

> Tools/Scripts/webkitdirs.pm:899
> +sub determineIsWK2

Please add "()" to the end of this line to force Perl to error when this
function is called with an argument.

> Tools/Scripts/webkitdirs.pm:1054
> +sub determineShouldUpdateEfl

Ditto.

> Tools/Scripts/webkitdirs.pm:1060
> +sub shouldUpdateEfl

Ditto.

> Tools/Scripts/webkitdirs.pm:1392
> +sub shouldUseGuardMalloc

Please add "()" to the end of this line to force Perl to error when this
function is called with an argument.

> Tools/Scripts/webkitdirs.pm:1398
> +sub determineShouldUseGuardMalloc

Ditto.

> Tools/Scripts/webkitdirs.pm:2360
> +sub initilizePrivateOptions($)

Nit: initilizePrivateOptions => initializePrivateOptions

That is, the word "initialize" is misspelled in the name of this function.

> Tools/Scripts/webkitdirs.pm:2371
> +	   'efl' => \$isEfl,
> +	   'gtk' => \$isGtk,
> +	   'blackberry' => \$isBlackBerry,
> +	   'wince' => \$isWinCE,
> +	   'wincairo' => \$isWinCairo,
> +	   '64-bit' => \$isWin64,
> +	   'update-efl' => \$shouldUpdateEfl,
> +	   'update-gtk' => \$shouldUpdateGtk

Nit: ' => "

We use double quotes for string literals unless we explicitly don't want string
interpolation.

> Tools/Scripts/webkitdirs.pm:2376
> +    determineArchitecture();
> +    determinePassedConfiguration();
> +    determineIsWK2();

Is this necessary to call these functions? I mean, we'll ultimately call these
functions when a caller queries for the such information (e.g. when
architecture() is called).