Bug 189228 – [Cocoa] Turn on ARC for WebKitTestRunner

• Status: ASSIGNED
• Component: Tools / Tests

[Darin Adler]

[Cocoa] Turn on ARC for WebKitTestRunner

[Darin Adler]

Created attachment 348729 [details]
Patch

[Darin Adler]

Created attachment 348746 [details]
Patch

[Darin Adler]

Created attachment 348777 [details]
Patch

[Sam Weinig]

Comment on attachment 348777 [details]
Patch

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

> Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityTextMarkerIOS.mm:32
> +    return [(__bridge id)platformTextMarker() isEqual:(__bridge id)other->platformTextMarker()];

Is there any way to make platformTextMarker() return (or rather make PlatformTextMarker) an id rather than CFTypeRef? I know in WebCore we use the trick of doing something like:

#ifndef __OBJC__
typedef struct objc_object *id;
#endif

in the C++ class. Though I am not sure what the implications for ARC are of that.

> Tools/WebKitTestRunner/InjectedBundle/ios/AccessibilityUIElementIOS.mm:1066
> +    return AccessibilityTextMarkerRange::create((__bridge PlatformTextMarkerRange)textMarkerRange);

Same question as above, but for PlatformTextMarkerRange.

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:1825
> -    id mutableString = [[[NSMutableString alloc] init] autorelease];
> +    id mutableString = [[NSMutableString alloc] init];

Seems like this could be NSMutableString * rather than id (as long as you are changing this line).

> Tools/WebKitTestRunner/ios/UIScriptControllerIOS.mm:138
> +    return JSStringCreateWithCFString((__bridge CFStringRef)webView.accessibilitySpeakSelectionContent);

There is a category in WebKitTestRunner that adds a  -[NSString createJSStringRef] (and a +[NSString stringWithJSStringRef:]). It is currently in AccessibilityCommonMac.h, but seems more generally useful (could probably add ones for WKStringRef as well if we were going wild).

[Darin Adler]

Committed r235832: <https://trac.webkit.org/changeset/235832>

[Radar WebKit Bug Importer]

<rdar://problem/44264628>

[Alexey Proskuryakov]

This patch broke WebKit2 tests, because it apparently makes system resources leak. Rolling back.

EWS detected this before landing.


worker/10: OSError('[Errno 35] Resource temporarily unavailable') raised:
    File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/common/message_pool.py", line 257, in run
      worker.handle(message.name, message.src, *message.args)
    File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py", line 283, in handle
...

[WebKit Commit Bot]

Re-opened since this is blocked by bug 189464

[Darin Adler]

This did not break WebKit2 tests on my computer.

[Darin Adler]

(In reply to Alexey Proskuryakov from comment #7)
> This patch broke WebKit2 tests, because it apparently makes system resources
> leak. Rolling back.
> 
> EWS detected this before landing.
> 
> 
> worker/10: OSError('[Errno 35] Resource temporarily unavailable') raised:
>     File
> "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/common/message_pool.py",
> line 257, in run
>       worker.handle(message.name, message.src, *message.args)
>     File
> "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/
> layout_test_runner.py", line 283, in handle
> ...

I’m accustomed to seeing this on EWS when there is a server problem, which is why I didn’t realize it was a problem with the patch.

I guess I can abandon this for now because I’m not sure what to do next since I don’t see any problem locally.

[Alexey Proskuryakov]

Watching open files and processes with lsof and ps would likely be the most straightforward way to see what's going on locally.

It's also possible that this is something specific to Sierra and the version of clang used there. This doesn't feel like a very likely cause to me though.

[Darin Adler]

Thanks. I will try your suggestion next time I have an opportunity to work on this.