Bug 188245 – [Cocoa] More tweaks and refactoring to prepare for ARC

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Darin Adler]

[Cocoa] More tweaks and refactoring to prepare for ARC

[Darin Adler]

Created attachment 346387 [details]
Patch

[Darin Adler]

Wenson, added you because I think you are the original author of the WebImmediateActionController code.

[EWS Watchlist]

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.

[Wenson Hsieh]

(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?

[EWS Watchlist]

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

[Darin Adler]

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.

[Darin Adler]

Created attachment 346479 [details]
Patch

[EWS Watchlist]

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.

[Darin Adler]

Created attachment 346483 [details]
Patch

[EWS Watchlist]

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.

[Darin Adler]

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.

[Darin Adler]

(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.

[Darin Adler]

(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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Darin Adler]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[Darin Adler]

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.

[Darin Adler]

Created attachment 346603 [details]
Patch

[EWS Watchlist]

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.

[Darin Adler]

Created attachment 346604 [details]
Patch

[EWS Watchlist]

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.

[Darin Adler]

Created attachment 346605 [details]
Patch

[EWS Watchlist]

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.

[Darin Adler]

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.

[Darin Adler]

Created attachment 346629 [details]
Patch

[Darin Adler]

Created attachment 346630 [details]
Patch

[EWS Watchlist]

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.

[Darin Adler]

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 *.

[Darin Adler]

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.

[Darin Adler]

Committed r234685: <https://trac.webkit.org/changeset/234685>

[Radar WebKit Bug Importer]

<rdar://problem/43031590>