[webkit-reviews] review granted: [Bug 222237] Use adoptNS() as soon as we [[ObjcClass alloc] init] to avoid leaks : [Attachment 421196] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 222237: Use adoptNS() as soon as we [[ObjcClass alloc] init] to avoid leaks
https://bugs.webkit.org/show_bug.cgi?id=222237

Attachment 421196: Patch

https://bugs.webkit.org/attachment.cgi?id=421196&action=review




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 421196
  --> https://bugs.webkit.org/attachment.cgi?id=421196
Patch

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

> Source/WebKit/UIProcess/mac/WebColorPickerMac.mm:133
>  + (NSPopover *)_colorPopoverCreateIfNecessary:(BOOL)forceCreation

This is a very strange interface for a method. It is really two separate
methods. One always creates if passed YES, overwriting the old one, and the
other never creates if passed NO, so what does "if necessary" even mean?

> Source/WebKit/WebProcess/InjectedBundle/API/mac/WKDOMInternals.mm:87
> +static RetainPtr<WKDOMNode> initWithImpl(WebCore::Node* impl)

Not a good name for a function that creates an object. I think createWrapper
would be a better name.

> Source/WebKit/WebProcess/InjectedBundle/API/mac/WKDOMInternals.mm:134
> +static RetainPtr<WKDOMRange> initWithImpl(WebCore::Range* impl)

Ditto.

> Source/WebKitLegacy/mac/Plugins/WebPluginController.mm:155
> +    auto& views = pluginViews();

Not sure we need a local variable. Could just call pluginViews() twice.

> Source/WebKitLegacy/mac/Plugins/WebPluginController.mm:163
>  #if PLATFORM(IOS_FAMILY)

Do we still need plug-in machinery on iOS? Is it used for video or something?

> Source/WebKitLegacy/mac/Plugins/WebPluginController.mm:166
> +    auto& views = pluginViews();

Ditto.

> Source/WebKitLegacy/mac/Plugins/WebPluginDatabase.mm:66
> +    static NeverDestroyed<RetainPtr<WebPluginDatabase>> _sharedDatabase;

Didn’t comment elsewhere, but are you sure these leading underscore names are
available? I seem to recall that C++ reserved those for the standard library or
something.

> Source/WebKitLegacy/mac/WebView/WebPreferences.mm:380
> +    // FIXME: This check is necessary to avoid recursion (see
<rdar://problem/9564337>), but it also makes standardPreferences() construction
not thread safe.
> +    if (auto& preferences = standardPreferences())
> +	   return preferences.get();

I really do not understand how this helps with anything given how we use
dispatch_once below.

My guess is that it does not prevent recursion.

> Source/WebKitLegacy/mac/WebView/WebView.mm:1352
> +static RetainPtr<NSString> outlookQuirksUserScriptContents()

I’d call this "createXXX".

> Source/WebKitLegacy/mac/WebView/WebView.mm:1374
> +static RetainPtr<NSString> laBanquePostaleQuirksScript()

Ditto.

> Tools/TestRunnerShared/mac/NSPasteboardAdditions.mm:41
>	   if (auto legacyType = adoptNS((__bridge NSString
*)UTTypeCopyPreferredTagWithClass((__bridge CFStringRef)type,
kUTTagClassNSPboardType)))

This is wrong. Must not bridge cast and then do adoptNS. Instead need to do
adoptCF.

> Tools/TestWebKitAPI/Tests/mac/WindowlessWebViewWithMedia.mm:67
> +	   RetainPtr<WebView> webView = adoptNS([[WebView alloc]
initWithFrame:NSMakeRect(0, 0, 120, 200) frameName:nil groupName:nil]);

Can we do auto here?

> Tools/TestWebKitAPI/Tests/mac/WindowlessWebViewWithMedia.mm:68
> +	   RetainPtr<WindowlessWebViewWithMediaFrameLoadDelegate>
testController = adoptNS([WindowlessWebViewWithMediaFrameLoadDelegate new]);

Can we do auto here?