[webkit-reviews] review granted: [Bug 91387] [CMake][EFL] Building jsc causes reconfiguration : [Attachment 152731] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Thiago Marcos P. Santos
<tmpsantos at gmail.com>'s request for review:
Bug 91387: [CMake][EFL] Building jsc causes reconfiguration
https://bugs.webkit.org/show_bug.cgi?id=91387

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

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


OK

> Tools/ChangeLog:9
> +	   other it will cause a reconfiguration every time someone calls

Nit: other => otherwise

> Tools/ChangeLog:11
> +	   not running WebKit 2 unit tests on the bots because the project was

Nit: WebKit 2 => WebKit2

> Tools/Scripts/webkitdirs.pm:2129
> +    my $config = configuration();
> +    my $buildPath = File::Spec->catdir(baseProductDir(), $config);
> +    my $cacheFilePath = File::Spec->catdir($buildPath, "CMakeCache.txt");

I don't see much value in defining the variables $config and $buildPath that
are only used towards building the filesystem path to the appropriate file
CMakeCache.txt. I would write this as:

my $cacheFilePath = File::Spec->catdir(baseProductDir(), configuration(),
"CMakeCache.txt");

> Tools/Scripts/webkitdirs.pm:2130
> +

Nit: I don't feel that this empty line improves the readability of this
function and I suggest that we remove it. The body of this function is concise
and straightforward to follow without this empty line.