[webkit-reviews] review granted: [Bug 135272] Allow for multiple DumpRenderTree and WebKitTestRunner instances in the iOS Simulator : [Attachment 235477] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted David Farler
<dfarler at apple.com>'s request for review:
Bug 135272: Allow for multiple DumpRenderTree and WebKitTestRunner instances in
the iOS Simulator
https://bugs.webkit.org/show_bug.cgi?id=135272

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=235477&action=review


> Tools/DumpRenderTree/mac/DumpRenderTree.mm:1126
> +    const char *stdoutPath = [[NSString stringWithFormat:@"/tmp/%@_OUT",
identifier] UTF8String];
> +    const char *stderrPath = [[NSString stringWithFormat:@"/tmp/%@_ERROR",
identifier] UTF8String];
> +

Do we not need to unique these paths per DRT instance if several are running at
once?

> Tools/DumpRenderTree/mac/DumpRenderTree.mm:1249
> +	   NSTimeInterval timeRemaining;
> +	   while (true) {
> +	       timeRemaining = [application backgroundTimeRemaining];
> +	       if (timeRemaining <= 10.0 || bgTask == UIBackgroundTaskInvalid)
{
> +		   [application endBackgroundTask:bgTask];
> +		   bgTask = [application
beginBackgroundTaskWithExpirationHandler:expirationHandler];
> +	       }
> +	       sleep(5);

Why is all this necessary?

> Tools/DumpRenderTree/mac/DumpRenderTreeMac.h:80
> +    UIBackgroundTaskIdentifier bgTask;

bgTask -> backgroundTaskIdentifier.

> Tools/WebKitTestRunner/ios/mainIOS.mm:35
> +    UIBackgroundTaskIdentifier bgTask;

backgroundTaskIdentifier

> Tools/WebKitTestRunner/ios/mainIOS.mm:68
> +   
dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0),
^{
> +
> +	   NSTimeInterval timeRemaining;
> +	   while (true) {
> +	       timeRemaining = [application backgroundTimeRemaining];
> +	       if (timeRemaining <= 10.0 || bgTask == UIBackgroundTaskInvalid)
{
> +		   [application endBackgroundTask:bgTask];
> +		   bgTask = [application
beginBackgroundTaskWithExpirationHandler:expirationHandler];
> +	       }
> +	       sleep(5);
> +	   }

Again, an explanatory comment would be useful.

> Tools/WebKitTestRunner/ios/mainIOS.mm:79
> +    UIApplicationMain(argc, (char**)argv,
NSStringFromClass([WebKitTestRunnerApp class]),
NSStringFromClass([WebKitTestRunnerApp class]));

Seems odd not to just say @"WebKitTestRunnerApp" here.