Skip to content

Conversation

AZero13
Copy link
Contributor

@AZero13 AZero13 commented Mar 29, 2023

cbd682e8d2841580e93f0c2622355ac49dcca9f4

Avoid smart pointers where we can use convinence initializers
https://bugs.webkit.org/show_bug.cgi?id=254676

Reviewed by NOBODY (OOPS!).

There is no reason to retain or worry about objects that are known to be
autoreleased, such as those from convenience initializers.

* Source/JavaScriptCore/API/JSContext.mm:
* Source/JavaScriptCore/API/JSManagedValue.mm:
* Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:
* Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:
* Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:
* Source/WebCore/bridge/objc/WebScriptObject.mm:
* Source/WebCore/editing/cocoa/DataDetection.mm:
* Source/WebCore/platform/audio/ios/AudioSessionIOS.mm:
* Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:
* Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:
* Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:
* Source/WebKit/Platform/cocoa/PaymentAuthorizationPresenter.mm:
* Source/WebKit/Shared/API/Cocoa/WKRemoteObjectCoder.mm:
* Source/WebKit/Shared/API/Cocoa/_WKRemoteObjectInterface.mm:
* Source/WebKit/Shared/ApplePay/cocoa/PaymentSetupConfiguration.mm:
* Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:
* Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataRecord.mm:
* Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:
* Source/WebKit/UIProcess/API/Cocoa/_WKDownload.mm:
* Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:
* Source/WebKit/UIProcess/Extensions/Cocoa/WebExtensionCocoa.mm:
* Source/WebKit/UIProcess/WebAuthentication/Cocoa/CcidConnection.mm:
* Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:
* Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:
* Source/WebKit/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInNodeHandle.mm:
* Source/WebKit/WebProcess/InjectedBundle/mac/InjectedBundleMac.mm:
* Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:
* Source/WebKitLegacy/mac/Misc/WebNSObjectExtras.mm:
* Source/WebKitLegacy/mac/Plugins/WebPluginContainerCheck.mm:
* Source/WebKitLegacy/mac/Plugins/WebPluginDatabase.mm:
* Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm:
* Source/WebKitLegacy/mac/WebCoreSupport/WebInspectorClient.mm:
* Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:
* Source/WebKitLegacy/mac/WebView/WebPDFView.mm:
* Source/WebKitLegacy/mac/WebView/WebResource.mm:
* Source/WebKitLegacy/mac/WebView/WebView.mm:

b7e7395

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2 ✅ 🛠 jsc-armv7
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 jsc-armv7-tests
✅ 🛠 watch ✅ 🛠 jsc-mips
✅ 🛠 watch-sim ✅ 🧪 jsc-mips-tests

@AZero13 AZero13 requested review from cdumez and a team as code owners March 29, 2023 18:57
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 29, 2023
@AZero13 AZero13 force-pushed the eng/Avoid-having-a-smart-pointer-if-the-end-result-is-an-autoreleased-object branch from 20b28b2 to 64c42b9 Compare March 30, 2023 17:49
@AZero13 AZero13 force-pushed the eng/Avoid-having-a-smart-pointer-if-the-end-result-is-an-autoreleased-object branch from 64c42b9 to 1dc8ba5 Compare March 30, 2023 18:00
@AZero13 AZero13 force-pushed the eng/Avoid-having-a-smart-pointer-if-the-end-result-is-an-autoreleased-object branch from 1dc8ba5 to 77e12a6 Compare March 30, 2023 18:42
@AZero13 AZero13 force-pushed the eng/Avoid-having-a-smart-pointer-if-the-end-result-is-an-autoreleased-object branch from 77e12a6 to 780f35e Compare March 30, 2023 18:56
@AZero13 AZero13 changed the title Avoid smart pointers for autoreleased objects Avoid using smart pointers for autoreleased objects Mar 30, 2023
@AZero13 AZero13 force-pushed the eng/Avoid-having-a-smart-pointer-if-the-end-result-is-an-autoreleased-object branch from 780f35e to 04ef2fc Compare March 30, 2023 19:01
@AZero13 AZero13 changed the title Avoid using smart pointers for autoreleased objects Avoid smart pointers where we can use convinence initializers Mar 30, 2023
@AZero13 AZero13 force-pushed the eng/Avoid-having-a-smart-pointer-if-the-end-result-is-an-autoreleased-object branch from 04ef2fc to 0c44840 Compare March 30, 2023 19:27
@AZero13 AZero13 force-pushed the eng/Avoid-having-a-smart-pointer-if-the-end-result-is-an-autoreleased-object branch from 0c44840 to 11635f4 Compare March 30, 2023 19:39
@AZero13 AZero13 force-pushed the eng/Avoid-having-a-smart-pointer-if-the-end-result-is-an-autoreleased-object branch from 11635f4 to c19ed10 Compare March 30, 2023 20:04
@AZero13 AZero13 force-pushed the eng/Avoid-having-a-smart-pointer-if-the-end-result-is-an-autoreleased-object branch from c19ed10 to d768ec6 Compare March 31, 2023 13:25
@AZero13 AZero13 force-pushed the eng/Avoid-having-a-smart-pointer-if-the-end-result-is-an-autoreleased-object branch from d768ec6 to feb90b1 Compare March 31, 2023 13:40
@AZero13 AZero13 force-pushed the eng/Avoid-having-a-smart-pointer-if-the-end-result-is-an-autoreleased-object branch from feb90b1 to 2ea9a3f Compare March 31, 2023 14:47
@AZero13 AZero13 force-pushed the eng/Avoid-having-a-smart-pointer-if-the-end-result-is-an-autoreleased-object branch from 2ea9a3f to e250f50 Compare March 31, 2023 14:58
@AZero13 AZero13 force-pushed the eng/Avoid-having-a-smart-pointer-if-the-end-result-is-an-autoreleased-object branch from cbd682e to d9a41c5 Compare April 3, 2023 18:15
@AZero13 AZero13 force-pushed the eng/Avoid-having-a-smart-pointer-if-the-end-result-is-an-autoreleased-object branch from d9a41c5 to 316da8e Compare April 3, 2023 18:18
@AZero13
Copy link
Contributor Author

AZero13 commented Apr 3, 2023

This PR appears to have a mix of changes:

  • Some are clearly streamlining the code, where we first adopt autoreleased objects, and then autorelease them to return.
  • Some I'm not certain about. I think that adopting an object removes it from autorelease pool, and manual lifetime management is more performant and memory use friendly, but I'm not sure if I understand this correctly, as I don't see in code how it's done.
  • Some are unrelated to what description says this PR does.

I don't see any example of anything unrelated.

@AZero13 AZero13 force-pushed the eng/Avoid-having-a-smart-pointer-if-the-end-result-is-an-autoreleased-object branch from 316da8e to f803d49 Compare April 3, 2023 18:52
@darinadler
Copy link
Member

I don't see any example of anything unrelated.

Many of these changes don’t involve "convenience initializers".

@AZero13 AZero13 force-pushed the eng/Avoid-having-a-smart-pointer-if-the-end-result-is-an-autoreleased-object branch from f803d49 to e1266e5 Compare April 3, 2023 20:40
@AZero13 AZero13 force-pushed the eng/Avoid-having-a-smart-pointer-if-the-end-result-is-an-autoreleased-object branch from e1266e5 to 3fd3282 Compare April 4, 2023 01:50
@AZero13
Copy link
Contributor Author

AZero13 commented Apr 4, 2023

We should convert to ARC before making a change like this. There is a lot of churn here that does not come with any benefit. For example there is no advantage to using autorelease directly instead of adoptNS and then RetainPtr::autorelease. But once we are using ARC we may not need either.

If we land any part of this it should be a part with some benefit. Like maybe there is a small bit of this change that helps improve performance.

‘I would have my doubts about this even from a long-time contributor but I also think it’s too much to take on for someone new to the project. Luckily there is nothing that connects the separate changes here so we could focus on one small part of this if there is significant benefit in any one change.

Some files by the very nature of their function cannot be converted to ARC. ESPECIALLY when they act as the objective C bridge.

That being said, I will try.

@AZero13 AZero13 force-pushed the eng/Avoid-having-a-smart-pointer-if-the-end-result-is-an-autoreleased-object branch from 3fd3282 to e46b99d Compare April 4, 2023 14:44
@AZero13
Copy link
Contributor Author

AZero13 commented Apr 4, 2023

WebCore CANNOT be converted to ARC because of things like NSInvocation, retainCount, and other hacks NOT supported by ARC

@darinadler
Copy link
Member

darinadler commented Apr 4, 2023

WebCore CANNOT be converted to ARC because of things like NSInvocation, retainCount, and other hacks NOT supported by ARC

This is a huge misunderstanding. Perhaps there is a very small amount of code that cannot be converted. Saying that means "WebCorw cannot be converted to ARC" is incorrect.

I think you may be getting a little out of your depth here as a contributor.

@AZero13 AZero13 marked this pull request as draft April 18, 2023 18:29
@AZero13 AZero13 force-pushed the eng/Avoid-having-a-smart-pointer-if-the-end-result-is-an-autoreleased-object branch from e46b99d to 521cf03 Compare April 18, 2023 18:30
@AZero13 AZero13 force-pushed the eng/Avoid-having-a-smart-pointer-if-the-end-result-is-an-autoreleased-object branch from 521cf03 to 11777d0 Compare April 18, 2023 18:56
@AZero13 AZero13 force-pushed the eng/Avoid-having-a-smart-pointer-if-the-end-result-is-an-autoreleased-object branch from 11777d0 to 1182a76 Compare April 19, 2023 14:23
https://bugs.webkit.org/show_bug.cgi?id=254676

Reviewed by NOBODY (OOPS!).

There is no reason to retain or worry about objects that are known to be
autoreleased, such as those from convenience initializers.

* Source/JavaScriptCore/API/JSContext.mm:
* Source/JavaScriptCore/API/JSManagedValue.mm:
* Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:
* Source/WebCore/accessibility/mac/AXObjectCacheMac.mm:
* Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:
* Source/WebCore/bridge/objc/WebScriptObject.mm:
* Source/WebCore/editing/cocoa/DataDetection.mm:
* Source/WebCore/platform/audio/ios/AudioSessionIOS.mm:
* Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:
* Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:
* Source/WebCore/platform/network/cocoa/WebCoreNSURLSession.mm:
* Source/WebKit/Platform/cocoa/PaymentAuthorizationPresenter.mm:
* Source/WebKit/Shared/API/Cocoa/WKRemoteObjectCoder.mm:
* Source/WebKit/Shared/API/Cocoa/_WKRemoteObjectInterface.mm:
* Source/WebKit/Shared/ApplePay/cocoa/PaymentSetupConfiguration.mm:
* Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:
* Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataRecord.mm:
* Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:
* Source/WebKit/UIProcess/API/Cocoa/_WKDownload.mm:
* Source/WebKit/UIProcess/API/Cocoa/_WKWebAuthenticationPanel.mm:
* Source/WebKit/UIProcess/Extensions/Cocoa/WebExtensionCocoa.mm:
* Source/WebKit/UIProcess/WebAuthentication/Cocoa/CcidConnection.mm:
* Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:
* Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:
* Source/WebKit/WebProcess/InjectedBundle/API/Cocoa/WKWebProcessPlugInNodeHandle.mm:
* Source/WebKit/WebProcess/InjectedBundle/mac/InjectedBundleMac.mm:
* Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:
* Source/WebKitLegacy/mac/Misc/WebNSObjectExtras.mm:
* Source/WebKitLegacy/mac/Plugins/WebPluginContainerCheck.mm:
* Source/WebKitLegacy/mac/Plugins/WebPluginDatabase.mm:
* Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm:
* Source/WebKitLegacy/mac/WebCoreSupport/WebInspectorClient.mm:
* Source/WebKitLegacy/mac/WebView/WebHTMLView.mm:
* Source/WebKitLegacy/mac/WebView/WebPDFView.mm:
* Source/WebKitLegacy/mac/WebView/WebResource.mm:
* Source/WebKitLegacy/mac/WebView/WebView.mm:
@AZero13 AZero13 force-pushed the eng/Avoid-having-a-smart-pointer-if-the-end-result-is-an-autoreleased-object branch from 1182a76 to b7e7395 Compare April 19, 2023 14:26
@justinmichaud justinmichaud added the skip-ews Applied to prevent a change from being run on EWS label May 3, 2023
@AZero13 AZero13 deleted the eng/Avoid-having-a-smart-pointer-if-the-end-result-is-an-autoreleased-object branch October 3, 2024 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merging-blocked Applied to prevent a change from being merged skip-ews Applied to prevent a change from being run on EWS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants