Bug 221780 – Reduce explicit usage of [objC release] in WebKit

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

[Chris Dumez]

Reduce explicit usage of [objC release] in WebKit by using RetainPtr.

[Chris Dumez]

Created attachment 420078 [details]
Patch

[Chris Dumez]

Created attachment 420079 [details]
Patch

[Chris Dumez]

Created attachment 420080 [details]
Patch

[Chris Dumez]

Created attachment 420092 [details]
Patch

[EWS]

Committed r272789: <https://commits.webkit.org/r272789>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 420092 [details].

[Radar WebKit Bug Importer]

<rdar://problem/74282389>

[Sam Weinig]

Comment on attachment 420092 [details]
Patch

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

Awesome!

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1326
>              NSAccessibilitySubroleAttribute,

Not part of this patch, but this should really be switched to use literal array syntax at some point, I believe that makes it so you don't need the nil check as well, as it could just be:

static NSArray* attributes = @[
    NSAccessibilityRoleDescriptionAttribute,
    ....
];

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1370
> +        auto tempArray = adoptNS([[NSMutableArray alloc] initWithArray:attributes]);
>          [tempArray addObject:NSAccessibilityURLAttribute];
>          [tempArray addObject:NSAccessibilityAccessKeyAttribute];
>          [tempArray addObject:NSAccessibilityLinkRelationshipTypeAttribute];
> -        anchorAttrs = [[NSArray alloc] initWithArray:tempArray];
> -        [tempArray release];
> +        anchorAttrs = [[NSArray alloc] initWithArray:tempArray.get()];

I'm not sure I really understand why two arrays are being allocated here. Seems like you could leak tempArray and assign it to anchorAttrs to avoid the second allocation. There isn't really anything gained as far as I know from making a non-mutable array from a mutable array like this. But that is not new, so you don't need to fix it here.

> Source/WebCore/bridge/testbindings.mm:248
> +        auto myInterface = adoptNS([[MyFirstInterface alloc] init]);

Does this file ever actually get used anymore?

> Source/WebCore/editing/cocoa/HTMLConverter.mm:298
>      NSURL *_baseURL;

No RetainPtr for this?

> Source/WebCore/editing/cocoa/HTMLConverter.mm:2325
>          NSFileWrapper *wrapper = [[[NSFileWrapper alloc] initRegularFileWithContents:[cachedResponse data]] autorelease];

Next stop, removing all the "autorelease" calls!

> Source/WebKit/UIProcess/Inspector/ios/WKInspectorHighlightView.h:32
> -    NSMutableArray *_layers; // CAShapeLayers.
> +    RetainPtr<NSMutableArray> _layers; // CAShapeLayers.

Maybe use RetainPtr<NSMutableArray<CAShapeLayer *>> _layers; and remove the comment?

> Source/WebKit/UIProcess/mac/WebColorPickerMac.mm:143
> +        [controller setPopover: popover];

Extra space after the :

> Source/WebKitLegacy/mac/History/WebHistory.mm:289
> -    WebHistoryItem *entry = [_entriesByURL objectForKey:URLString];
> +    RetainPtr<WebHistoryItem> entry = [_entriesByURL objectForKey:URLString];

in most other cases you are using auto entry = retainPtr(...) in cases like this. Seems better to be consistent.

> Source/WebKitLegacy/mac/Plugins/Hosted/NetscapePluginHostManager.mm:178
> +    auto hostProperties = adoptNS([[NSDictionary alloc] initWithObjectsAndKeys:
>                                      visibleName, @"visibleName",
>                                      (NSString *)pluginPath, @"bundlePath",
> -                                    nil];
> +                                    nil]);

Probably better to use use an NSDictionary literal here.

> Source/WebKitLegacy/mac/WebCoreSupport/WebChromeClient.mm:266
> +        auto x = features.x ? adoptNS([[NSNumber alloc] initWithFloat:*features.x]) : nil;
> +        auto y = features.y ? adoptNS([[NSNumber alloc] initWithFloat:*features.y]) : nil;
> +        auto width = features.width ? adoptNS([[NSNumber alloc] initWithFloat:*features.width]) : nil;
> +        auto height = features.height ? adoptNS([[NSNumber alloc] initWithFloat:*features.height]) : nil;
> +        auto menuBarVisible = adoptNS([[NSNumber alloc] initWithBool:features.menuBarVisible]);
> +        auto statusBarVisible = adoptNS([[NSNumber alloc] initWithBool:features.statusBarVisible]);
> +        auto toolBarVisible = adoptNS([[NSNumber alloc] initWithBool:features.toolBarVisible]);
> +        auto scrollbarsVisible = adoptNS([[NSNumber alloc] initWithBool:features.scrollbarsVisible]);
> +        auto resizable = adoptNS([[NSNumber alloc] initWithBool:features.resizable]);
> +        auto fullscreen = adoptNS([[NSNumber alloc] initWithBool:features.fullscreen]);
> +        auto dialog = adoptNS([[NSNumber alloc] initWithBool:features.dialog]);

These would not better using objective-c value literals, e.g. @(...);

> Source/WebKitLegacy/mac/WebCoreSupport/WebChromeClient.mm:460
> -    NSDictionary *dictionary = [[NSDictionary alloc] initWithObjectsAndKeys:
> +    auto dictionary = adoptNS([[NSDictionary alloc] initWithObjectsAndKeys:
>          (NSString *)message, @"message",
>          @(lineNumber), @"lineNumber",
>          @(columnNumber), @"columnNumber",
>          (NSString *)sourceURL, @"sourceURL",
>          messageSource, @"MessageSource",
>          stringForMessageLevel(level), @"MessageLevel",
> -        NULL];
> +        NULL]);

Use dictionary literal here.

> Source/WebKitLegacy/mac/WebCoreSupport/WebEditorClient.mm:459
> +    NSDictionary *dictionary = @{ NSExcludedElementsDocumentAttribute: excludedElements.get() };
>  
>      return dictionary;

Can just return that dictionary. No need for a temporary anymore.

> Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm:838
> +    auto features = adoptNS([[NSDictionary alloc] init]);

Can just use an empty dictionary literal here.

> Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm:1569
> +        auto element = adoptNS([[WebElementDictionary alloc]
> +            initWithHitTestResult:core(m_webFrame.get())->eventHandler().hitTestResultAtPoint(mouseEventData->absoluteLocation, hitType)]);

I'd keep this all on one line.

> Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm:1613
> +    RefPtr<WebCore::Frame> result = [WebFrame _createSubframeWithOwnerElement:&ownerElement frameName:name frameView:childView.get()];

This should use auto. By using RefPtr, this actually looses that the method actually returns a Ref<Frame>.

> Source/WebKitLegacy/mac/WebCoreSupport/WebInspectorClient.mm:570
> +    [window setTitlebarAppearsTransparent: YES];

Extra space after :

> Source/WebKitLegacy/mac/WebView/WebDataSource.mm:321
> +            DOMDocumentFragment *fragment = [[self webFrame] _documentFragmentWithMarkupString:markupString.get() baseURLString:[[mainResource URL] _web_originalDataAsString]];
>              return fragment;

This can be one line.

> Source/WebKitLegacy/mac/WebView/WebDataSource.mm:387
> +        RetainPtr<id> newRep = repClass != nil ? adoptNS([(NSObject *)[repClass alloc] init]) : nil;

I don't think the cast to NSObject * here is needed.

> Source/WebKitLegacy/mac/WebView/WebFrame.mm:305
> +    RetainPtr<WebFrame> frame = adoptNS([[self alloc] _initWithWebFrameView:frameView webView:webView]);

auto?

> Source/WebKitLegacy/mac/WebView/WebFrame.mm:326
> +    RetainPtr<WebFrame> frame = adoptNS([[self alloc] _initWithWebFrameView:frameView webView:webView]);

auto?

> Source/WebKitLegacy/mac/WebView/WebFrame.mm:353
> +    RetainPtr<WebFrame> frame = adoptNS([[self alloc] _initWithWebFrameView:frameView webView:webView]);

auto?

> Source/WebKitLegacy/mac/WebView/WebPDFRepresentation.mm:128
>      doc = nil;

This seems unnecessary.

> Source/WebKitLegacy/mac/WebView/WebPDFView.mm:1104
> +    auto actionsToTags = adoptNS([[NSDictionary alloc] initWithObjectsAndKeys:

NSDictionary literal?

> Source/WebKitLegacy/mac/WebView/WebScriptDebugger.mm:100
> +        auto info = adoptNS([[NSDictionary alloc] initWithObjectsAndKeys:nsErrorMessage, WebScriptErrorDescriptionKey, @(errorLine), WebScriptErrorLineNumberKey, nil]);

NSDictionary literal.

> Source/WebKitLegacy/mac/WebView/WebView.mm:2581
> +    auto features = adoptNS([[NSDictionary alloc] init]);

NSDictionary literal?

> Source/WebKitLegacy/mac/WebView/WebWindowAnimation.mm:183
>      _subAnimation = [animation retain];

Can _subAnimation be in RetainPtr?

> Tools/DumpRenderTree/mac/ResourceLoadDelegate.mm:182
> +        auto nsHeader = adoptNS([[NSString alloc] initWithUTF8String:header->c_str()]);
> +        [newRequest setValue:nil forHTTPHeaderField:nsHeader.get()];

There should be a toNS() function for this (std::string to RetainPtr<NSString>) in DumpRenderTree. Though it may not be in header, I can't recall.

> Tools/DumpRenderTree/mac/TestRunnerMac.mm:851
> +    data = [(__bridge NSString *)adoptCF(JSStringCopyCFString(kCFAllocatorDefault, dataString)).get() dataUsingEncoding:NSUTF8StringEncoding];
> +    baseURL = [NSURL URLWithString:(__bridge NSString *)adoptCF(JSStringCopyCFString(kCFAllocatorDefault, baseURLString)).get()];

I should make a StringFunctions.h like WebKitTestRunner has for these common conversions in DRT. (Or just share and expand the one in WebKitTestRunner more likely).

> Tools/WebKitTestRunner/mac/PlatformWebViewMac.mm:153
> +    [textField setTag: 1];

Extra space after the :

[Darin Adler]

Comment on attachment 420092 [details]
Patch

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

Looked this over carefully to see if I could spot any mistakes. I found one, in WebNSDataExtras.mm. Halfway through writing my comments I saw Sam writing some similar ones.

> Source/WebCore/editing/cocoa/HTMLConverter.mm:1897
> +    RetainPtr<NSMutableParagraphStyle> newStyle;

Seems like we should move this down inside the loop.

> Source/WebCore/platform/ios/WebCoreMotionManager.mm:148
> +        [m_motionManager setDeviceMotionUpdateInterval: kMotionUpdateInterval];

We don’t normally leave spaces after the colons in this kind of code.

> Source/WebCore/platform/ios/WebCoreMotionManager.mm:150
> +        [m_motionManager setAccelerometerUpdateInterval: kMotionUpdateInterval];

Ditto.

> Source/WebCore/platform/network/ios/NetworkStateNotifierIOS.mm:65
>  - (void)dealloc
>  {
> -    [block release];
>      [super dealloc];
>  }

Can delete this method entirely instead.

> Source/WebKit/UIProcess/ios/WKScrollView.mm:223
>  - (void)dealloc
>  {
> -    [_delegateForwarder release];
>      [super dealloc];
>  }

Could remove this method entirely.

>> Source/WebKit/UIProcess/mac/WebColorPickerMac.mm:143
>> +        [controller setPopover: popover];
> 
> Extra space after the :

Space after colon thing here.

> Source/WebKit/WebProcess/WebPage/mac/WKAccessibilityWebPageObjectBase.mm:135
> +    if (parent != m_parent)
> +        m_parent = parent;

No need for the if statement here.

> Source/WebKitLegacy/ios/WebView/WebPDFViewPlaceholder.mm:336
> +        title = adoptNS((NSString *)CGPDFStringCopyTextString(value));

This isn’t good if we ever plan to move to ARC. We don’t want to mix NS/CF retain counts. Instead we want to use adoptCF here. We want a RetainPtr<CFStringRef>. Then we can cast to NSString * either once using a local variable, or three times in the three places this is used.

> Source/WebKitLegacy/mac/DOM/DOMUIKitExtensions.mm:187
> +        auto quadObject = adoptNS([[WKQuadObject alloc] initWithQuad:[self absoluteQuad]]);
> +        quads = @[quadObject.get()];

Seems like this would read better as a on-liner.

> Source/WebKitLegacy/mac/DOM/DOMUIKitExtensions.mm:364
> +    auto quadObject = adoptNS([[WKQuadObject alloc] initWithQuad:[self absoluteQuadWithOwner:owner]]);
> +    NSArray *quadArray = @[quadObject.get()];
>      return quadArray;

Seems like this would read better as a on-liner.

> Source/WebKitLegacy/mac/DOM/ExceptionHandlers.mm:51
> +    auto userInfo = adoptNS([[NSDictionary alloc] initWithObjectsAndKeys:@(description.legacyCode), DOMException, nil]);

Should we consider using the @{} syntax here instead of -[NSDictionary initWithObjectsAndKeys:]?

> Source/WebKitLegacy/mac/DefaultDelegates/WebDefaultUIDelegate.mm:195
>  #if !PLATFORM(IOS_FAMILY)

I suggest we change this to #if PLATFORM(MAC)

> Source/WebKitLegacy/mac/History/WebHistory.mm:143
>  - (void)dealloc
>  {
> -    [_entriesByURL release];
> -    [_orderedLastVisitedDays release];
>      [super dealloc];
>  }

Could remove the whole method.

> Source/WebKitLegacy/mac/History/WebHistory.mm:307
> +    return entry.get();

I think the reason why this code is correct too subtle. Normally it’s incorrect to ever return x.get() where x is a smart pointer. Here it’s OK because it’s owned by some the collection data structure too. Normally we’d want to return entry.autorelease() here instead. And while that would be less efficient, maybe we should consider it.

> Source/WebKitLegacy/mac/History/WebHistory.mm:863
> +    auto entries = adoptNS([[NSArray alloc] initWithObjects:entry, nil]);
> +    [self _sendNotification:WebHistoryItemsAddedNotification entries:entries.get()];

This should use @[] syntax instead:

    [self _sendNotification:WebHistoryItemsAddedNotification entries:@[entry]];

> Source/WebKitLegacy/mac/History/WebHistoryItem.mm:354
>      if (childDicts) {
>          for (int i = [childDicts count] - 1; i >= 0; i--) {

Sure would be nice to find a better idiom for iterating an NSArray backwards.

> Source/WebKitLegacy/mac/Misc/WebNSDataExtras.mm:337
> +                auto newValue = [[NSString alloc] initWithFormat:@"%@, %@", oldValue, value.get()];
> +                value = WTFMove(newValue);

I think there’s a newly introduced leak here. We need adoptNS in the line of code that calls alloc/initWithFormat:. Also, no reason to have the local variable newValue.

> Source/WebKitLegacy/mac/Misc/WebNSPasteboardExtras.mm:319
> +    auto extensions = adoptNS([[NSArray alloc] initWithObjects:extension, nil]);
> +    [self setPropertyList:extensions.get() forType:legacyFilesPromisePasteboardType()];

Array literal would be better here:

    [self setPropertyList:@[extension] forType:legacyFilesPromisePasteboardType()];

Unless extension might be nil, in which case the old code would make an empty array and this code will raise an Objective-C exception. If concerned about that we could write:

    extension ? @[extension] : nil

Or some other solution like that. But probably we should skip the code if extension is nil.

>> Source/WebKitLegacy/mac/Plugins/Hosted/NetscapePluginHostManager.mm:178
>> +                                    nil]);
> 
> Probably better to use use an NSDictionary literal here.

I agree, but two issues with using a literal:

1) It will be autoreleased, which is likely fine.
2) If anything is nil, initWithObjectsAndKeys: just quietly stops at that point and ignores everything else. The dictionary literal ends up raising an Objective-C exception.

Also, this is Netscape plug-in code for Cocoa platforms so … can we delete it?

> Source/WebKitLegacy/mac/Plugins/Hosted/NetscapePluginInstanceProxy.mm:657
> +                                                                  windowFeatures:features.get()];

Should just use @{} here.

>> Source/WebKitLegacy/mac/WebCoreSupport/WebChromeClient.mm:266
>> +        auto dialog = adoptNS([[NSNumber alloc] initWithBool:features.dialog]);
> 
> These would not better using objective-c value literals, e.g. @(...);

You mean "be better". And I think so too. Likely won’t even need local variables.

>> Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm:1613
>> +    RefPtr<WebCore::Frame> result = [WebFrame _createSubframeWithOwnerElement:&ownerElement frameName:name frameView:childView.get()];
> 
> This should use auto. By using RefPtr, this actually looses that the method actually returns a Ref<Frame>.

Loses and loosens.

> Source/WebKitLegacy/mac/WebCoreSupport/WebInspectorClient.mm:515
> +    auto request = adoptNS([[NSURLRequest alloc] initWithURL:[NSURL fileURLWithPath: pagePath]]);

Consider removing the space after colon when touching this line fo code.

> Source/WebKitLegacy/mac/WebCoreSupport/WebInspectorClient.mm:524
>  - (void)dealloc
>  {
> -    [_frontendWebView release];
>      [super dealloc];
>  }

I suggest deleting this.

> Source/WebKitLegacy/mac/WebView/WebFrame.mm:193
>  - (void)dealloc
>  {
> -    [webFrameView release];
> -
>      [super dealloc];
>  }

Can remove.

> Source/WebKitLegacy/mac/WebView/WebFrameView.mm:123
>  - (void)dealloc
>  {
> -    [frameScrollView release];
>      [super dealloc];
>  }

Can remove.

> Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:1916
>      if ([hitView isKindOfClass:[WebHTMLView class]])
>          view = (WebHTMLView *)hitView;    

We have dynamic_objc_cast for cases like this: we can write:

    auto view = retainPtr(dynamic_objc_cast<WebHTMLView>(hitView));

> Source/WebKitLegacy/mac/WebView/WebNavigationData.mm:47
>  - (void)dealloc
>  {
> -    [url release];
> -    [title release];
> -    [originalRequest release];
> -    [response release];
> -    [clientRedirectSource release];
> -
>      [super dealloc];
>  }

Can delete this method.

> Source/WebKitLegacy/mac/WebView/WebPDFRepresentation.mm:124
> +    auto doc = adoptNS([[[[self class] PDFDocumentClass] alloc] initWithData:data]);

When touching the code, should rename "doc" to "document".

> Source/WebKitLegacy/mac/WebView/WebPolicyDelegate.mm:69
>  - (void)dealloc
>  {
> -    [target release];
>      [super dealloc];
>  }

Can delete this.

[Chris Dumez]

Comment on attachment 420092 [details]
Patch

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

>> Source/WebKitLegacy/mac/WebView/WebFrame.mm:305
>> +    RetainPtr<WebFrame> frame = adoptNS([[self alloc] _initWithWebFrameView:frameView webView:webView]);
> 
> auto?

The cases where I have used an explicit RetainPtr<> instead of auto is because the compiler did not like the auto. I usually get a build error about a method call on this variable being ambiguous later on.

[Chris Dumez]

Reopening to attach new patch.

[Chris Dumez]

Created attachment 420174 [details]
Follow-up

[Darin Adler]

Comment on attachment 420174 [details]
Follow-up

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

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1320
> +    static NSArray *anchorAttrs = nil;
> +    static NSArray *webAreaAttrs = nil;
> +    static NSArray *textAttrs = nil;
> +    static NSArray *listAttrs = nil;
> +    static NSArray *listBoxAttrs = nil;
> +    static NSArray *rangeAttrs = nil;
> +    static NSArray *menuAttrs = nil;
> +    static NSArray *menuBarAttrs = nil;
> +    static NSArray *menuItemAttrs = nil;
> +    static NSArray *controlAttrs = nil;
> +    static NSArray *tableAttrs = nil;
> +    static NSArray *tableRowAttrs = nil;
> +    static NSArray *tableColAttrs = nil;
> +    static NSArray *tableCellAttrs = nil;
> +    static NSArray *groupAttrs = nil;
> +    static NSArray *inputImageAttrs = nil;
> +    static NSArray *passwordFieldAttrs = nil;
> +    static NSArray *tabListAttrs = nil;
> +    static NSArray *comboBoxAttrs = nil;
> +    static NSArray *outlineAttrs = nil;
> +    static NSArray *outlineRowAttrs = nil;
> +    static NSArray *buttonAttrs = nil;
> +    static NSArray *scrollViewAttrs = nil;
> +    static NSArray *incrementorAttrs = nil;

Since these are static, could omit the "= nil".

But I am not sure how much style commentary to make here since this code seems unnecessarily complex to me and could be refactored further to simplify.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1464
> +       NSAccessibilityChildrenAttribute];

Indented one space not enough. By the way, I typically format these things like this:

    xxx = @[
        a,
        b,
        c,
    ];

Note the trailing comma on the last element and keeping the [] on their own lines, so the array element lines are all the same and can be reordered or sorted without making syntax errors.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3051
> +    static NSArray * paramAttrs = nil;
> +    static NSArray * textParamAttrs = nil;
> +    static NSArray * tableParamAttrs = nil;
> +    static NSArray * webAreaParamAttrs = nil;

Should remove the spaces after the *, I think. Also, "= nil" not needed for static.

> Source/WebKitLegacy/ios/WebView/WebPDFViewPlaceholder.mm:338
> +    if (CFStringGetLength(title.get())) {

The old version of this code implicitly had a null check in the Objective-C dispatch to the length method. To preserve that behavior we need to add an explicit null check now that we are calling a CFString function. The null case definitely exists, like when the dictionary has no Title in it.

> Source/WebKitLegacy/mac/DefaultDelegates/WebDefaultUIDelegate.mm:195
> -#if !PLATFORM(IOS_FAMILY)
> +#if PLATFORM(MAC)

Not sure I should have pushed you to make this change in this one place. The file this is in has so many other uses of IOS_FAMILY as an indirect way to say Mac and not Mac.

> Source/WebKitLegacy/mac/History/WebHistory.mm:858
> +    NSArray *entries = @[ entry ];
> +    [self _sendNotification:WebHistoryItemsAddedNotification entries:entries];

No need for the local variable. Could just be a one-liner.

> Source/WebKitLegacy/mac/History/WebHistoryItem.mm:353
>      if (childDicts) {

No need for this check any more; the code below will cleanly handle nil without it.

> Source/WebKitLegacy/mac/Plugins/Hosted/NetscapePluginHostManager.mm:175
> +    NSDictionary *hostProperties = @{ @"visibleName":  visibleName, @"bundlePath": (NSString *)pluginPath };

Double space after ":" before "visibleName".

> Source/WebKitLegacy/mac/WebCoreSupport/WebChromeClient.mm:441
> +    NSDictionary *dictionary = @{

Why not auto?

> Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm:1614
> +    WebFrame *newFrame = kit(result.ptr());

I would have used auto here too.

> Source/WebKitLegacy/mac/WebView/WebDataSource.mm:384
> +        RetainPtr<id> newRep = repClass != nil ? adoptNS([[repClass alloc] init]) : nil;

Could also remove the "!= nil" here.

> Source/WebKitLegacy/mac/WebView/WebScriptDebugger.mm:103
> +        NSDictionary *info = @{
> +            WebScriptErrorDescriptionKey: nsErrorMessage,
> +            WebScriptErrorLineNumberKey: @(errorLine)
> +        };

The nsErrorMessage here can be nil. Notice that the code above explicitly uses nsStringNilIfEmpty. The old code would quietly make an empty dictionary in that case. The new code will throw an Objective-C exception. I think the best behavior would be to just omit WebScriptErrorDescriptionKey in that case, which probably means two different dictionary literals depending on whether errorMsg is empty or not.

[Darin Adler]

Comment on attachment 420174 [details]
Follow-up

review- because of the null check removed in WebPDFViewPlaceholder.mm and the nil in the dictionary literal

[Chris Dumez]

Created attachment 420180 [details]
Follow-up

[Darin Adler]

Comment on attachment 420180 [details]
Follow-up

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

Darn it, spotted another possible problem. I don’t think you can store literals into globals safely without retaining.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1321
> +    static NSArray *attributes = @[

Not sure, but in non-ARC code like this I don’t think this is safe. I think it might autorelease and then store without retaining! So you might need to add a retain. Which is like the opposite of what we are trying to do here. Various options:

    static NeverDestroyed<RetainPtr<NSArray>>
    use retainPtr() and leakRef()
    add a retain
    Stop using globals ... why does this function have so many globals? There must be a better way, like storing these in a global object?

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1351
> +    static NSArray *commonMenuAttrs = @[

Ditto.

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1459
> +    static NSArray *menuButtonAttrs = @[

Again.

> Source/WebCore/editing/cocoa/HTMLConverter.mm:1935
> +                    auto newStyle = adoptNS([paragraphStyle mutableCopy]);

Can also delete the "newStyle = nil" line below since we did this.

[Darin Adler]

Comment on attachment 420180 [details]
Follow-up

So unless I am wrong about those literals in globals, this has to be another review-

[Chris Dumez]

Created attachment 420183 [details]
Follow-up

[Chris Dumez]

(In reply to Darin Adler from comment #16)
> Comment on attachment 420180 [details]
> Follow-up
> 
> So unless I am wrong about those literals in globals, this has to be another
> review-

Based on existing usage in the code base, you are very likely right. I have opted for retainPtr().leakRef() which was the minimal change.

[Darin Adler]

Comment on attachment 420183 [details]
Follow-up

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

> Source/WebKitLegacy/mac/History/WebHistoryItem.mm:352
>      NSArray *childDicts = [dict objectForKey:childrenKey];

Could get rid of this local and merge it into the next line.

> Source/WebKitLegacy/mac/Plugins/Hosted/NetscapePluginInstanceProxy.mm:656
> +                                                        windowFeatures:@{ }];

Existing indenting here is lining up the ":" characters. I prefer just putting things on one line, but this mix is messy.

> Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm:1612
> +    auto *newFrame = kit(result.ptr());

I don’t think this should be:

    auto *xxx =

Either of these makes sense to me:

    auto xxx =
    WebFrame *xxx =

And possibly if you want to emphasize it’s a pointer:

    auto* xxx =

But to put the * in the "Objective-C" place and use auto, doesn’t seem right.

> Source/WebKitLegacy/mac/WebView/WebScriptDebugger.mm:100
> +        if (NSString* nsErrorMessage = nsStringNilIfEmpty(errorMsg)) {

Seems like we want to move that "*" before nsErrorMessage. Also, could just name it errorMessage. Or could reverse the logic, write:

    if (errorMsg.isEmpty()) {

And then we would not even have to use nsStringNilIfEmpty at all.

> Source/WebKitLegacy/mac/WebView/WebView.mm:2583
> +                                            windowFeatures:@{ }];

Existing indenting here is lining up the ":" characters. I prefer just putting things on one line, but this mix is messy.

[Chris Dumez]

Created attachment 420195 [details]
Follow-up

[EWS]

Committed r272828: <https://commits.webkit.org/r272828>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 420195 [details].