Bug 135269 – iOS Simulator LayoutTestRelay

• Component: Tools / Tests
• Status: RESOLVED FIXED

[David Farler]

Bug for new tool to relay DumpRenderTree / WebKitTestRunner IO in the simulator.

[David Farler]

<rdar://problem/10864368>

[David Farler]

Created attachment 235576 [details]
Patch

[David Farler]

Thanks to Matt Wright for providing most of the OptionParser code.

[Build Bot]

Comment on attachment 235576 [details]
Patch

Attachment 235576 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4809760775864320

New failing tests:
media/track/add-and-remove-track.html

[Build Bot]

Created attachment 235578 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Simon Fraser (smfr)]

I think you should email webkit-dev and ask if people are OK with using Swift in the webkit repository.

[David Kilzer (:ddkilzer)]

(In reply to comment #6)
> I think you should email webkit-dev and ask if people are OK with using Swift in the webkit repository.

This is a good thing to do, but LayoutTestRelay will only be used for the iOS Simulator at this time, and the iOS Simulator only runs on Mac OS X.

Note that Swift was chosen because we wanted a scripting language with an Objective-C bridge.  (MacRuby was removed from Yosemite, and Python hasn't been updated in a long time.)  Obviously we could have used C (with platform-specific Objective-C code), but that would have required compiling.

If there is a need to extend LayoutTestRelay to work with other device simulators which are available to the public, then obviously we should revisit this decision.

[David Kilzer (:ddkilzer)]

(In reply to comment #7)
> Note that Swift was chosen because we wanted a scripting language with an Objective-C bridge.

Oops, I guess we're compiling it anyway!

[Simon Fraser (smfr)]

There's also an issue of having sufficient reviewer experience to review Swift code.

[David Farler]

(In reply to comment #8)
> (In reply to comment #7)
> > Note that Swift was chosen because we wanted a scripting language with an Objective-C bridge.
> 
> Oops, I guess we're compiling it anyway!

We don't really have to compile it, actually. I just got into the compiling mode while iterating in Xcode with my test arguments. I'll need to amend the patch to support immediate mode, though.

[David Kilzer (:ddkilzer)]

Comment on attachment 235576 [details]
Patch

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

r=me, but please email webkit-dev first, and consider the feedback.

> Tools/LayoutTestRelay/LayoutTestRelay.xcodeproj/project.pbxproj:262
> +		2EB3B11C196F095F00ED4429 /* Debug */ = {
> +			isa = XCBuildConfiguration;
> +			buildSettings = {
> +				ALWAYS_SEARCH_USER_PATHS = NO;
> +				CLANG_CXX_LANGUAGE_STANDARD = "gnu++0x";
> +				CLANG_CXX_LIBRARY = "libc++";
> +				CLANG_ENABLE_MODULES = YES;
> +				CLANG_ENABLE_OBJC_ARC = YES;
> +				CLANG_WARN_BOOL_CONVERSION = YES;
> +				CLANG_WARN_CONSTANT_CONVERSION = YES;
> +				CLANG_WARN_DIRECT_OBJC_ISA_USAGE = YES_ERROR;
> +				CLANG_WARN_EMPTY_BODY = YES;
> +				CLANG_WARN_ENUM_CONVERSION = YES;
> +				CLANG_WARN_INT_CONVERSION = YES;
> +				CLANG_WARN_OBJC_ROOT_CLASS = YES_ERROR;
> +				CLANG_WARN_UNREACHABLE_CODE = YES;
> +				CLANG_WARN__DUPLICATE_METHOD_MATCH = YES;
> +				COPY_PHASE_STRIP = NO;
> +				ENABLE_STRICT_OBJC_MSGSEND = YES;
> +				GCC_C_LANGUAGE_STANDARD = gnu99;
> +				GCC_DYNAMIC_NO_PIC = NO;
> +				GCC_OPTIMIZATION_LEVEL = 0;
> +				GCC_PREPROCESSOR_DEFINITIONS = (
> +					"DEBUG=1",
> +					"$(inherited)",
> +				);
> +				GCC_SYMBOLS_PRIVATE_EXTERN = NO;
> +				GCC_WARN_64_TO_32_BIT_CONVERSION = YES;
> +				GCC_WARN_ABOUT_RETURN_TYPE = YES_ERROR;
> +				GCC_WARN_UNDECLARED_SELECTOR = YES;
> +				GCC_WARN_UNINITIALIZED_AUTOS = YES_AGGRESSIVE;
> +				GCC_WARN_UNUSED_FUNCTION = YES;
> +				GCC_WARN_UNUSED_VARIABLE = YES;
> +				MACOSX_DEPLOYMENT_TARGET = 10.10;
> +				MTL_ENABLE_DEBUG_INFO = YES;
> +				ONLY_ACTIVE_ARCH = YES;
> +				SDKROOT = macosx;
> +				SWIFT_OPTIMIZATION_LEVEL = "-Onone";
> +			};
> +			name = Debug;
> +		};
> +		2EB3B11D196F095F00ED4429 /* Release */ = {
> +			isa = XCBuildConfiguration;
> +			buildSettings = {
> +				ALWAYS_SEARCH_USER_PATHS = NO;
> +				CLANG_CXX_LANGUAGE_STANDARD = "gnu++0x";
> +				CLANG_CXX_LIBRARY = "libc++";
> +				CLANG_ENABLE_MODULES = YES;
> +				CLANG_ENABLE_OBJC_ARC = YES;
> +				CLANG_WARN_BOOL_CONVERSION = YES;
> +				CLANG_WARN_CONSTANT_CONVERSION = YES;
> +				CLANG_WARN_DIRECT_OBJC_ISA_USAGE = YES_ERROR;
> +				CLANG_WARN_EMPTY_BODY = YES;
> +				CLANG_WARN_ENUM_CONVERSION = YES;
> +				CLANG_WARN_INT_CONVERSION = YES;
> +				CLANG_WARN_OBJC_ROOT_CLASS = YES_ERROR;
> +				CLANG_WARN_UNREACHABLE_CODE = YES;
> +				CLANG_WARN__DUPLICATE_METHOD_MATCH = YES;
> +				COPY_PHASE_STRIP = YES;
> +				DEBUG_INFORMATION_FORMAT = "dwarf-with-dsym";
> +				ENABLE_NS_ASSERTIONS = NO;
> +				ENABLE_STRICT_OBJC_MSGSEND = YES;
> +				GCC_C_LANGUAGE_STANDARD = gnu99;
> +				GCC_WARN_64_TO_32_BIT_CONVERSION = YES;
> +				GCC_WARN_ABOUT_RETURN_TYPE = YES_ERROR;
> +				GCC_WARN_UNDECLARED_SELECTOR = YES;
> +				GCC_WARN_UNINITIALIZED_AUTOS = YES_AGGRESSIVE;
> +				GCC_WARN_UNUSED_FUNCTION = YES;
> +				GCC_WARN_UNUSED_VARIABLE = YES;
> +				MACOSX_DEPLOYMENT_TARGET = 10.10;
> +				MTL_ENABLE_DEBUG_INFO = NO;
> +				SDKROOT = macosx;
> +			};
> +			name = Release;
> +		};

We should extract these settings out into *.xcconfig files in a Configuration directory like all the other projects.

> Tools/LayoutTestRelay/LayoutTestRelay/Array.swift:26
> +// <rdar://problem/17768202>

We don't have to be so opaque here.  Add a title like:  Swift: Add first and last properties to Array class

> Tools/LayoutTestRelay/LayoutTestRelay/OptionParser.swift:28
> +class OptionParser {

How hard would it be to use plain ol' getopt_long() from C instead?  Seems like a lot of code to re-invent the wheel here, but I'm not sure how much code would be require to wrap the old C wheel.

> Tools/Scripts/build-layouttestrelay:47
> +GetOptions(

You should check the result of GetOptions():

my $result = GetOptions([...]);

> Tools/Scripts/build-layouttestrelay:52
> +if ($showHelp) {

Then check for !$result here:

if ($showHelp || !$result) {

> Tools/Scripts/build-layouttestrelay:63
> +my $result = buildXCodeProject("LayoutTestRelay", $clean, XcodeOptions(), @ARGV);

Don't need 'my $result' here if it is declared above.

[David Kilzer (:ddkilzer)]

(In reply to comment #9)
> There's also an issue of having sufficient reviewer experience to review Swift code.

I guess we'll have to have Chris Lattner review all the code since he's the only one with four years experience writing Swift code:  <https://twitter.com/clattner_llvm/status/473835365137416192>

[David Farler]

(In reply to comment #11)
> (From update of attachment 235576 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235576&action=review
> 
> r=me, but please email webkit-dev first, and consider the feedback.

Will do.

> 
> > Tools/LayoutTestRelay/LayoutTestRelay.xcodeproj/project.pbxproj:262
> We should extract these settings out into *.xcconfig files in a Configuration directory like all the other projects.

Gotcha. Will break this out.

> 
> > Tools/LayoutTestRelay/LayoutTestRelay/Array.swift:26
> > +// <rdar://problem/17768202>
> 
> We don't have to be so opaque here.  Add a title like:  Swift: Add first and last properties to Array class
> 
> > Tools/LayoutTestRelay/LayoutTestRelay/OptionParser.swift:28
> > +class OptionParser {
> 
> How hard would it be to use plain ol' getopt_long() from C instead?  Seems like a lot of code to re-invent the wheel here, but I'm not sure how much code would be require to wrap the old C wheel.

I tried that first and it's extremely cumbersome and ends up being pretty verbose. I knew including something equivalent to a module is a little heavyweight for this tool but I'm going to push for something like this in the Swift standard library because it's pretty important. If using Swift in the WebKit tree becomes more common, I can break this module out further until the standard library builds up.

> 
> > Tools/Scripts/build-layouttestrelay:47
> > +GetOptions(
> 
> You should check the result of GetOptions():
> 
> my $result = GetOptions([...]);
> 
> > Tools/Scripts/build-layouttestrelay:52
> > +if ($showHelp) {
> 
> Then check for !$result here:
> 
> if ($showHelp || !$result) {
> 
> > Tools/Scripts/build-layouttestrelay:63
> > +my $result = buildXCodeProject("LayoutTestRelay", $clean, XcodeOptions(), @ARGV);
> 
> Don't need 'my $result' here if it is declared above.

Thanks - I copied this from another build script we already have, so I should probably fix those as well. :)

[David Farler]

Sent out an e-mail to webkit-dev on this bug and Swift.

[David Farler]

Created attachment 235729 [details]
Patch

[WebKit Commit Bot]

Attachment 235729 [details] did not pass style-queue:


ERROR: Unexpected diff format when parsing a chunk: '//'
ERROR: Unexpected diff format when parsing a chunk: '// Redistribution and use in source and binary forms, with or without'
ERROR: Unexpected diff format when parsing a chunk: '// modification, are permitted provided that the following conditions'
ERROR: Unexpected diff format when parsing a chunk: '// are met:'
ERROR: Unexpected diff format when parsing a chunk: '// 1. Redistributions of source code must retain the above copyright'
ERROR: Unexpected diff format when parsing a chunk: '//    notice, this list of conditions and the following disclaimer.'
ERROR: Unexpected diff format when parsing a chunk: '// 2. Redistributions in binary form must reproduce the above copyright'
ERROR: Unexpected diff format when parsing a chunk: '//    notice, this list of conditions and the following disclaimer in the'
ERROR: Unexpected diff format when parsing a chunk: '//    documentation and/or other materials provided with the distribution.'
ERROR: Unexpected diff format when parsing a chunk: '//'
ERROR: Unexpected diff format when parsing a chunk: "// THIS SOFTWARE IS PROVIDED BY APPLE INC. ``AS IS'' AND ANY"
ERROR: Unexpected diff format when parsing a chunk: '// EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE'
ERROR: Unexpected diff format when parsing a chunk: '// IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR'
ERROR: Unexpected diff format when parsing a chunk: '// PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL APPLE INC. OR'
ERROR: Unexpected diff format when parsing a chunk: '// CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,'
ERROR: Unexpected diff format when parsing a chunk: '// EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,'
ERROR: Unexpected diff format when parsing a chunk: '// PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR'
ERROR: Unexpected diff format when parsing a chunk: '// PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY'
ERROR: Unexpected diff format when parsing a chunk: '// OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT'
ERROR: Unexpected diff format when parsing a chunk: '// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE'
ERROR: Unexpected diff format when parsing a chunk: '// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.'
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: '#include "Base.xcconfig"'
ERROR: Tools/LayoutTestRelay/LayoutTestRelay/LTRelayController.m:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Tools/LayoutTestRelay/LayoutTestRelay/LTRelayController.m:107:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/LayoutTestRelay/LayoutTestRelay/LTPipeRelay.m:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 3 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Farler]

Created attachment 235730 [details]
Patch

[WebKit Commit Bot]

Attachment 235730 [details] did not pass style-queue:


ERROR: Unexpected diff format when parsing a chunk: '//'
ERROR: Unexpected diff format when parsing a chunk: '// Redistribution and use in source and binary forms, with or without'
ERROR: Unexpected diff format when parsing a chunk: '// modification, are permitted provided that the following conditions'
ERROR: Unexpected diff format when parsing a chunk: '// are met:'
ERROR: Unexpected diff format when parsing a chunk: '// 1. Redistributions of source code must retain the above copyright'
ERROR: Unexpected diff format when parsing a chunk: '//    notice, this list of conditions and the following disclaimer.'
ERROR: Unexpected diff format when parsing a chunk: '// 2. Redistributions in binary form must reproduce the above copyright'
ERROR: Unexpected diff format when parsing a chunk: '//    notice, this list of conditions and the following disclaimer in the'
ERROR: Unexpected diff format when parsing a chunk: '//    documentation and/or other materials provided with the distribution.'
ERROR: Unexpected diff format when parsing a chunk: '//'
ERROR: Unexpected diff format when parsing a chunk: "// THIS SOFTWARE IS PROVIDED BY APPLE INC. ``AS IS'' AND ANY"
ERROR: Unexpected diff format when parsing a chunk: '// EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE'
ERROR: Unexpected diff format when parsing a chunk: '// IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR'
ERROR: Unexpected diff format when parsing a chunk: '// PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL APPLE INC. OR'
ERROR: Unexpected diff format when parsing a chunk: '// CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,'
ERROR: Unexpected diff format when parsing a chunk: '// EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,'
ERROR: Unexpected diff format when parsing a chunk: '// PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR'
ERROR: Unexpected diff format when parsing a chunk: '// PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY'
ERROR: Unexpected diff format when parsing a chunk: '// OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT'
ERROR: Unexpected diff format when parsing a chunk: '// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE'
ERROR: Unexpected diff format when parsing a chunk: '// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.'
ERROR: Unexpected diff format when parsing a chunk: ''
ERROR: Unexpected diff format when parsing a chunk: '#include "Base.xcconfig"'
ERROR: Tools/LayoutTestRelay/LayoutTestRelay/LTRelayController.m:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Tools/LayoutTestRelay/LayoutTestRelay/LTRelayController.m:107:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Tools/LayoutTestRelay/LayoutTestRelay/LTPipeRelay.m:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 3 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Farler]

^ I filed https://bugs.webkit.org/show_bug.cgi?id=135408

[David Kilzer (:ddkilzer)]

Comment on attachment 235730 [details]
Patch

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

r=me

> Tools/LayoutTestRelay/Configurations/DebugRelease.xcconfig:25
>  \ No newline at end of file

Nit: Please add a newline.

> Tools/LayoutTestRelay/LayoutTestRelay.xcodeproj/project.pbxproj:184
> +			buildSettings = {
> +				ALWAYS_SEARCH_USER_PATHS = NO;
> +				CLANG_CXX_LANGUAGE_STANDARD = "gnu++0x";
> +				CLANG_CXX_LIBRARY = "libc++";
> +				CLANG_ENABLE_MODULES = YES;
> +				CLANG_ENABLE_OBJC_ARC = YES;
> +				CLANG_WARN_BOOL_CONVERSION = YES;
> +				CLANG_WARN_CONSTANT_CONVERSION = YES;
> +				CLANG_WARN_DIRECT_OBJC_ISA_USAGE = YES_ERROR;
> +				CLANG_WARN_EMPTY_BODY = YES;
> +				CLANG_WARN_ENUM_CONVERSION = YES;
> +				CLANG_WARN_INT_CONVERSION = YES;
> +				CLANG_WARN_OBJC_ROOT_CLASS = YES_ERROR;
> +				CLANG_WARN_UNREACHABLE_CODE = YES;
> +				CLANG_WARN__DUPLICATE_METHOD_MATCH = YES;
> +				COPY_PHASE_STRIP = NO;
> +				ENABLE_STRICT_OBJC_MSGSEND = YES;
> +				GCC_C_LANGUAGE_STANDARD = gnu99;
> +				GCC_DYNAMIC_NO_PIC = NO;
> +				GCC_OPTIMIZATION_LEVEL = 0;
> +				GCC_PREPROCESSOR_DEFINITIONS = (
> +					"DEBUG=1",
> +					"$(inherited)",
> +				);
> +				GCC_SYMBOLS_PRIVATE_EXTERN = NO;
> +				GCC_WARN_64_TO_32_BIT_CONVERSION = YES;
> +				GCC_WARN_ABOUT_RETURN_TYPE = YES_ERROR;
> +				GCC_WARN_UNDECLARED_SELECTOR = YES;
> +				GCC_WARN_UNINITIALIZED_AUTOS = YES_AGGRESSIVE;
> +				GCC_WARN_UNUSED_FUNCTION = YES;
> +				GCC_WARN_UNUSED_VARIABLE = YES;
> +				MACOSX_DEPLOYMENT_TARGET = 10.10;
> +				MTL_ENABLE_DEBUG_INFO = YES;
> +				ONLY_ACTIVE_ARCH = YES;
> +				SDKROOT = macosx;
> +			};

Most of these settings should go into Base.xcconfig.  See Base.xcconfig from other projects.  (I'm sorry.)

> Tools/LayoutTestRelay/LayoutTestRelay.xcodeproj/project.pbxproj:219
> +			buildSettings = {
> +				ALWAYS_SEARCH_USER_PATHS = NO;
> +				CLANG_CXX_LANGUAGE_STANDARD = "gnu++0x";
> +				CLANG_CXX_LIBRARY = "libc++";
> +				CLANG_ENABLE_MODULES = YES;
> +				CLANG_ENABLE_OBJC_ARC = YES;
> +				CLANG_WARN_BOOL_CONVERSION = YES;
> +				CLANG_WARN_CONSTANT_CONVERSION = YES;
> +				CLANG_WARN_DIRECT_OBJC_ISA_USAGE = YES_ERROR;
> +				CLANG_WARN_EMPTY_BODY = YES;
> +				CLANG_WARN_ENUM_CONVERSION = YES;
> +				CLANG_WARN_INT_CONVERSION = YES;
> +				CLANG_WARN_OBJC_ROOT_CLASS = YES_ERROR;
> +				CLANG_WARN_UNREACHABLE_CODE = YES;
> +				CLANG_WARN__DUPLICATE_METHOD_MATCH = YES;
> +				COPY_PHASE_STRIP = YES;
> +				DEBUG_INFORMATION_FORMAT = "dwarf-with-dsym";
> +				ENABLE_NS_ASSERTIONS = NO;
> +				ENABLE_STRICT_OBJC_MSGSEND = YES;
> +				GCC_C_LANGUAGE_STANDARD = gnu99;
> +				GCC_WARN_64_TO_32_BIT_CONVERSION = YES;
> +				GCC_WARN_ABOUT_RETURN_TYPE = YES_ERROR;
> +				GCC_WARN_UNDECLARED_SELECTOR = YES;
> +				GCC_WARN_UNINITIALIZED_AUTOS = YES_AGGRESSIVE;
> +				GCC_WARN_UNUSED_FUNCTION = YES;
> +				GCC_WARN_UNUSED_VARIABLE = YES;
> +				MACOSX_DEPLOYMENT_TARGET = 10.10;
> +				MTL_ENABLE_DEBUG_INFO = NO;
> +				SDKROOT = macosx;
> +			};

Ditto.

> Tools/LayoutTestRelay/LayoutTestRelay/LTPipeRelay.m:80
> +    _standardErrorPipe = [NSInputStream inputStreamWithFileAtPath: [self errorPipePath]];

Nit: Space after ":".

> Tools/LayoutTestRelay/LayoutTestRelay/LTPipeRelay.m:143
> +    default:
> +        break;

Did we cover all the enum values of NSStreamEvent?  If so, do we need a default: item?

> Tools/LayoutTestRelay/LayoutTestRelay/LTRelay.h:33
> +- (void)connected;
> +- (void)disconnected;

Should these be -didConnect and -didDisconnect instead?

> Tools/LayoutTestRelay/LayoutTestRelay/LTRelay.h:36
> +- (void)crashWithMessage:(NSString *)message;
> +- (void)receivedStandardOutputData:(NSData *)data;
> +- (void)receivedStandardErrorData:(NSData *)data;

Similarly, should these be -didVerb* instead of -verbed*?

> Tools/LayoutTestRelay/LayoutTestRelay/LTRelayController.m:98
> +- (void) receivedStandardErrorData:(NSData *)data {

Nit:  Space after "(void)".

> Tools/LayoutTestRelay/LayoutTestRelay/LTRelayController.m:122
> +    exit(-777);

Hmm, that looks special!  What does it mean?  (Maybe define as a constant with a descriptive name?)
How is this different from EXIT_FAILURE?

> Tools/LayoutTestRelay/LayoutTestRelay/LTRelayController.m:131
> +        [xcodeSelectTask setStandardOutput: [NSPipe pipe]];

Nit: Space after ":".

> Tools/LayoutTestRelay/LayoutTestRelay/LTRelayController.m:134
> +        [xcodeSelectTask launch];

Do we have to close/stop/call a method before releasing the NSTask to clean it up?

> Tools/LayoutTestRelay/LayoutTestRelay/LTRelayController.m:157
> +        fprintf(stderr, "Couldn't launch iOS Simulator from %s: %s\n", [[simulatorURL path] UTF8String], [[error description] UTF8String]);

Just noticed you used fprintf() everywhere.  Why not use NSLog instead?  Then you can use "%@" for NSString instead of "%s" plus -UTF8String all the time.

> Tools/LayoutTestRelay/LayoutTestRelay/LTRelayController.m:194
> +    NSString *infoPlistPath = [[self uniqueAppPath] stringByAppendingPathComponent:@"/Info.plist"];

Do you need the "/" here when adding a path component?

> Tools/LayoutTestRelay/LayoutTestRelay/LTRelayController.m:208
> +    if ([[self device] applicationIsInstalled:[self uniqueAppIdentifier] type: nil error: &error]) {
> +        BOOL uninstalled = [[self device ] uninstallApplication:[self uniqueAppIdentifier] withOptions: nil error: &error];

Nit: Spaces after ":" here.

> Tools/LayoutTestRelay/LayoutTestRelay/LTRelayController.m:245
> +    pid_t pid = [[self device] launchApplicationWithID:[self uniqueAppIdentifier] options:launchOptions error: &error];

Nit: Space after ":".

> Tools/LayoutTestRelay/LayoutTestRelay/main.m:76
> +        fprintf(stderr, "--%s is required.\n", [parameter UTF8String]);

The help message above uses single dashes, while this error message uses double-dashes for arguments.  Which is correct?

> Tools/LayoutTestRelay/LayoutTestRelay/main.m:117
> +            fprintf(stderr, "There is no supporte device type \"%s\"\n", [deviceTypeIdentifier UTF8String]);

Type:  supporte => supported

> Tools/Scripts/build-layouttestrelay:47
> +GetOptions(
> +    'help' => \$showHelp,
> +    'clean' => \$clean,
> +);

Still would like to check the result of GetOptions() here:

my $result = GetOptions(

> Tools/Scripts/build-layouttestrelay:49
> +if ($showHelp) {

Then:

if (!$result || $showHelp) {

> Tools/Scripts/build-layouttestrelay:60
> +my $result;

This doesn't need to be declared if you declare it above first for GetOptions().

[Simon Fraser (smfr)]

Comment on attachment 235730 [details]
Patch

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

> Tools/LayoutTestRelay/LayoutTestRelay/LTPipeRelay.m:58
> +- (NSString *)inPipePath {
> +    return [_fifoPrefix stringByAppendingString:@"_IN"];
> +}
> +
> +- (NSString *)outPipePath {
> +    return [_fifoPrefix stringByAppendingString:@"_OUT"];
> +}
> +
> +- (NSString *)errorPipePath {
> +    return [_fifoPrefix stringByAppendingString:@"_ERROR"];
> +}
> +
> +- (NSOutputStream *) outputStream {
> +    return _standardInputPipe;
> +}

The opening paren should go on a new line for all functions (Obj-C, C and C++).

> Tools/LayoutTestRelay/LayoutTestRelay/LTPipeRelay.m:87
> +    [[self standardOutputPipe] scheduleInRunLoop:[NSRunLoop mainRunLoop] forMode:NSDefaultRunLoopMode];
> +    [[self standardErrorPipe] scheduleInRunLoop:[NSRunLoop mainRunLoop] forMode:NSDefaultRunLoopMode];

No need to schedule the standardInput pipe?

> Tools/LayoutTestRelay/LayoutTestRelay/LTPipeRelay.m:96
> +- (void) disconnect {

No space after )

> Tools/LayoutTestRelay/LayoutTestRelay/LTRelayController.h:30
> +#import <CoreSimulator/CoreSimulator.h>

You should not include this here, and just forward declare the SimDevice class.

> Tools/LayoutTestRelay/LayoutTestRelay/LTRelayController.m:67
> +- (id)initWithDevice:(SimDevice *)device productDir:(NSString *)productDir appPath:(NSString *)appPath identifierSuffix:(NSString *)suffix dumpToolArguments:(NSArray *)arguments {

I prefer init methods to be first in the file.

> Tools/LayoutTestRelay/LayoutTestRelay/LTRelayController.m:112
> +- (void)crashWithMessage:(NSString *)message {

This sounds like it would make _this_ process crash. Should be didCrashWithMessage:?

> Tools/LayoutTestRelay/LayoutTestRelay/LTRelayController.m:146
> +    NSURL *simulatorURL = [NSURL fileURLWithPath:[developerDir stringByAppendingPathComponent:@"/Applications/iOS Simulator.app"]];

Pretty sure you don't need the leading / on the path component.

> Tools/LayoutTestRelay/LayoutTestRelay/main.m:83
> +NSArray *getRemainderArguments()

"remainder" is unclear.

> Tools/LayoutTestRelay/LayoutTestRelay/main.m:102
> +        if ([[[NSProcessInfo processInfo] arguments] indexOfObject:@"-h"] != NSNotFound) {

Make --help work too?

[David Farler]

Thanks to you both - I've adjusted for all of your comments. I'll respond to open questions for clarity.

[David Farler]

> Did we cover all the enum values of NSStreamEvent?  If so, do we need a default: item?

Yep. Seems fine without it.

> 
> > Tools/LayoutTestRelay/LayoutTestRelay/LTRelay.h:33
> > +- (void)connected;
> > +- (void)disconnected;
> 
> Should these be -didConnect and -didDisconnect instead?
> Similarly, should these be -didVerb* instead of -verbed*?

I think so, yeah.

> > Tools/LayoutTestRelay/LayoutTestRelay/LTRelayController.m:122
> > +    exit(-777);
> 
> Hmm, that looks special!  What does it mean?  (Maybe define as a constant with a descriptive name?)
> How is this different from EXIT_FAILURE?

Heh, that was a testing value that I forgot to remove ("bad luck"). I have a different mechanism for detecting crashes so I moved this back to EXIT_FAILURE.

> 
> > Tools/LayoutTestRelay/LayoutTestRelay/LTRelayController.m:131
> > +        [xcodeSelectTask setStandardOutput: [NSPipe pipe]];
> 
> Nit: Space after ":".
> 
> > Tools/LayoutTestRelay/LayoutTestRelay/LTRelayController.m:134
> > +        [xcodeSelectTask launch];
> 
> Do we have to close/stop/call a method before releasing the NSTask to clean it up?

No, but I did add a waitUntilExit call. Checking the rc is useless (it's always 0).

> 
> > Tools/LayoutTestRelay/LayoutTestRelay/LTRelayController.m:157
> > +        fprintf(stderr, "Couldn't launch iOS Simulator from %s: %s\n", [[simulatorURL path] UTF8String], [[error description] UTF8String]);
> 
> Just noticed you used fprintf() everywhere.  Why not use NSLog instead?  Then you can use "%@" for NSString instead of "%s" plus -UTF8String all the time.

That's a good point. I originally had this to remove the timestamp but actually it's probably a good thing to have it (agrees with my "more information" stance).

> > Tools/LayoutTestRelay/LayoutTestRelay/LTRelayController.m:194
> > +    NSString *infoPlistPath = [[self uniqueAppPath] stringByAppendingPathComponent:@"/Info.plist"];
> 
> Do you need the "/" here when adding a path component?

Checking the docs, looks like not.

> The help message above uses single dashes, while this error message uses double-dashes for arguments.  Which is correct?

Good catch - it should be single dash now.

[David Farler]

(In reply to comment #21)
> No need to schedule the standardInput pipe?

Nope, that's the dump tool's stdin - we write to it. There is a scheduled handler for the actual relay's stdin, and I immediately write to the dump tool's. 
 
> > Tools/LayoutTestRelay/LayoutTestRelay/LTRelayController.m:112
> > +- (void)crashWithMessage:(NSString *)message {
> 
> This sounds like it would make _this_ process crash. Should be didCrashWithMessage:?

Yeah. In a way, it is sort of making the relay pretend-crash but we don't really care why since it's always in response to the dump tool itself crashing. I changed all of the delegate methods to match the style.

[David Farler]

Committed r171800: <http://trac.webkit.org/changeset/171800>