Bug 222177 - Review remaining usage of autorelease to make sure it is necessary
Summary: Review remaining usage of autorelease to make sure it is necessary
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-02-19 09:58 PST by Chris Dumez
Modified: 2021-02-20 10:14 PST (History)
16 users (show)

See Also:


Attachments
Patch (81.46 KB, patch)
2021-02-19 10:07 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (80.60 KB, patch)
2021-02-19 11:57 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (82.16 KB, patch)
2021-02-19 16:00 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (84.14 KB, patch)
2021-02-19 21:01 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2021-02-19 09:58:55 PST
Review remaining usage of autorelease to make sure it is necessary.
Comment 1 Chris Dumez 2021-02-19 10:07:00 PST
Created attachment 420985 [details]
Patch
Comment 2 Chris Dumez 2021-02-19 11:57:13 PST
Created attachment 421003 [details]
Patch
Comment 3 Chris Dumez 2021-02-19 16:00:12 PST
Created attachment 421053 [details]
Patch
Comment 4 Geoffrey Garen 2021-02-19 20:24:25 PST
Comment on attachment 421053 [details]
Patch

Can't tell if the safe browsing API test is related or not.
Comment 5 Chris Dumez 2021-02-19 20:27:43 PST
(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.
Comment 6 Chris Dumez 2021-02-19 21:01:31 PST
Created attachment 421076 [details]
Patch
Comment 7 EWS 2021-02-19 22:37:07 PST
Committed r273194: <https://commits.webkit.org/r273194>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421076 [details].
Comment 8 Radar WebKit Bug Importer 2021-02-19 22:38:14 PST
<rdar://problem/74549820>
Comment 9 Darin Adler 2021-02-20 09:53:18 PST
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.
Comment 10 Chris Dumez 2021-02-20 09:57:03 PST
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.
Comment 11 Darin Adler 2021-02-20 10:08:17 PST
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.
Comment 12 Chris Dumez 2021-02-20 10:14:25 PST
(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.