Bug 127169 – If not running on Mac, build-webkit should not print Safari related output

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Éva Balázsfalvi]

If not runned on MAC, "Tools/Scripts/build-webkit" should not post Safari related informations.

[Éva Balázsfalvi]

Created attachment 221463 [details]
Patch

[Daniel Bates]

Comment on attachment 221463 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=221463&action=review

> Tools/ChangeLog:3
> +        If not runned on MAC, "Tools/Scripts/build-webkit" should not post Safari related informations

Nit: MAC => Mac

Mac is not an acronym. It's shorthand for Macintosh.

> Tools/Scripts/build-webkit:431
> +    if ($launcherPath ne "0" && $launcherName ne "0") {

Without loss of generality, take $launcherName to be undefined (say launcherName() falls off the end of the function) then evaluating the condition of this if statement will cause an uninitialized value warning because we run this script with -w (see the first line in this script). Moreover, it seems weird to define "0" as an invalid value and compare to it. It's sufficient to look at the truth value of the variables:

if ($launcherPath && $launcherName) {
   ...
}

> Tools/Scripts/webkitdirs.pm:1290
> +    } elsif (isDarwin()) {

Can you elaborate on this change?

> Tools/Scripts/webkitdirs.pm:1305
> +    } elsif (isDarwin()) {

Ditto.

[Éva Balázsfalvi]

(In reply to comment #2)

Thank you for your feedback.

> Mac is not an acronym. It's shorthand for Macintosh.
> if ($launcherPath && $launcherName) { ... }

Thanks for noticing, you were absolutely right. I've modified the code.

>> Tools/Scripts/webkitdirs.pm:1290
>> +    } elsif (isDarwin()) {

>Can you elaborate on this change?

isAppleWebKit() gives back a value depending on the values of isGtk(), isEfl(), isWinCE(), isNix(). If we're using the script without any switch, each of these fuctions will give back a false value. But because of this, isAppleWebKit() will return as true, regardless of where we were running the script. This is why the Safari related output was given. isDarwin()'s value doesn't depend on any of the previously mentioned functions, that is why I thought it might be a good idea to use it instead of isAppleWebKit().

[Éva Balázsfalvi]

Ping?

[Csaba Osztrogonác]

(In reply to comment #4)
> Ping?
You turn now. :) Please fix what Daniel mentioned in https://bugs.webkit.org/show_bug.cgi?id=127169#c2 and then upload the fixed patch with r?

[Éva Balázsfalvi]

Created attachment 222980 [details]
Patch

[Darin Adler]

Comment on attachment 222980 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=222980&action=review

> Tools/Scripts/build-webkit:434
> +    if ($launcherPath && $launcherName) {
> +        print " To run $launcherName with this newly-built code, use the\n";
> +        print " \"$launcherPath\" script.\n";
> +    }

I understand this change.

> Tools/Scripts/webkitdirs.pm:1290
> -    } elsif (isAppleWebKit()) {
> +    } elsif (isDarwin()) {

Why this change? Unclear how this relates to what the change log says. This makes it look like it will use run-safari more often, for anyone running on a Darwin system. That seems like the opposite of what the change log says this patch does.

> Tools/Scripts/webkitdirs.pm:1307
> +    } elsif (isDarwin()) {
> +        return "Safari";
>      }

Same comment here.

[Darin Adler]

(In reply to comment #3)
> isAppleWebKit() gives back a value depending on the values of isGtk(), isEfl(), isWinCE(), isNix(). If we're using the script without any switch, each of these fuctions will give back a false value. But because of this, isAppleWebKit() will return as true, regardless of where we were running the script.

If this is the problem, then we need to fix the isAppleWebKit function to correctly return false in these cases, rather than changing how it’s used.

[Brent Fulgham]

Comment on attachment 222980 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=222980&action=review

> Tools/Scripts/webkitdirs.pm:-1300
> -        return "Safari";

We should change this to WinLauncher.

[Éva Balázsfalvi]

Created attachment 223332 [details]
Patch

[Csaba Osztrogonác]

Comment on attachment 223332 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=223332&action=review

Please fix the typo before landing.

> Tools/Scripts/webkitdirs.pm:1116
> +    return return isDarwin() && !isGtk();

typo: return return

> Tools/Scripts/webkitdirs.pm:1121
> +    return return (isCygwin() || isWindows()) && !isWinCairo() && !isGtk() && !isWinCE();

ditto

[Éva Balázsfalvi]

Created attachment 223692 [details]
Patch

[Éva Balázsfalvi]

Created attachment 223697 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 223697 [details]
patch for landing

Clearing flags on attachment: 223697

Committed r163780: <http://trac.webkit.org/changeset/163780>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.