[webkit-reviews] review granted: [Bug 22602] Enable Java in DumpRenderTree : [Attachment 49339] proposed patch

[bugzilla-daemon at webkit.org]

Mark Rowe (bdash) <mrowe at apple.com> has granted Alexey Proskuryakov
<ap at webkit.org>'s request for review:
Bug 22602: Enable Java in DumpRenderTree
https://bugs.webkit.org/show_bug.cgi?id=22602

Attachment 49339: proposed patch
https://bugs.webkit.org/attachment.cgi?id=49339&action=review

------- Additional Comments from Mark Rowe (bdash) <mrowe at apple.com>
> Index: WebKitTools/DumpRenderTree/DumpRenderTree.h
> ===================================================================
> --- WebKitTools/DumpRenderTree/DumpRenderTree.h	(revision 55174)
> +++ WebKitTools/DumpRenderTree/DumpRenderTree.h	(working copy)
> @@ -53,6 +53,7 @@ std::wstring urlSuitableForTestResult(co
>  class LayoutTestController;
>  
>  extern volatile bool done;
> +void exitApplicationRunLoop();

It looks like this is no longer used outside of DumpRenderTreeMac.mm.  It can
be removed from this header and marked as static.

> Index: WebKitTools/DumpRenderTree/LayoutTestController.cpp
> ===================================================================
> --- WebKitTools/DumpRenderTree/LayoutTestController.cpp	(revision
55174)
> +++ WebKitTools/DumpRenderTree/LayoutTestController.cpp	(working copy)
> @@ -1278,6 +1278,15 @@ static JSValueRef apiTestNewWindowDataLo
>      return JSValueMakeUndefined(context);
>  }
>  
> +
> +static JSValueRef enableJavaCallback(JSContextRef context, JSObjectRef
function, JSObjectRef thisObject, size_t argumentCount, const JSValueRef
arguments[], JSValueRef* exception)
> +{
> +    LayoutTestController* controller =
static_cast<LayoutTestController*>(JSObjectGetPrivate(thisObject));
> +    controller->setJavaEnabled(true);
> +
> +    return JSValueMakeUndefined(context);
> +}

Can the tests make use of LayoutTestController.overridePreference rather than
these Java-specific methods?  It is what is used to enable the WebGL preference
in each of those tests.

> +void exitApplicationRunLoop()
> +{
> +    [[NSApplication sharedApplication] stop:nil];
> +    [[NSApplication sharedApplication] postEvent:[NSEvent
otherEventWithType:NSApplicationDefined location:NSMakePoint(0, 0)
modifierFlags:0 timestamp:0 windowNumber:0 context:0 subtype:0 data1:0 data2:0]
atStart:NO];
> +}

The postEvent: call here is a little bit mysterious until you realize that the
run loop is blocked waiting on an event.  A comment could make this less
mysterious.

> Index: WebKitTools/Scripts/run-webkit-tests
> ===================================================================
> --- WebKitTools/Scripts/run-webkit-tests	(revision 55174)
> +++ WebKitTools/Scripts/run-webkit-tests	(working copy)
> @@ -398,6 +398,16 @@ $expectedDirectory = $ENV{"WebKitExpecte
>  $testResultsDirectory = File::Spec->rel2abs($testResultsDirectory);
>  my $testResults = File::Spec->catfile($testResultsDirectory,
"results.html");
>  
> +if (isAppleMacWebKit()) {
> +    print STDERR "Compiling Java tests\n";
> +    my $javaTestsDirectory = catdir($testDirectory, "java");
> +
> +    if (system("/usr/bin/make -C $javaTestsDirectory")) {
> +	   exit 1;
> +    }
> +}

This will break if $testDirectory happens to contain a space or shell
meta-character.  You can avoid this by using: system “/usr/bin/make”, “-C”,
$javaTestsDirectory

> +	   * java/Makefile: Added. Compile all .java files in the directory.

Please make sure that you add .class files to the svn:ignore property.


> Index: LayoutTests/java/Makefile
> ===================================================================
> --- LayoutTests/java/Makefile (revision 0)
> +++ LayoutTests/java/Makefile (revision 0)
> @@ -0,0 +1,41 @@
> +# Copyright (C) 2010 Apple Inc. All rights reserved.
> +#
> +# Redistribution and use in source and binary forms, with or without
> +# modification, are permitted provided that the following conditions
> +# are met:
> +#
> +# 1.  Redistributions of source code must retain the above copyright
> +#	 notice, this list of conditions and the following disclaimer. 
> +# 2.  Redistributions in binary form must reproduce the above copyright
> +#	 notice, this list of conditions and the following disclaimer in the
> +#	 documentation and/or other materials provided with the distribution. 
> +# 3.  Neither the name of Apple Computer, Inc. ("Apple") nor the names of
> +#	 its contributors may be used to endorse or promote products derived
> +#	 from this software without specific prior written permission. 

This looks like the old license header.

r=me, but I’d suggest checking whether you can use overridePreference rather
than introducing a new call on LayoutTestController.