Bug for new tool to relay DumpRenderTree / WebKitTestRunner IO in the simulator.
<rdar://problem/10864368>
Created attachment 235576 [details] Patch
Thanks to Matt Wright for providing most of the OptionParser code.
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
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
I think you should email webkit-dev and ask if people are OK with using Swift in the webkit repository.
(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.
(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!
There's also an issue of having sufficient reviewer experience to review Swift code.
(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.
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.
(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>
(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. :)
Sent out an e-mail to webkit-dev on this bug and Swift.
Created attachment 235729 [details] Patch
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.
Created attachment 235730 [details] Patch
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.
^ I filed https://bugs.webkit.org/show_bug.cgi?id=135408
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().
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?
Thanks to you both - I've adjusted for all of your comments. I'll respond to open questions for clarity.
> 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.
(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.
Committed r171800: <http://trac.webkit.org/changeset/171800>