Bug 222177 – Review remaining usage of autorelease to make sure it is necessary

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

[Chris Dumez]

Review remaining usage of autorelease to make sure it is necessary.

[Chris Dumez]

Created attachment 420985 [details]
Patch

[Chris Dumez]

Created attachment 421003 [details]
Patch

[Chris Dumez]

Created attachment 421053 [details]
Patch

[Geoffrey Garen]

Comment on attachment 421053 [details]
Patch

Can't tell if the safe browsing API test is related or not.

[Chris Dumez]

(In reply to Geoffrey Garen from comment #4)
> Comment on attachment 421053 [details]
> Patch
> 
> Can't tell if the safe browsing API test is related or not.

I will check before landing. Thanks for reviewing.

[Chris Dumez]

Created attachment 421076 [details]
Patch

[EWS]

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

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

[Radar WebKit Bug Importer]

<rdar://problem/74549820>

[Darin Adler]

Comment on attachment 421076 [details]
Patch

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

I spotted at least one serious problem here.

> Source/WebKit/Platform/cocoa/WKPaymentAuthorizationDelegate.mm:71
> +    auto update = !paymentMethodUpdate ? adoptNS([PAL::allocPKPaymentRequestPaymentMethodUpdateInstance() initWithPaymentSummaryItems:_summaryItems.get()]) : nil;

This is wrong! If paymentMethodUpdate is non-nil, update will be nil. The old code did not do that. It set paymentMethodUpdate to update.

> Source/WebKit/Platform/cocoa/WKPaymentAuthorizationDelegate.mm:83
> +    auto update = !shippingContactUpdate ? adoptNS([PAL::allocPKPaymentRequestShippingContactUpdateInstance() initWithErrors:@[] paymentSummaryItems:_summaryItems.get() shippingMethods:_shippingMethods.get()]) : nil;

Same mistake.

> Source/WebKit/Platform/cocoa/WKPaymentAuthorizationDelegate.mm:91
> +    auto update = !shippingMethodUpdate ? adoptNS([PAL::allocPKPaymentRequestShippingMethodUpdateInstance() initWithPaymentSummaryItems:_summaryItems.get()]) : nil;

Same mistake.

[Chris Dumez]

Comment on attachment 421076 [details]
Patch

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

>> Source/WebKit/Platform/cocoa/WKPaymentAuthorizationDelegate.mm:71
>> +    auto update = !paymentMethodUpdate ? adoptNS([PAL::allocPKPaymentRequestPaymentMethodUpdateInstance() initWithPaymentSummaryItems:_summaryItems.get()]) : nil;
> 
> This is wrong! If paymentMethodUpdate is non-nil, update will be nil. The old code did not do that. It set paymentMethodUpdate to update.

Oh, is that what the ?: meant. My bad. I thought :? was an implicit nil :S Will fix.

[Darin Adler]

Comment on attachment 421076 [details]
Patch

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

>>> Source/WebKit/Platform/cocoa/WKPaymentAuthorizationDelegate.mm:71
>>> +    auto update = !paymentMethodUpdate ? adoptNS([PAL::allocPKPaymentRequestPaymentMethodUpdateInstance() initWithPaymentSummaryItems:_summaryItems.get()]) : nil;
>> 
>> This is wrong! If paymentMethodUpdate is non-nil, update will be nil. The old code did not do that. It set paymentMethodUpdate to update.
> 
> Oh, is that what the ?: meant. My bad. I thought :? was an implicit nil :S Will fix.

For future reference, ?: is a gnu/clang extension and this

    a ?: b

is

    a ? a : b

Except it only evaluates "a" once. Since I learned perl first, I think of it as the same as the || operator in perl.

[Chris Dumez]

(In reply to Chris Dumez from comment #10)
> Comment on attachment 421076 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421076&action=review
> 
> >> Source/WebKit/Platform/cocoa/WKPaymentAuthorizationDelegate.mm:71
> >> +    auto update = !paymentMethodUpdate ? adoptNS([PAL::allocPKPaymentRequestPaymentMethodUpdateInstance() initWithPaymentSummaryItems:_summaryItems.get()]) : nil;
> > 
> > This is wrong! If paymentMethodUpdate is non-nil, update will be nil. The old code did not do that. It set paymentMethodUpdate to update.
> 
> Oh, is that what the ?: meant. My bad. I thought :? was an implicit nil :S
> Will fix.

Follow-up fix: <https://commits.webkit.org/r273201>. 

Thanks for spotting this.