[webkit-reviews] review denied: [Bug 51642] Add WinCE support to build-webkit : [Attachment 77500] Patch

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has denied Patrick R. Gansterer
<paroga at paroga.com>'s request for review:
Bug 51642: Add WinCE support to build-webkit
https://bugs.webkit.org/show_bug.cgi?id=51642

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

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=77500&action=review

r- to clean up variable usage in buildCMakeProject().

>>> Tools/Scripts/build-webkit:522
>>> +	 exit exitStatus($result) if exitStatus($result);
>> 
>> Why does WinCE short-circut the rest of the build-webkit infrastructure? 
That seems like a bad idea long-term.
> 
> I do the same as EFL port, which uses CMake too.
> 
> (In reply to comment #3)

This is fine IMO.

> Tools/Scripts/webkitdirs.pm:1454
> +	       $makeArgs .= "--config Debug";

I think this should have a space at the beginning of the string (before
"--config").

> Tools/Scripts/webkitdirs.pm:1456
> +	       $makeArgs .= "--config Release";

This should also have a space at the beginning of the string.

Or are these cmake arguments that must come before the "--" argument?  If so,
they should really be in their own variable, not prepended to $makeArgs.

> Tools/Scripts/webkitdirs.pm:1459
> +	   $makeArgs .= "-- -j" . numberOfCPUs() if ($makeArgs !~ m/-j\s*\d+/);


Why is the empty "--" used here?  That makes this code path different than the
previous code path.

I see, you changed $make to "$cmakebin --build ." below, which means you need
to pass arguments to make after "--", right?  I think it would be clearer to
add the "--" to the $make command so that $makeArgs doesn't need to add it.

> Tools/Scripts/webkitdirs.pm:1469
> +	   my $make = $cmakebin . " --build .";

Add " --" to the end of the $make string here so it doesn't have to be added to
$makeArgs in special places.

> Tools/Scripts/webkitdirs.pm:1476
> +		   $makeArgs = "-- " . $1;

This change is not needed if " --" is prepended to $make above.