[webkit-reviews] review granted: [Bug 135269] iOS Simulator LayoutTestRelay : [Attachment 235730] Patch

[bugzilla-daemon at webkit.org]

David Kilzer (:ddkilzer) <ddkilzer at webkit.org> has granted David Farler
<dfarler at apple.com>'s request for review:
Bug 135269: iOS Simulator LayoutTestRelay
https://bugs.webkit.org/show_bug.cgi?id=135269

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

------- Additional Comments from David Kilzer (:ddkilzer) <ddkilzer at webkit.org>
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().