Review remaining usage of autorelease to make sure it is necessary.
Created attachment 420985 [details] Patch
Created attachment 421003 [details] Patch
Created attachment 421053 [details] Patch
Comment on attachment 421053 [details] Patch Can't tell if the safe browsing API test is related or not.
(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.
Created attachment 421076 [details] Patch
Committed r273194: <https://commits.webkit.org/r273194> All reviewed patches have been landed. Closing bug and clearing flags on attachment 421076 [details].
<rdar://problem/74549820>
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 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 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.
(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.