[Cocoa] More tweaks and refactoring to prepare for ARC
Created attachment 346387 [details] Patch
Wenson, added you because I think you are the original author of the WebImmediateActionController code.
Attachment 346387 [details] did not pass style-queue: ERROR: Source/WebKitLegacy/mac/Plugins/WebBasePluginPackage.mm:134: The parameter name "id" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Tools/WebKitTestRunner/mac/WebKitTestRunnerWindow.mm:36: Should not have spaces around = in property synthesis. [whitespace/property] [4] ERROR: Tools/DumpRenderTree/mac/DumpRenderTreePasteboard.mm:199: The parameter name "data" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 3 in 68 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to Darin Adler from comment #2) > Wenson, added you because I think you are the original author of the > WebImmediateActionController code. This file predates me, but I can still help look at this and some of the pasteboard-related changes.
Comment on attachment 346387 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346387&action=review > Source/JavaScriptCore/API/JSValue.mm:639 > + HashMap<JSValueRef, CFTypeRef> m_objectMap; What if we just changed the type here from id to __unsafe_unretained id and didn’t any of the casts below? Would that work? What are the pros and cons?
Comment on attachment 346387 [details] Patch Attachment 346387 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/8739497 New failing tests: stress/ftl-put-by-id-setter-exception-interesting-live-state.js.ftl-eager-no-cjit apiTests
Comment on attachment 346387 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346387&action=review Looks like there is a missing include of <cstdlib> in WKCrashReporter.mm causing compilation problems so it didn’t run tests for me. >> Source/JavaScriptCore/API/JSValue.mm:639 >> + HashMap<JSValueRef, CFTypeRef> m_objectMap; > > What if we just changed the type here from id to __unsafe_unretained id and didn’t any of the casts below? Would that work? What are the pros and cons? I believe I tried it and it didn’t "just work". I am willing to try again. And even if it doesn’t work immediately I can easily imagine someone fixing this so it works with changes to the HashMap template family. But I think I’m pretty happy with just using CFTypeRef for now; can always come back and make it more elegant if we get it working. Already did that for all the cases inside WebCore in a previous recent patch.
Created attachment 346479 [details] Patch
Attachment 346479 [details] did not pass style-queue: ERROR: Source/WebKitLegacy/mac/Plugins/WebBasePluginPackage.mm:134: The parameter name "id" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Tools/WebKitTestRunner/mac/WebKitTestRunnerWindow.mm:36: Should not have spaces around = in property synthesis. [whitespace/property] [4] ERROR: Tools/DumpRenderTree/mac/DumpRenderTreePasteboard.mm:199: The parameter name "data" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 3 in 68 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 346483 [details] Patch
Attachment 346483 [details] did not pass style-queue: ERROR: Source/WebKitLegacy/mac/Plugins/WebBasePluginPackage.mm:134: The parameter name "id" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Tools/WebKitTestRunner/mac/WebKitTestRunnerWindow.mm:36: Should not have spaces around = in property synthesis. [whitespace/property] [4] ERROR: Tools/DumpRenderTree/mac/DumpRenderTreePasteboard.mm:199: The parameter name "data" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 3 in 68 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 346387 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346387&action=review >>> Source/JavaScriptCore/API/JSValue.mm:639 >>> + HashMap<JSValueRef, CFTypeRef> m_objectMap; >> >> What if we just changed the type here from id to __unsafe_unretained id and didn’t any of the casts below? Would that work? What are the pros and cons? > > I believe I tried it and it didn’t "just work". I am willing to try again. And even if it doesn’t work immediately I can easily imagine someone fixing this so it works with changes to the HashMap template family. But I think I’m pretty happy with just using CFTypeRef for now; can always come back and make it more elegant if we get it working. Already did that for all the cases inside WebCore in a previous recent patch. I was wrong. Tried it again and it seemed to work fine under ARC. Maybe I am confusing this case with use of object types as keys in maps, rather than values. I suppose it’s better to use __unsafe_unretained and do fewer type casts. When there is CFRetain/Release/Autorelease involved I think I prefer using the CF types directly, but doesn't seem to be the case here. Now I am wondering if I missed opportunities to do it this way elsewhere.
(In reply to Darin Adler from comment #12) > Maybe I am confusing this case with use of object types as keys in maps Yes, that was it. Trying to use "__unsafe_unretained id" as the type for the *key* of a HashMap results in HashTraits=related compilation errors.
(In reply to Darin Adler from comment #13) > (In reply to Darin Adler from comment #12) > > Maybe I am confusing this case with use of object types as keys in maps > > Yes, that was it. Trying to use "__unsafe_unretained id" as the type for the > *key* of a HashMap results in HashTraits=related compilation errors. I’ve been able to make "__unsafe_unretained id" work as a key with HashTraits and DefaultHashFunction specializations. I have not yet been able to make "__unsafe_unretained NSObject *" work.
Comment on attachment 346483 [details] Patch Attachment 346483 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/8751368 New failing tests: fast/dom/Geolocation/success.html fast/dom/Geolocation/reentrant-error.html fast/dom/Geolocation/error.html fast/dom/Geolocation/srcdoc-getCurrentPosition.html fast/dom/Geolocation/callback-to-deleted-context.html fast/history/page-cache-geolocation-active-oneshot.html fast/dom/Geolocation/cached-position-iframe.html fast/dom/Geolocation/callback-to-remote-context2.html fast/dom/Geolocation/position-string.html fast/dom/Geolocation/success-clear-watch.html fast/dom/Geolocation/delayed-permission-allowed.html fast/dom/Geolocation/floorLevel.html fast/dom/Geolocation/error-clear-watch.html fast/dom/Geolocation/reentrant-success.html fast/dom/Geolocation/timeout.html fast/dom/Geolocation/timestamp.html fast/dom/Geolocation/callback-exception.html fast/dom/Geolocation/maximum-age.html fast/dom/Geolocation/callback-to-remote-context.html
Created attachment 346499 [details] Archive of layout-test-results from ews115 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 346483 [details] Patch Attachment 346483 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/8753164 New failing tests: fast/dom/Geolocation/success.html fast/dom/Geolocation/reentrant-error.html fast/dom/Geolocation/error.html fast/dom/Geolocation/srcdoc-getCurrentPosition.html fast/dom/Geolocation/callback-to-deleted-context.html fast/history/page-cache-geolocation-active-oneshot.html fast/dom/Geolocation/cached-position-iframe.html fast/dom/Geolocation/callback-to-remote-context2.html fast/dom/Geolocation/position-string.html fast/dom/Geolocation/success-clear-watch.html fast/dom/Geolocation/delayed-permission-allowed.html fast/dom/Geolocation/floorLevel.html fast/dom/Geolocation/error-clear-watch.html fast/dom/Geolocation/reentrant-success.html fast/dom/Geolocation/timeout.html fast/dom/Geolocation/timestamp.html fast/dom/Geolocation/callback-exception.html fast/dom/Geolocation/maximum-age.html fast/dom/Geolocation/callback-to-remote-context.html
Created attachment 346510 [details] Archive of layout-test-results from ews112 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 346483 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346483&action=review > Tools/DumpRenderTree/mac/MockGeolocationProvider.mm:128 > // Expect that views won't be (un)registered while iterating. > - HashSet<WebView*> views = _registeredViews; > - for (HashSet<WebView*>::iterator iter = views.begin(); iter != views.end(); ++iter) { > + for (auto typelessView : _registeredViews) { Oops, reading that comment "expect that" steered me wrong; I thought it was making a true statement about the behavior of the code, when it’s actually something other than that. My next version of this patch will restore the copying of the set, or copying into a Vector at least. I don’t fully understand how this works correctly. If views are unregistering, we will still iterate them since we copied the set, which is not good. But I’ll just restore the old behavior. This is test code so it can’t cause a security vulnerability or other serious issue like that.
Comment on attachment 346483 [details] Patch Attachment 346483 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/8755184 New failing tests: http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-video.html
Created attachment 346557 [details] Archive of layout-test-results from ews200 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment on attachment 346483 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346483&action=review > Source/WTF/wtf/BlockPtr.h:28 > +#include "StdLibExtras.h" Should probably say <wtf/StdLibExtras.h> like all other library includes in this library header. > Source/WTF/wtf/WeakObjCPtr.h:100 > +#if defined(__OBJC__) && __has_feature(objc_arc) We’ve determined that we don’t need the defined(__OBJC__) part. > Source/WTF/wtf/WeakObjCPtr.h:101 > + mutable __weak id m_weakReference; I worry that with the member declared __weak, the compiler is going to emit code that duplicates the work our implementation does. Either we should declare this __autoreleasing (I think) and keep our code that calls the runtime functions, or we should declare this __weak but replace our calls (under the __has_feature(objc_arc) condition) with bare assignments.
Comment on attachment 346483 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346483&action=review >> Source/WTF/wtf/BlockPtr.h:28 >> +#include "StdLibExtras.h" > > Should probably say <wtf/StdLibExtras.h> like all other library includes in this library header. OK, done. >> Source/WTF/wtf/WeakObjCPtr.h:100 >> +#if defined(__OBJC__) && __has_feature(objc_arc) > > We’ve determined that we don’t need the defined(__OBJC__) part. Done. >> Source/WTF/wtf/WeakObjCPtr.h:101 >> + mutable __weak id m_weakReference; > > I worry that with the member declared __weak, the compiler is going to emit code that duplicates the work our implementation does. Either we should declare this __autoreleasing (I think) and keep our code that calls the runtime functions, or we should declare this __weak but replace our calls (under the __has_feature(objc_arc) condition) with bare assignments. I’ll do the latter.
Created attachment 346603 [details] Patch
Attachment 346603 [details] did not pass style-queue: ERROR: Source/WebKitLegacy/mac/Plugins/WebBasePluginPackage.mm:134: The parameter name "id" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Tools/WebKitTestRunner/mac/WebKitTestRunnerWindow.mm:36: Should not have spaces around = in property synthesis. [whitespace/property] [4] ERROR: Source/WTF/wtf/HashFunctions.h:247: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/spi/cocoa/FoundationSPI.h:39: objc_loadWeakRetained is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WTF/wtf/spi/cocoa/FoundationSPI.h:40: objc_initWeak is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WTF/wtf/spi/cocoa/FoundationSPI.h:41: objc_destroyWeak is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WTF/wtf/spi/cocoa/FoundationSPI.h:42: objc_copyWeak is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WTF/wtf/spi/cocoa/FoundationSPI.h:43: objc_moveWeak is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Tools/DumpRenderTree/mac/DumpRenderTreePasteboard.mm:199: The parameter name "data" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 9 in 71 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 346604 [details] Patch
Attachment 346604 [details] did not pass style-queue: ERROR: Source/WebKitLegacy/mac/Plugins/WebBasePluginPackage.mm:134: The parameter name "id" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Tools/WebKitTestRunner/mac/WebKitTestRunnerWindow.mm:36: Should not have spaces around = in property synthesis. [whitespace/property] [4] ERROR: Source/WTF/wtf/HashFunctions.h:247: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/spi/cocoa/FoundationSPI.h:40: objc_loadWeakRetained is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WTF/wtf/spi/cocoa/FoundationSPI.h:41: objc_initWeak is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WTF/wtf/spi/cocoa/FoundationSPI.h:42: objc_destroyWeak is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WTF/wtf/spi/cocoa/FoundationSPI.h:43: objc_copyWeak is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WTF/wtf/spi/cocoa/FoundationSPI.h:44: objc_moveWeak is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Tools/DumpRenderTree/mac/DumpRenderTreePasteboard.mm:199: The parameter name "data" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 9 in 71 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 346605 [details] Patch
Attachment 346605 [details] did not pass style-queue: ERROR: Source/WebKitLegacy/mac/Plugins/WebBasePluginPackage.mm:134: The parameter name "id" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Tools/WebKitTestRunner/mac/WebKitTestRunnerWindow.mm:36: Should not have spaces around = in property synthesis. [whitespace/property] [4] ERROR: Source/WTF/wtf/HashFunctions.h:247: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WTF/wtf/spi/cocoa/FoundationSPI.h:40: objc_loadWeakRetained is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WTF/wtf/spi/cocoa/FoundationSPI.h:41: objc_initWeak is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WTF/wtf/spi/cocoa/FoundationSPI.h:42: objc_destroyWeak is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WTF/wtf/spi/cocoa/FoundationSPI.h:43: objc_copyWeak is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WTF/wtf/spi/cocoa/FoundationSPI.h:44: objc_moveWeak is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Tools/DumpRenderTree/mac/DumpRenderTreePasteboard.mm:199: The parameter name "data" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 9 in 71 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 346605 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346605&action=review Reviewed the Source changes but none of the Tools changes. One thing seemed potentially incorrect. > Source/WTF/wtf/spi/cocoa/FoundationSPI.h:47 > +WTF_EXTERN_C_END Not new to this patch, but I’d have named this header objcSPI.h (like the one in WebKit). > Source/WebKitLegacy/mac/History/WebHistory.mm:647 > + static NSData *emptyHistoryData = [[NSData alloc] initWithBytes:nullptr length:0]; Can also use -init. > Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:85 > + result = CFBridgingRelease(CFStringCreateWithCharactersNoCopy(nullptr, uniCharPtr, len, nullptr)); I’d replace the first nullptr with kCFAllocatorDefault like in some of the above code, but I don’t think there’s a style guideline either way. > Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:91 > - result = [self retain]; > + result = self; > > - return [result autorelease]; > + return result; This introduces a slight change in behavior under manual retain/release. Not a problem since the only caller is in this file and doesn’t deallocate the receiver right after calling this method. > Source/WebKitLegacy/mac/WebView/WebImmediateActionController.mm:148 > - Frame* coreFrame = core([[[[_webView _selectedOrMainFrame] frameView] documentView] _frame]); > + Frame* coreFrame = [_webView _mainCoreFrame]; Seems like this won’t do the right thing if the selection or focus is in a subframe. Unless WebCore hit-testing now traverses frames? Even then, it seems wrong.
Comment on attachment 346605 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346605&action=review Since the Tools part is completely separable from the non-Tools part, it’s OK if I just get a review on one or the other. I can land that half and open another bug for the other half. >> Source/WTF/wtf/spi/cocoa/FoundationSPI.h:47 >> +WTF_EXTERN_C_END > > Not new to this patch, but I’d have named this header objcSPI.h (like the one in WebKit). OK, I’ll rename and upload a new version of the patch. >> Source/WebKitLegacy/mac/History/WebHistory.mm:647 >> + static NSData *emptyHistoryData = [[NSData alloc] initWithBytes:nullptr length:0]; > > Can also use -init. Will do. >> Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:85 >> + result = CFBridgingRelease(CFStringCreateWithCharactersNoCopy(nullptr, uniCharPtr, len, nullptr)); > > I’d replace the first nullptr with kCFAllocatorDefault like in some of the above code, but I don’t think there’s a style guideline either way. Sure, OK, why not? >> Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:91 >> + return result; > > This introduces a slight change in behavior under manual retain/release. Not a problem since the only caller is in this file and doesn’t deallocate the receiver right after calling this method. I think I’ll leave it like this. Otherwise, the alternative I would probably choose would be a copy/autorelease in the "result = self" line above. Even under ARC I could imagine returning a copy rather than self. But I don’t want to risk a small performance regression without a compelling reason. >> Source/WebKitLegacy/mac/WebView/WebImmediateActionController.mm:148 >> + Frame* coreFrame = [_webView _mainCoreFrame]; > > Seems like this won’t do the right thing if the selection or focus is in a subframe. Unless WebCore hit-testing now traverses frames? Even then, it seems wrong. I see. I was under the incorrect impression that the frame was only to work with the immediate action stage, like the other call sites. But I see now that it’s also used for hit testing. I will restore the old code for the hit testing purposes.
Created attachment 346629 [details] Patch
Created attachment 346630 [details] Patch
Attachment 346630 [details] did not pass style-queue: ERROR: Source/WebKitLegacy/mac/Plugins/WebBasePluginPackage.mm:134: The parameter name "id" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Tools/WebKitTestRunner/mac/WebKitTestRunnerWindow.mm:36: Should not have spaces around = in property synthesis. [whitespace/property] [4] ERROR: Source/WTF/wtf/HashFunctions.h:247: More than one command on the same line [whitespace/newline] [4] ERROR: Tools/DumpRenderTree/mac/DumpRenderTreePasteboard.mm:199: The parameter name "data" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WTF/wtf/spi/cocoa/objcSPI.h:36: objc_autoreleasePoolPush is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WTF/wtf/spi/cocoa/objcSPI.h:37: objc_autoreleasePoolPop is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WTF/wtf/spi/cocoa/objcSPI.h:40: objc_loadWeakRetained is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WTF/wtf/spi/cocoa/objcSPI.h:41: objc_initWeak is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WTF/wtf/spi/cocoa/objcSPI.h:42: objc_destroyWeak is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WTF/wtf/spi/cocoa/objcSPI.h:43: objc_copyWeak is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] ERROR: Source/WTF/wtf/spi/cocoa/objcSPI.h:44: objc_moveWeak is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 11 in 79 files If any of these errors are false positives, please file a bug against check-webkit-style.
Latest patch has passed tests on all the EWS servers (except for Windows which is failing do to lack of disk space) and has been updated based on the comments Dan made in his review. Waiting on further review before I land.
Comment on attachment 346630 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346630&action=review > Tools/ChangeLog:53 > + (dumpBackForwardListForAllWindows): User a modern for loop instead of Typo: “User” > Tools/DumpRenderTree/mac/MockGeolocationProvider.mm:130 > + auto webView = (__bridge WebView*)typelessView; Missing space before the *. > Tools/TestRunnerShared/cocoa/LayoutTestSpellChecker.mm:254 > + [resultsForWord addObject:[[LayoutTestTextCheckingResult alloc] initWithType:nsTextCheckingType(WTFMove(typeValue)) range:NSMakeRange(fromValue, toValue - fromValue) replacement:(__bridge NSString *)replacementText.get() details:details.get()]]; Isn’t this just leaking now when not using ARC? > Tools/WebKitTestRunner/mac/WebKitTestRunnerPasteboard.mm:175 > + if (data == nil) if (!data) > Tools/WebKitTestRunner/mac/WebKitTestRunnerWindow.h:37 > +@property (nonatomic, assign) WTR::PlatformWebView* platformWebView; assign is the default for this type, so I’d omit it. > Tools/WebKitTestRunner/mac/WebKitTestRunnerWindow.mm:34 > +static Vector<WebKitTestRunnerWindow*> allWindows; Missing space before the *.
Comment on attachment 346630 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=346630&action=review >> Tools/ChangeLog:53 >> + (dumpBackForwardListForAllWindows): User a modern for loop instead of > > Typo: “User” Fixed. >> Tools/DumpRenderTree/mac/MockGeolocationProvider.mm:130 >> + auto webView = (__bridge WebView*)typelessView; > > Missing space before the *. Fixed. >> Tools/TestRunnerShared/cocoa/LayoutTestSpellChecker.mm:254 >> + [resultsForWord addObject:[[LayoutTestTextCheckingResult alloc] initWithType:nsTextCheckingType(WTFMove(typeValue)) range:NSMakeRange(fromValue, toValue - fromValue) replacement:(__bridge NSString *)replacementText.get() details:details.get()]]; > > Isn’t this just leaking now when not using ARC? Added the autorelease back. >> Tools/WebKitTestRunner/mac/WebKitTestRunnerPasteboard.mm:175 >> + if (data == nil) > > if (!data) Done. >> Tools/WebKitTestRunner/mac/WebKitTestRunnerWindow.h:37 >> +@property (nonatomic, assign) WTR::PlatformWebView* platformWebView; > > assign is the default for this type, so I’d omit it. OK. >> Tools/WebKitTestRunner/mac/WebKitTestRunnerWindow.mm:34 >> +static Vector<WebKitTestRunnerWindow*> allWindows; > > Missing space before the *. Fixed.
Committed r234685: <https://trac.webkit.org/changeset/234685>
<rdar://problem/43031590>